added ldap_abandon()-like functionality to the XPCOM wrapper, and changed the nsLDAPChannel code to implement request cancellation (eg pressing the stop button in the browser) using said functionality. a=r=(not built).

directory/xpcom/TODO.txt

@@ -6,8 +6,6 @@ housecleaning
 
 * deal with timeouts
 
-* add request cancel/abandon functionality
-
 * come up with a strategy for removing completed nsILDAPOperations from
   the connection's pending request queue
 
@@ -44,7 +42,6 @@ housecleaning
 * implement nsIPipeObserver in nsLDAPChannel in case pipe fills up?
 
 * audit for and implement any appropriate NOT_IMPLEMENTED functions
-  (eg nsLDAPChannel::Cancel() and friends)
 
 * grep for XXXs and fix the issues 
 
@@ -52,8 +49,8 @@ housecleaning
 
 * use bloat tools to ensure code is not doing anything dumb
 
-* re-read (esp nsIChannel) idl and make sure all interfaces are 
+* re-read the IDL for interfaces implemented by nsLDAPChannel.cpp make sure 
-  implemented correctly
+  all interfaces are implemented correctly
 
 * go through the various options that can be used with the SDK function 
   ldap_set_options() and decide if and where the various functions should 
@@ -79,7 +76,7 @@ housecleaning
   boundaries to make sure they do appropriate checking.
 
 * migrate from "#ifdef DEBUG_dmose PR_fprintf(PR_STDERR)" to a more
-  general logging mechanism (NSPR logging facilities? nsIConsoleService?)
+  general logging mechanism (PR_Log)
 
 * does always using this-> for member vars cause inheritance problems?  if so, 
   does it matter in an XPCOM world?

directory/xpcom/base/public/nsILDAPConnection.idl

@@ -82,7 +82,19 @@ interface nsILDAPConnection : nsISupports
      * @exception NS_ERROR_INVALID_POINTER  aOperation was NULL
      * @exception NS_ERROR_UNEXPECTED	    this operation's msgId was not
      *	                                    unique to this connection
+     * @exception NS_ERROR_OUT_OF_MEMORY    out of memory
      */
     void addPendingOperation(in nsILDAPOperation aOperation);
+
+    /**
+     * Remove an nsILDAPOperation from the list of operations pending on this
+     * connection.  Mainly intended for use by the nsLDAPOperation code.
+     *
+     * @param aOperation	operation to add
+     * @exception NS_ERROR_INVALID_POINTER  aOperation was NULL
+     * @exception NS_ERROR_OUT_OF_MEMORY    out of memory
+     * @exception NS_ERROR_FAILURE	    could not delete the operation 
+     */
+    void removePendingOperation(in nsILDAPOperation aOperation);
 };
 

directory/xpcom/base/public/nsILDAPOperation.idl

@@ -107,4 +107,17 @@ interface nsILDAPOperation : nsISupports
                    in string aFilter, in PRIntervalTime aTimeOut,
                    in PRInt32 aSizeLimit);
 
+    /**  
+     * wrapper for ldap_abandon_ext() with NULL LDAPControl
+     * parameters, equivalent to old-style ldap_abandon(), thus the name.
+     *
+     * @exception NS_ERROR_ILLEGAL_VALUE  	an argument was invalid
+     * @exception NS_ERROR_LDAP_ENCODING_ERROR  error during BER-encoding
+     * @exception NS_ERROR_LDAP_SERVER_DOWN	the LDAP server did not
+     *						receive	the request or the
+     *						connection was lost
+     * @exception NS_ERROR_OUT_OF_MEMORY	out of memory
+     * @exception NS_ERROR_UNEXPECTED		internal error
+     */
+    void abandon();
 };

directory/xpcom/base/src/nsLDAPChannel.cpp

@@ -72,7 +72,9 @@ nsLDAPChannel::Init(nsIURI *uri)
     nsresult rv;
 
     mURI = uri;
+    mStatus = NS_OK;
     mReadPipeOffset = 0;
+    mReadPipeClosed = PR_FALSE;
 
     // create an LDAP connection
     //
@@ -114,16 +116,40 @@ nsLDAPChannel::IsPending(PRBool *result)
 NS_IMETHODIMP
 nsLDAPChannel::GetStatus(nsresult *status)
 {
-    return NS_OK;
+    return mStatus;
 }
 
 NS_IMETHODIMP
-nsLDAPChannel::Cancel(nsresult status)
+nsLDAPChannel::Cancel(nsresult aStatus)
 {
-  // should assert if called after OnStop fired?
+    nsresult rv;
+
+    // set the status
     //
-  //NS_NOTYETIMPLEMENTED("nsLDAPChannel::Cancel");
+    mStatus = aStatus;
-  //return NS_ERROR_NOT_IMPLEMENTED;
+
+    // if there is an operation running, abandon it and remove it from the 
+    //
+    if (mCurrentOperation) {
+
+	// if this fails in a non-debug build, there's not much we can do
+	//
+	rv = mCurrentOperation->Abandon();
+	NS_ASSERTION(NS_SUCCEEDED(rv), "nsLDAPChannel::Cancel(): "
+		     "mCurrentOperation->Abandon() failed\n");
+    }
+
+    // if the read pipe exists and hasn't already been closed, close it
+    //
+    if (mReadPipeOut != 0 && !mReadPipeClosed) {
+
+	// if this fails in a non-debug build, there's not much we can do
+	//
+	rv = mReadPipeOut->Close();
+	NS_ASSERTION(NS_SUCCEEDED(rv), "nsLDAPChannel::Cancel(): "
+		     "mReadPipeOut->Close() failed");
+    }
+
     return NS_OK;
 }
 
@@ -504,7 +530,6 @@ nsLDAPChannel::AsyncRead(nsIStreamListener* aListener,
 			 nsISupports* aCtxt)
 {
     nsresult rv;
-    nsCOMPtr<nsILDAPOperation> bindOperation;
     nsXPIDLCString host;
     PRInt32 port;
 
@@ -569,12 +594,13 @@ nsLDAPChannel::AsyncRead(nsIStreamListener* aListener,
 
     // create and initialize an LDAP operation (to be used for the bind)
     //	
-    bindOperation = do_CreateInstance("mozilla.network.ldapoperation", &rv);
+    mCurrentOperation = do_CreateInstance("mozilla.network.ldapoperation", 
+					  &rv);
     NS_ENSURE_SUCCESS(rv, rv);
 
     // our OnLDAPMessage accepts all result callbacks
     //
-    rv = bindOperation->Init(mConnection, this);
+    rv = mCurrentOperation->Init(mConnection, this);
     NS_ENSURE_SUCCESS(rv, rv);
 
     // kick off a bind operation 
@@ -583,10 +609,11 @@ nsLDAPChannel::AsyncRead(nsIStreamListener* aListener,
 #ifdef DEBUG_dmose
     PR_fprintf(PR_STDERR, "initiating SimpleBind\n");
 #endif
-    rv = bindOperation->SimpleBind(NULL);
+    rv = mCurrentOperation->SimpleBind(NULL);
     if (NS_FAILED(rv)) {
 #ifdef DEBUG
-	PR_fprintf(PR_STDERR, "bindOperation->SimpleBind failed. rv=%d\n", rv);
+	PR_fprintf(PR_STDERR, "mCurrentOperation->SimpleBind failed. rv=%d\n",
+		   rv);
 #endif
 	return(rv);
     }
@@ -681,7 +708,6 @@ nsLDAPChannel::OnLDAPMessage(nsILDAPMessage *aMessage, PRInt32 aRetVal)
 nsresult
 nsLDAPChannel::OnLDAPBind(nsILDAPMessage *aMessage) 
 {
-    nsCOMPtr<nsILDAPOperation> searchOperation;
     nsCOMPtr<nsILDAPURL> url;
     nsXPIDLCString baseDn;
     nsXPIDLCString filter;
@@ -690,12 +716,15 @@ nsLDAPChannel::OnLDAPBind(nsILDAPMessage *aMessage)
 
     // XXX should call ldap_parse_result() here
 
+    mCurrentOperation = 0;	// done with bind operation
+
     // create and initialize an LDAP operation (to be used for the bind)
     //	
-    searchOperation = do_CreateInstance("mozilla.network.ldapoperation", &rv);
+    mCurrentOperation = do_CreateInstance("mozilla.network.ldapoperation", 
+					  &rv);
     NS_ENSURE_SUCCESS(rv, rv);
 
-    rv = searchOperation->Init(mConnection, this);
+    rv = mCurrentOperation->Init(mConnection, this);
     NS_ENSURE_SUCCESS(rv, rv);
 
     // QI() the URI to an nsILDAPURL so we get can the LDAP specific portions
@@ -730,7 +759,7 @@ nsLDAPChannel::OnLDAPBind(nsILDAPMessage *aMessage)
 #ifdef DEBUG_dmose
     PR_fprintf(PR_STDERR, "bind completed; starting search\n");
 #endif
-    rv = searchOperation->SearchExt(baseDn, scope, filter, 0, LDAP_NO_LIMIT);
+    rv = mCurrentOperation->SearchExt(baseDn, scope, filter, 0, LDAP_NO_LIMIT);
     NS_ENSURE_SUCCESS(rv,rv);
     
     return NS_OK;
@@ -758,19 +787,21 @@ nsLDAPChannel::OnLDAPSearchResult(nsILDAPMessage *aMessage)
 	return NS_ERROR_FAILURE;
     }
 
-    // XXXdmose this is synchronous!  (and presumably could conceivably stall)
-    // should check SDK code to verify, and somehow deal with this better.
-    //
-    mConnection = 0;
-
-    // all done
-    //
-    mListener->OnStopRequest(this, mResponseContext, NS_OK, nsnull);
-
     // close the pipe 
     //
     rv = mReadPipeOut->Close();
     NS_ENSURE_SUCCESS(rv, rv);
+    mReadPipeClosed = PR_TRUE;
+
+    // we're done with the current operation.  cause nsCOMPtr to Release() it
+    // so that if nsLDAPChannel::Cancel gets called, that doesn't try to call 
+    // mCurrentOperation->Abandon().
+    //
+    mCurrentOperation = 0;
+
+    // all done
+    //
+    mListener->OnStopRequest(this, mResponseContext, NS_OK, nsnull);
 
     return NS_OK;
 }

directory/xpcom/base/src/nsLDAPChannel.h

@@ -97,7 +97,10 @@ protected:
   nsCOMPtr<nsISupports> mResponseContext; 
   nsCOMPtr<nsIBufferInputStream> mReadPipeIn; // this end given to the listener
   nsCOMPtr<nsIBufferOutputStream> mReadPipeOut; // for writes from the channel
+  nsCOMPtr<nsILDAPOperation> mCurrentOperation; // current ldap operation
   PRUint32 mReadPipeOffset; // how many bytes written so far?
+  PRBool mReadPipeClosed; // has the pipe already been closed?
+  nsresult mStatus;
 
 };
 

directory/xpcom/base/src/nsLDAPConnection.cpp

@@ -133,8 +133,7 @@ nsLDAPConnection::Init(const char *aHost, PRInt16 aPort, const char *aBindName)
     // be threadsafe
     //
     mPendingOperations = new nsSupportsHashtable(10, PR_TRUE);
-    if (!mPendingOperations)
+    NS_ENSURE_TRUE(mPendingOperations, NS_ERROR_FAILURE);
-	return NS_ERROR_FAILURE;
 
 #ifdef DEBUG_dmose
     const int lDebug = 0;
@@ -268,6 +267,58 @@ nsLDAPConnection::AddPendingOperation(nsILDAPOperation *aOperation)
     return NS_OK;
 }
 
+/**
+ * Remove an nsILDAPOperation from the list of operations pending on this
+ * connection.  Mainly intended for use by the nsLDAPOperation code.
+ *
+ * @param aOperation	operation to add
+ * @exception NS_ERROR_INVALID_POINTER  aOperation was NULL
+ * @exception NS_ERROR_OUT_OF_MEMORY    out of memory
+ * @exception NS_ERROR_FAILURE	    could not delete the operation 
+ *
+ * void removePendingOperation(in nsILDAPOperation aOperation);
+ */
+NS_IMETHODIMP
+nsLDAPConnection::RemovePendingOperation(nsILDAPOperation *aOperation)
+{
+    nsresult rv;
+    PRInt32 msgId;
+
+    NS_ENSURE_ARG_POINTER(aOperation);
+
+    // find the message id
+    //
+    rv = aOperation->GetMessageId(&msgId);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // turn it into an nsVoidKey.  note that this is another spot that
+    // assumes that sizeof(void*) >= sizeof(PRInt32).  
+    //
+    // XXXdmose  should really create an nsPRInt32Key.
+    //
+    nsVoidKey *key = new nsVoidKey(NS_REINTERPRET_CAST(void *, msgId));
+    if (!key) {
+	return NS_ERROR_OUT_OF_MEMORY;
+    }
+
+    if (!mPendingOperations->Remove(key)) {
+#ifdef DEBUG
+	PR_fprintf(PR_STDERR, "nsLDAPConnection::RemovePendingOperation was\n"
+		   " unable to remove the requested item from the pending\n"
+		   " operations queue.  This probably means that the item\n"
+		   " in question didn't exist in the queue, which in turn\n"
+		   " probably means that you have found a bug in the code\n"
+		   " that calls this function.\n");
+#endif
+
+	delete key;
+	return NS_ERROR_FAILURE;
+    }
+
+    delete key;
+    return NS_OK;
+}
+
 // for nsIRunnable.  this thread spins in ldap_result() awaiting the next
 // message.  once one arrives, it dispatches it to the nsILDAPMessageListener 
 // on the main thread.
@@ -341,6 +392,12 @@ nsLDAPConnection::Run(void)
 	    //
 	    msg = 0;
 
+#if 0
+	    // sleep for a while to workaround event queue flooding so that
+	    // it's possible to test pressing the stop button.
+	    //
+	    PR_Sleep(2000);
+#endif
 	    break;
 	}	
 
@@ -390,13 +447,16 @@ nsLDAPConnection::InvokeMessageCallback(LDAPMessage *aMsgHandle,
     //
     nsISupports *data = mPendingOperations->Get(key);
     if (data == nsnull) {
-#ifdef DEBUG
+#ifdef DEBUG_dmose
 	PR_fprintf(PR_STDERR, "InvokeMessageCallback(): couldn't find "
 		   "nsILDAPOperation corresponding to this message id\n");
 #endif
 	delete key;
 
-	return NS_ERROR_UNEXPECTED;
+	// this may well be ok, since it could just mean that the operation
+	// was aborted while some number of messages were already in transit.
+	//
+	return NS_OK;
     }
 
     operation = getter_AddRefs(NS_STATIC_CAST(nsILDAPOperation *, data));

directory/xpcom/base/src/nsLDAPOperation.cpp

@@ -66,6 +66,11 @@ NS_IMETHODIMP
 nsLDAPOperation::Init(nsILDAPConnection *aConnection,
 		      nsILDAPMessageListener *aMessageListener)
 {
+
+    // so we know that the operation is not yet running
+    //
+    mMsgId = 0;
+
     // set the connection 
     //
     mConnection = aConnection;
@@ -275,3 +280,73 @@ nsLDAPOperation::GetMessageId(PRInt32 *aMsgId)
    
     return NS_OK;
 }
+
+nsresult
+nsLDAPOperation::AbandonExt(LDAPControl **serverctrls,
+			    LDAPControl **clientctrls)
+{
+    nsresult rv;
+    int retVal;
+
+    NS_ENSURE_TRUE(mMessageListener != 0, NS_ERROR_NOT_INITIALIZED);
+    NS_ENSURE_TRUE(mMsgId != 0, NS_ERROR_NOT_INITIALIZED);
+
+    retVal = ldap_abandon_ext(mConnectionHandle, mMsgId, serverctrls, 
+				  clientctrls);
+    switch (retVal) {
+
+    case LDAP_SUCCESS:
+	break;
+
+    case LDAP_PARAM_ERROR:
+	return NS_ERROR_ILLEGAL_VALUE;
+	break;
+
+    case LDAP_ENCODING_ERROR:
+	return NS_ERROR_LDAP_ENCODING_ERROR;
+	break;
+    
+    case LDAP_SERVER_DOWN:
+	return NS_ERROR_LDAP_SERVER_DOWN;
+	break;
+
+    case LDAP_NO_MEMORY:
+	return NS_ERROR_OUT_OF_MEMORY;
+	break;
+
+    default: 
+	// XXX PR_Log stuff here
+	//
+	return NS_ERROR_UNEXPECTED;
+    }
+
+    // try to remove it from the pendingOperations queue, if it's there.
+    // even if something goes wrong here, the abandon() has already succeeded
+    // succeeded (and there's nothing else the caller can reasonably do), 
+    // so we only pay attention to this in debug builds.
+    //
+    rv = mConnection->RemovePendingOperation(this);
+    NS_ASSERTION(NS_SUCCEEDED(rv), "nsLDAPOperation::AbandonExt: "
+		 "mConnection->RemovePendingOperation(this) failed\n");
+
+    return NS_OK;
+}
+
+/**  
+ * wrapper for ldap_abandon_ext() with NULL LDAPControl
+ * parameters, equivalent to old-style ldap_abandon(), thus the name.
+ *
+ * @exception NS_ERROR_ILLEGAL_VALUE  	    	an argument was invalid
+ * @exception NS_ERROR_LDAP_ENCODING_ERROR  	error during BER-encoding
+ * @exception NS_ERROR_LDAP_SERVER_DOWN	    	the LDAP server did not
+ *					    	receive the request or the
+ *					    	connection was lost
+ * @exception NS_ERROR_OUT_OF_MEMORY	    	out of memory
+ * @exception NS_ERROR_UNEXPECTED		internal error
+ */
+
+NS_IMETHODIMP
+nsLDAPOperation::Abandon(void)
+{
+    return nsLDAPOperation::AbandonExt(NULL, NULL);
+}

directory/xpcom/base/src/nsLDAPOperation.h

@@ -58,8 +58,11 @@ class nsLDAPOperation : public nsILDAPOperation
     virtual ~nsLDAPOperation();
 
   protected:
-    // wrapper for ldap_search_ext 
+    /**
-    //
+     * wrapper for ldap_search_ext()
+     *
+     * XXX should move to idl, once LDAPControls have an IDL representation
+     */
     int SearchExt(const char *base, // base DN to search
 		  int scope, // LDAP_SCOPE_{BASE,ONELEVEL,SUBTREE}
 		  const char* filter, // search filter
@@ -70,6 +73,13 @@ class nsLDAPOperation : public nsILDAPOperation
 		  struct timeval *timeoutp, // how long to wait
 		  int sizelimit); // max # of entries to return
 
+    /** 
+     * wrapper for ldap_abandon_ext().
+     *
+     * XXX should move to idl, once LDAPControls have an IDL representation
+     */
+    nsresult AbandonExt(LDAPControl **serverctrls, LDAPControl **clientctrls);
+
     nsCOMPtr<nsILDAPConnection> mConnection; // connection this op is on
     nsCOMPtr<nsILDAPMessageListener> mMessageListener; // results go here
     PRInt32 mMsgId;	     // opaque handle to outbound message for this op