Bug 1460871 - Add request number for each ldap lookup to avoid adding results from older queries. r=jorgk DONTBUILD
authorJan Horak <jhorak@redhat.com>
Fri, 03 Aug 2018 18:28:16 +0200
changeset 31979 eddeeab5cd43946438025ab16dbe46248f844ab0
parent 31978 5d37f39381a374dcd8c39b258c944ac199c0d60c
child 31980 87fcfb8fc1d48d7c45fbc10e4e23d27ed8b88287
push id2308
push userclokep@gmail.com
push dateWed, 05 Sep 2018 00:34:58 +0000
treeherdercomm-beta@e326b2dcd127 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorgk
bugs1460871
Bug 1460871 - Add request number for each ldap lookup to avoid adding results from older queries. r=jorgk DONTBUILD The waiting until the query ends and ignoring results could lead to ignoring results from subsequent lookup. I've removed the code which waited for the all results and replaced it by request number. Every LDAP operation now has the request number set during creating and its value is compared to the current request number (static variable). If they don't match the result is ignored in nsAbQueryLDAPMessageListener::OnLDAPMessage. MozReview-Commit-ID: 9ShVyHmCNS7
ldap/xpcom/public/nsILDAPOperation.idl
ldap/xpcom/src/nsLDAPOperation.cpp
ldap/xpcom/src/nsLDAPOperation.h
mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp
mailnews/addrbook/src/nsAbLDAPListenerBase.cpp
mailnews/addrbook/src/nsAbLDAPListenerBase.h
--- a/ldap/xpcom/public/nsILDAPOperation.idl
+++ b/ldap/xpcom/public/nsILDAPOperation.idl
@@ -47,16 +47,20 @@ interface nsILDAPOperation : nsISupports
      * @exception NS_ERROR_ILLEGAL_VALUE        a NULL pointer was passed in
      */
     readonly attribute long messageID;
 
     /**
      * private parameter (anything caller desires)
      */
     attribute nsISupports closure;
+    /**
+     * number of the request for compare that the request is still valid.
+     */
+    attribute unsigned long requestNum;
 
     /**
      * No time and/or size limit specified
      */
     const long NO_LIMIT = 0;
 
     /**
      * If specified, these arrays of nsILDAPControls are passed into the LDAP
--- a/ldap/xpcom/src/nsLDAPOperation.cpp
+++ b/ldap/xpcom/src/nsLDAPOperation.cpp
@@ -395,16 +395,29 @@ convertControlArray(nsIArray *aXpcomArra
         }
         ++i;
     }
 
     *aArray = controls;
     return NS_OK;
 }
 
+  /* attribute unsigned long requestNum; */
+NS_IMETHODIMP nsLDAPOperation::GetRequestNum(uint32_t *aRequestNum)
+{
+    *aRequestNum = mRequestNum;
+    return NS_OK;
+}
+
+NS_IMETHODIMP nsLDAPOperation::SetRequestNum(uint32_t aRequestNum)
+{
+    mRequestNum = aRequestNum;
+    return NS_OK;
+}
+
 NS_IMETHODIMP
 nsLDAPOperation::SearchExt(const nsACString& aBaseDn, int32_t aScope,
                            const nsACString& aFilter,
                            const nsACString &aAttributes,
                            PRIntervalTime aTimeOut, int32_t aSizeLimit)
 {
     if (!mMessageListener) {
         NS_ERROR("nsLDAPOperation::SearchExt(): mMessageListener not set");
--- a/ldap/xpcom/src/nsLDAPOperation.h
+++ b/ldap/xpcom/src/nsLDAPOperation.h
@@ -32,16 +32,19 @@ class nsLDAPOperation : public nsILDAPOp
     //
     nsLDAPOperation();
 
     /**
      * used to break cycles
      */
     void Clear();
 
+    // Stores the request number for later check of the operation is still valid
+    uint32_t mRequestNum;
+
   private:
     virtual ~nsLDAPOperation();
 
     /**
      * wrapper for ldap_add_ext()
      *
      * XXX should move to idl, once LDAPControls have an IDL representation
      */
--- a/mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp
+++ b/mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp
@@ -18,16 +18,18 @@
 #include "nsCategoryManagerUtils.h"
 #include "nsAbLDAPDirectory.h"
 #include "nsAbLDAPListenerBase.h"
 #include "nsXPCOMCIDInternal.h"
 #include "nsIURIMutator.h"
 
 using namespace mozilla;
 
+extern mozilla::LazyLogModule gLDAPLogModule;  // defined in nsLDAPService.cpp
+
 // nsAbLDAPListenerBase inherits nsILDAPMessageListener
 class nsAbQueryLDAPMessageListener : public nsAbLDAPListenerBase
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
 
   // Note that the directoryUrl is the details of the ldap directory
   // without any search params or return attributes specified. The searchUrl
@@ -62,17 +64,16 @@ protected:
   nsCOMPtr<nsILDAPURL> mSearchUrl;
   nsIAbDirectoryQueryResultListener *mResultListener;
   int32_t mContextID;
   nsCOMPtr<nsIAbDirectoryQueryArguments> mQueryArguments;
   int32_t mResultLimit;
 
   bool mFinished;
   bool mCanceled;
-  bool mWaitingForPrevQueryToFinish;
 
   nsCOMPtr<nsIMutableArray> mServerSearchControls;
   nsCOMPtr<nsIMutableArray> mClientSearchControls;
 };
 
 
 NS_IMPL_ISUPPORTS(nsAbQueryLDAPMessageListener, nsILDAPMessageListener)
 
@@ -90,17 +91,16 @@ nsAbQueryLDAPMessageListener::nsAbQueryL
         const int32_t timeOut) :
   nsAbLDAPListenerBase(directoryUrl, connection, login, timeOut),
   mSearchUrl(searchUrl),
   mResultListener(resultListener),
   mQueryArguments(queryArguments),
   mResultLimit(resultLimit),
   mFinished(false),
   mCanceled(false),
-  mWaitingForPrevQueryToFinish(false),
   mServerSearchControls(serverSearchControls),
   mClientSearchControls(clientSearchControls)
 {
   mSaslMechanism.Assign(mechanism);
 }
 
 nsAbQueryLDAPMessageListener::~nsAbQueryLDAPMessageListener ()
 {
@@ -112,37 +112,44 @@ nsresult nsAbQueryLDAPMessageListener::C
     NS_ENSURE_SUCCESS(rv, rv);
 
     MutexAutoLock lock(mLock);
 
     if (mFinished || mCanceled)
         return NS_OK;
 
     mCanceled = true;
-    if (!mFinished)
-      mWaitingForPrevQueryToFinish = true;
-
     return NS_OK;
 }
 
 NS_IMETHODIMP nsAbQueryLDAPMessageListener::OnLDAPMessage(nsILDAPMessage *aMessage)
 {
   nsresult rv = Initiate();
   NS_ENSURE_SUCCESS(rv, rv);
 
   int32_t messageType;
   rv = aMessage->GetType(&messageType);
+  uint32_t requestNum;
+  mOperation->GetRequestNum(&requestNum);
   NS_ENSURE_SUCCESS(rv, rv);
 
   bool cancelOperation = false;
 
   // Enter lock
   {
     MutexAutoLock lock (mLock);
 
+    if (requestNum != sCurrentRequestNum) {
+      MOZ_LOG(gLDAPLogModule, mozilla::LogLevel::Debug,
+           ("nsAbQueryLDAPMessageListener::OnLDAPMessage: Ignoring message with "
+            "request num %" PRIx32 ", current request num is %" PRIx32 ".",
+            requestNum, sCurrentRequestNum));
+      return NS_OK;
+    }
+
     if (mFinished)
       return NS_OK;
 
     if (messageType == nsILDAPMessage::RES_SEARCH_RESULT)
       mFinished = true;
     else if (mCanceled)
     {
       mFinished = true;
@@ -162,21 +169,20 @@ NS_IMETHODIMP nsAbQueryLDAPMessageListen
       rv = OnLDAPMessageBind(aMessage);
       if (NS_FAILED(rv))
         // We know the bind failed and hence the message has an error, so we
         // can just call SearchResult with the message and that'll sort it out
         // for us.
         rv = OnLDAPMessageSearchResult(aMessage);
       break;
     case nsILDAPMessage::RES_SEARCH_ENTRY:
-      if (!mFinished && !mWaitingForPrevQueryToFinish)
+      if (!mFinished)
         rv = OnLDAPMessageSearchEntry(aMessage);
       break;
     case nsILDAPMessage::RES_SEARCH_RESULT:
-      mWaitingForPrevQueryToFinish = false;
       rv = OnLDAPMessageSearchResult(aMessage);
       NS_ENSURE_SUCCESS(rv, rv);
       break;
     default:
       break;
     }
   }
   else
@@ -203,16 +209,18 @@ nsresult nsAbQueryLDAPMessageListener::D
   mCanceled = mFinished = false;
 
   mOperation = do_CreateInstance(NS_LDAPOPERATION_CONTRACTID, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = mOperation->Init(mConnection, this, nullptr);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  mOperation->SetRequestNum(++sCurrentRequestNum);
+
   nsAutoCString dn;
   rv = mSearchUrl->GetDn(dn);
   NS_ENSURE_SUCCESS(rv, rv);
 
   int32_t scope;
   rv = mSearchUrl->GetScope(&scope);
   NS_ENSURE_SUCCESS(rv, rv);
 
--- a/mailnews/addrbook/src/nsAbLDAPListenerBase.cpp
+++ b/mailnews/addrbook/src/nsAbLDAPListenerBase.cpp
@@ -15,16 +15,18 @@
 #include "nsILoginInfo.h"
 #include "nsServiceManagerUtils.h"
 #include "nsComponentManagerUtils.h"
 #include "nsMemory.h"
 #include "mozilla/Services.h"
 
 using namespace mozilla;
 
+uint32_t nsAbLDAPListenerBase::sCurrentRequestNum = 0;
+
 nsAbLDAPListenerBase::nsAbLDAPListenerBase(nsILDAPURL* url,
                                            nsILDAPConnection* connection,
                                            const nsACString &login,
                                            const int32_t timeOut) :
   mDirectoryUrl(url), mConnection(connection), mLogin(login),
   mTimeOut(timeOut), mBound(false), mInitialized(false),
   mLock("nsAbLDAPListenerBase.mLock")
 {
@@ -244,16 +246,17 @@ NS_IMETHODIMP nsAbLDAPListenerBase::OnLD
 
   rv = mOperation->Init(mConnection, this, nullptr);
   if (NS_FAILED(rv))
   {
     NS_ERROR("nsAbLDAPMessageBase::OnLDAPInit(): failed to Initialise operation");
     InitFailed();
     return rv;
   }
+  mOperation->SetRequestNum(++sCurrentRequestNum);
 
   // Try non-password mechanisms first
   if (mSaslMechanism.EqualsLiteral("GSSAPI"))
   {
     nsAutoCString service;
     rv = mDirectoryUrl->GetAsciiHost(service);
     NS_ENSURE_SUCCESS(rv, rv);
 
--- a/mailnews/addrbook/src/nsAbLDAPListenerBase.h
+++ b/mailnews/addrbook/src/nsAbLDAPListenerBase.h
@@ -42,13 +42,14 @@ protected:
   nsCOMPtr<nsILDAPURL> mDirectoryUrl;
   nsCOMPtr<nsILDAPOperation> mOperation;        // current ldap op
   nsILDAPConnection* mConnection;
   nsCString mLogin;
   nsCString mSaslMechanism;
   int32_t mTimeOut;
   bool mBound;
   bool mInitialized;
+  static uint32_t sCurrentRequestNum;
 
   mozilla::Mutex mLock;
 };
 
 #endif