Bug 1110935 - Part 2: Remove ReentrantMonitor and ReentrantMonitorAutoEnter uses. draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Sun, 01 Nov 2015 20:23:40 -0800
changeset 304991 09814d657cf5508c42c5f23efd4392da55430d3d
parent 304990 fdd5a75c0cc6cc92463ce9659e9ff97b8da6fcb8
child 304992 df4fb2e9a75fcdf5412d282f4e7b7d37e3cf422d
push id7016
push usercykesiopka.bmo@gmail.com
push dateMon, 02 Nov 2015 05:02:59 +0000
bugs1110935
milestone45.0a1
Bug 1110935 - Part 2: Remove ReentrantMonitor and ReentrantMonitorAutoEnter uses.
security/manager/ssl/nsSecureBrowserUIImpl.cpp
security/manager/ssl/nsSecureBrowserUIImpl.h
--- a/security/manager/ssl/nsSecureBrowserUIImpl.cpp
+++ b/security/manager/ssl/nsSecureBrowserUIImpl.cpp
@@ -94,18 +94,17 @@ class nsAutoAtomic {
     Atomic<int32_t> &mI;
 
   private:
     nsAutoAtomic(); // not accessible
 };
 #endif
 
 nsSecureBrowserUIImpl::nsSecureBrowserUIImpl()
-  : mReentrantMonitor("nsSecureBrowserUIImpl.mReentrantMonitor")
-  , mNotifiedSecurityState(lis_no_security)
+  : mNotifiedSecurityState(lis_no_security)
   , mNotifiedToplevelIsEV(false)
   , mNewToplevelSecurityState(STATE_IS_INSECURE)
   , mNewToplevelIsEV(false)
   , mNewToplevelSecurityStateKnown(true)
   , mIsViewSource(false)
   , mSubRequestsBrokenSecurity(0)
   , mSubRequestsNoSecurity(0)
   , mRestoreSubrequests(false)
@@ -186,18 +185,18 @@ nsSecureBrowserUIImpl::Init(nsIDOMWindow
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsSecureBrowserUIImpl::GetState(uint32_t* aState)
 {
   MOZ_ASSERT(NS_IsMainThread());
-  ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-  return MapInternalToExternalState(aState, mNotifiedSecurityState, mNotifiedToplevelIsEV);
+  return MapInternalToExternalState(aState, mNotifiedSecurityState,
+                                    mNotifiedToplevelIsEV);
 }
 
 // static
 already_AddRefed<nsISupports> 
 nsSecureBrowserUIImpl::ExtractSecurityInfo(nsIRequest* aRequest)
 {
   nsCOMPtr<nsISupports> retval;
   nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));
@@ -377,20 +376,19 @@ nsSecureBrowserUIImpl::OnProgressChange(
                                         int32_t aMaxSelfProgress,
                                         int32_t aCurTotalProgress,
                                         int32_t aMaxTotalProgress)
 {
   NS_NOTREACHED("notification excluded in AddProgressListener(...)");
   return NS_OK;
 }
 
-void nsSecureBrowserUIImpl::ResetStateTracking()
+void
+nsSecureBrowserUIImpl::ResetStateTracking()
 {
-  ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-
   mDocumentRequestsInProgress = 0;
   mTransferringRequests.Clear();
 }
 
 void
 nsSecureBrowserUIImpl::EvaluateAndUpdateSecurityState(nsIRequest* aRequest,
                                                       nsISupports *info,
                                                       bool withNewLocation,
@@ -424,55 +422,49 @@ nsSecureBrowserUIImpl::EvaluateAndUpdate
           temp_NewToplevelIsEV = aTemp;
         }
       }
     }
 
   // assume temp_NewToplevelSecurityState was set in this scope!
   // see code that is directly above
 
-  {
-    ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-    mNewToplevelSecurityStateKnown = true;
-    mNewToplevelSecurityState = temp_NewToplevelSecurityState;
-    mNewToplevelIsEV = temp_NewToplevelIsEV;
-    if (updateStatus) {
-      mSSLStatus = temp_SSLStatus;
-    }
-    MOZ_LOG(gSecureDocLog, LogLevel::Debug,
-           ("SecureUI:%p: remember securityInfo %p\n", this,
-            info));
-    nsCOMPtr<nsIAssociatedContentSecurity> associatedContentSecurityFromRequest =
-        do_QueryInterface(aRequest);
-    if (associatedContentSecurityFromRequest)
-        mCurrentToplevelSecurityInfo = aRequest;
-    else
-        mCurrentToplevelSecurityInfo = info;
+  mNewToplevelSecurityStateKnown = true;
+  mNewToplevelSecurityState = temp_NewToplevelSecurityState;
+  mNewToplevelIsEV = temp_NewToplevelIsEV;
+  if (updateStatus) {
+    mSSLStatus = temp_SSLStatus;
+  }
+  MOZ_LOG(gSecureDocLog, LogLevel::Debug,
+         ("SecureUI:%p: remember securityInfo %p\n", this,
+          info));
+  nsCOMPtr<nsIAssociatedContentSecurity> associatedContentSecurityFromRequest(
+    do_QueryInterface(aRequest));
+  if (associatedContentSecurityFromRequest) {
+    mCurrentToplevelSecurityInfo = aRequest;
+  } else {
+    mCurrentToplevelSecurityInfo = info;
+  }
 
-    // The subrequest counters are now in sync with 
-    // mCurrentToplevelSecurityInfo, don't restore after top level
-    // document load finishes.
-    mRestoreSubrequests = false;
-  }
+  // The subrequest counters are now in sync with mCurrentToplevelSecurityInfo,
+  // don't restore after top level document load finishes.
+  mRestoreSubrequests = false;
 
   UpdateSecurityState(aRequest, withNewLocation, withNewSink || updateStatus);
 }
 
 void
 nsSecureBrowserUIImpl::UpdateSubrequestMembers(nsISupports* securityInfo,
                                                nsIRequest* request)
 {
   // For wyciwyg channels in subdocuments we only update our
   // subrequest state members.
   uint32_t reqState = GetSecurityStateFromSecurityInfoAndRequest(securityInfo,
                                                                  request);
 
-  // the code above this line should run without a lock
-  ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-
   if (reqState & STATE_IS_SECURE) {
     // do nothing
   } else if (reqState & STATE_IS_BROKEN) {
     MOZ_LOG(gSecureDocLog, LogLevel::Debug,
            ("SecureUI:%p: OnStateChange: subreq BROKEN\n", this));
     ++mSubRequestsBrokenSecurity;
   } else {
     MOZ_LOG(gSecureDocLog, LogLevel::Debug,
@@ -586,30 +578,26 @@ nsSecureBrowserUIImpl::OnStateChange(nsI
   nsCOMPtr<nsIDOMWindow> windowForProgress;
   aWebProgress->GetDOMWindow(getter_AddRefs(windowForProgress));
 
   nsCOMPtr<nsIDOMWindow> window;
   bool isViewSource;
 
   nsCOMPtr<nsINetUtil> ioService;
 
-  {
-    ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-    window = do_QueryReferent(mWindow);
-    NS_ASSERTION(window, "Window has gone away?!");
-    isViewSource = mIsViewSource;
-    ioService = mIOService;
-  }
+  window = do_QueryReferent(mWindow);
+  NS_ASSERTION(window, "Window has gone away?!");
+  isViewSource = mIsViewSource;
+  ioService = mIOService;
 
   if (!ioService)
   {
     ioService = do_GetService(NS_IOSERVICE_CONTRACTID);
     if (ioService)
     {
-      ReentrantMonitorAutoEnter lock(mReentrantMonitor);
       mIOService = ioService;
     }
   }
 
   bool isNoContentResponse = false;
   nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(aRequest);
   if (httpChannel) 
   {
@@ -860,36 +848,31 @@ nsSecureBrowserUIImpl::OnStateChange(nsI
 #endif
 
   if (aProgressStateFlags & STATE_TRANSFERRING
       &&
       aProgressStateFlags & STATE_IS_REQUEST)
   {
     // The listing of a request in mTransferringRequests
     // means, there has already been data transfered.
-
-    ReentrantMonitorAutoEnter lock(mReentrantMonitor);
     mTransferringRequests.Add(aRequest, fallible);
 
     return NS_OK;
   }
 
   bool requestHasTransferedData = false;
 
   if (aProgressStateFlags & STATE_STOP
       &&
       aProgressStateFlags & STATE_IS_REQUEST)
   {
-    { /* scope for the ReentrantMonitorAutoEnter */
-      ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-      PLDHashEntryHdr* entry = mTransferringRequests.Search(aRequest);
-      if (entry) {
-        mTransferringRequests.RemoveEntry(entry);
-        requestHasTransferedData = true;
-      }
+    PLDHashEntryHdr* entry = mTransferringRequests.Search(aRequest);
+    if (entry) {
+      mTransferringRequests.RemoveEntry(entry);
+      requestHasTransferedData = true;
     }
 
     if (!requestHasTransferedData) {
       // Because image loads doesn't support any TRANSFERRING notifications but
       // only START and STOP we must ask them directly whether content was
       // transferred.  See bug 432685 for details.
       nsCOMPtr<nsISecurityInfoProvider> securityInfoProvider =
         do_QueryInterface(aRequest);
@@ -925,26 +908,22 @@ nsSecureBrowserUIImpl::OnStateChange(nsI
 
     int32_t saveSubBroken;
     int32_t saveSubNo;
     nsCOMPtr<nsIAssociatedContentSecurity> prevContentSecurity;
 
     int32_t newSubBroken = 0;
     int32_t newSubNo = 0;
 
-    {
-      ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-      inProgress = (mDocumentRequestsInProgress!=0);
+    inProgress = (mDocumentRequestsInProgress!=0);
 
-      if (allowSecurityStateChange && !inProgress)
-      {
-        saveSubBroken = mSubRequestsBrokenSecurity;
-        saveSubNo = mSubRequestsNoSecurity;
-        prevContentSecurity = do_QueryInterface(mCurrentToplevelSecurityInfo);
-      }
+    if (allowSecurityStateChange && !inProgress) {
+      saveSubBroken = mSubRequestsBrokenSecurity;
+      saveSubNo = mSubRequestsNoSecurity;
+      prevContentSecurity = do_QueryInterface(mCurrentToplevelSecurityInfo);
     }
 
     if (allowSecurityStateChange && !inProgress)
     {
       MOZ_LOG(gSecureDocLog, LogLevel::Debug,
              ("SecureUI:%p: OnStateChange: start for toplevel document\n", this
               ));
 
@@ -1002,57 +981,48 @@ nsSecureBrowserUIImpl::OnStateChange(nsI
         // it didn't get rendered.  But we reset the state to unknown and
         // mSubRequests* to zeros.  If we would have left these values after 
         // this top level load stoped, we would override the original top level
         // load with all zeros and break mixed content state on back and forward.
         mRestoreSubrequests = true;
       }
     }
 
-    {
-      ReentrantMonitorAutoEnter lock(mReentrantMonitor);
+    if (allowSecurityStateChange && !inProgress) {
+      ResetStateTracking();
+      mSubRequestsBrokenSecurity = newSubBroken;
+      mSubRequestsNoSecurity = newSubNo;
+      mNewToplevelSecurityStateKnown = false;
+    }
 
-      if (allowSecurityStateChange && !inProgress)
-      {
-        ResetStateTracking();
-        mSubRequestsBrokenSecurity = newSubBroken;
-        mSubRequestsNoSecurity = newSubNo;
-        mNewToplevelSecurityStateKnown = false;
-      }
-
-      // By using a counter, this code also works when the toplevel
-      // document get's redirected, but the STOP request for the 
-      // previous toplevel document has not yet have been received.
-      MOZ_LOG(gSecureDocLog, LogLevel::Debug,
-             ("SecureUI:%p: OnStateChange: ++mDocumentRequestsInProgress\n", this
-              ));
-      ++mDocumentRequestsInProgress;
-    }
+    // By using a counter, this code also works when the toplevel
+    // document get's redirected, but the STOP request for the 
+    // previous toplevel document has not yet have been received.
+    MOZ_LOG(gSecureDocLog, LogLevel::Debug,
+           ("SecureUI:%p: OnStateChange: ++mDocumentRequestsInProgress\n", this
+            ));
+    ++mDocumentRequestsInProgress;
 
     return NS_OK;
   }
 
   if (aProgressStateFlags & STATE_STOP
       &&
       aProgressStateFlags & STATE_IS_REQUEST
       &&
       isToplevelProgress
       &&
       loadFlags & nsIChannel::LOAD_DOCUMENT_URI)
   {
     int32_t temp_DocumentRequestsInProgress;
     nsCOMPtr<nsISecurityEventSink> temp_ToplevelEventSink;
 
-    {
-      ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-      temp_DocumentRequestsInProgress = mDocumentRequestsInProgress;
-      if (allowSecurityStateChange)
-      {
-        temp_ToplevelEventSink = mToplevelEventSink;
-      }
+    temp_DocumentRequestsInProgress = mDocumentRequestsInProgress;
+    if (allowSecurityStateChange) {
+      temp_ToplevelEventSink = mToplevelEventSink;
     }
 
     if (temp_DocumentRequestsInProgress <= 0)
     {
       // Ignore stop requests unless a document load is in progress
       // Unfortunately on application start, see some stops without having seen any starts...
       return NS_OK;
     }
@@ -1066,26 +1036,22 @@ nsSecureBrowserUIImpl::OnStateChange(nsI
       if (allowSecurityStateChange)
       {
         ObtainEventSink(channel, temp_ToplevelEventSink);
       }
     }
 
     bool sinkChanged = false;
     bool inProgress;
-    {
-      ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-      if (allowSecurityStateChange)
-      {
-        sinkChanged = (mToplevelEventSink != temp_ToplevelEventSink);
-        mToplevelEventSink = temp_ToplevelEventSink;
-      }
-      --mDocumentRequestsInProgress;
-      inProgress = mDocumentRequestsInProgress > 0;
+    if (allowSecurityStateChange) {
+      sinkChanged = (mToplevelEventSink != temp_ToplevelEventSink);
+      mToplevelEventSink = temp_ToplevelEventSink;
     }
+    --mDocumentRequestsInProgress;
+    inProgress = mDocumentRequestsInProgress > 0;
 
     if (allowSecurityStateChange && requestHasTransferedData) {
       // Data has been transferred for the single toplevel
       // request. Evaluate the security state.
 
       // Do this only when the sink has changed.  We update and notify
       // the state from OnLacationChange, this is actually redundant.
       // But when the target sink changes between OnLocationChange and
@@ -1103,49 +1069,42 @@ nsSecureBrowserUIImpl::OnStateChange(nsI
     {
       // We get here when there were no OnLocationChange between 
       // OnStateChange(START) and OnStateChange(STOP).  Then the load has not
       // been rendered but has been retargeted in some other way then by external
       // app handler.  Restore mSubRequests* members to what the current security 
       // state info holds (it was reset to all zero in OnStateChange(START) 
       // before).
       nsCOMPtr<nsIAssociatedContentSecurity> currentContentSecurity;
-      {
-        ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-        currentContentSecurity = do_QueryInterface(mCurrentToplevelSecurityInfo);
+      currentContentSecurity = do_QueryInterface(mCurrentToplevelSecurityInfo);
 
-        // Drop this indication flag, the restore opration is just being
-        // done.
-        mRestoreSubrequests = false;
+      // Drop this indication flag, the restore operation is just being done.
+      mRestoreSubrequests = false;
 
-        // We can do this since the state didn't actually change.
-        mNewToplevelSecurityStateKnown = true;
-      }
+      // We can do this since the state didn't actually change.
+      mNewToplevelSecurityStateKnown = true;
 
       int32_t subBroken = 0;
       int32_t subNo = 0;
 
       if (currentContentSecurity)
       {
         currentContentSecurity->GetCountSubRequestsBrokenSecurity(&subBroken);
         currentContentSecurity->GetCountSubRequestsNoSecurity(&subNo);
         MOZ_LOG(gSecureDocLog, LogLevel::Debug, ("SecureUI:%p: Restoring subs in STOP from %p to %d,%d\n", 
           this, currentContentSecurity.get(), subBroken, subNo));      
       }
 
-      {
-        ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-        mSubRequestsBrokenSecurity = subBroken;
-        mSubRequestsNoSecurity = subNo;
-      }
+      mSubRequestsBrokenSecurity = subBroken;
+      mSubRequestsNoSecurity = subNo;
     }
-    
+
     return NS_OK;
   }
-  
+
   if (aProgressStateFlags & STATE_STOP
       &&
       aProgressStateFlags & STATE_IS_REQUEST)
   {
     if (!isSubDocumentRelevant)
       return NS_OK;
     
     // if we arrive here, LOAD_DOCUMENT_URI is not set
@@ -1162,20 +1121,17 @@ nsSecureBrowserUIImpl::OnStateChange(nsI
       // 
       // At this point, we are learning about the security state of a sub-document.
       // We must not update the security state based on the sub content,
       // if the new top level state is not yet known.
       //
       // We skip updating the security state in this case.
 
       bool temp_NewToplevelSecurityStateKnown;
-      {
-        ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-        temp_NewToplevelSecurityStateKnown = mNewToplevelSecurityStateKnown;
-      }
+      temp_NewToplevelSecurityStateKnown = mNewToplevelSecurityStateKnown;
 
       if (temp_NewToplevelSecurityStateKnown) {
         UpdateSecurityState(aRequest, false, false);
       }
     }
 
     return NS_OK;
   }
@@ -1237,21 +1193,18 @@ nsSecureBrowserUIImpl::UpdateSecuritySta
   }
 }
 
 void
 nsSecureBrowserUIImpl::TellTheWorld(nsIRequest* aRequest)
 {
   nsCOMPtr<nsISecurityEventSink> toplevelEventSink;
   uint32_t state = STATE_IS_INSECURE;
-  {
-    ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-    toplevelEventSink = mToplevelEventSink;
-    GetState(&state);
-  }
+  toplevelEventSink = mToplevelEventSink;
+  GetState(&state);
 
   if (toplevelEventSink) {
     MOZ_LOG(gSecureDocLog, LogLevel::Debug,
            ("SecureUI:%p: UpdateSecurityState: calling OnSecurityChange\n",
             this));
 
     toplevelEventSink->OnSecurityChange(aRequest, state);
   } else {
@@ -1292,25 +1245,22 @@ nsSecureBrowserUIImpl::OnLocationChange(
       MOZ_LOG(gSecureDocLog, LogLevel::Debug,
              ("SecureUI:%p: OnLocationChange: view-source\n", this));
     }
 
     updateIsViewSource = true;
     temp_IsViewSource = vs;
   }
 
-  {
-    ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-    if (updateIsViewSource) {
-      mIsViewSource = temp_IsViewSource;
-    }
-    mCurrentURI = aLocation;
-    window = do_QueryReferent(mWindow);
-    NS_ASSERTION(window, "Window has gone away?!");
+  if (updateIsViewSource) {
+    mIsViewSource = temp_IsViewSource;
   }
+  mCurrentURI = aLocation;
+  window = do_QueryReferent(mWindow);
+  NS_ASSERTION(window, "Window has gone away?!");
 
   // When |aRequest| is null, basically we don't trust that document. But if
   // docshell insists that the document has not changed at all, we will reuse
   // the previous security state, no matter what |aRequest| may be.
   if (aFlags & LOCATION_CHANGE_SAME_DOCUMENT)
     return NS_OK;
 
   // The location bar has changed, so we must update the security state.  The
@@ -1343,20 +1293,17 @@ nsSecureBrowserUIImpl::OnLocationChange(
   // 
   // At this point, we are learning about the security state of a sub-document.
   // We must not update the security state based on the sub content, if the new
   // top level state is not yet known.
   //
   // We skip updating the security state in this case.
 
   bool temp_NewToplevelSecurityStateKnown;
-  {
-    ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-    temp_NewToplevelSecurityStateKnown = mNewToplevelSecurityStateKnown;
-  }
+  temp_NewToplevelSecurityStateKnown = mNewToplevelSecurityStateKnown;
 
   if (temp_NewToplevelSecurityStateKnown) {
     UpdateSecurityState(aRequest, true, false);
   }
 
   return NS_OK;
 }
 
@@ -1398,29 +1345,27 @@ nsSecureBrowserUIImpl::OnSecurityChange(
 
 // nsISSLStatusProvider methods
 NS_IMETHODIMP
 nsSecureBrowserUIImpl::GetSSLStatus(nsISSLStatus** _result)
 {
   NS_ENSURE_ARG_POINTER(_result);
   MOZ_ASSERT(NS_IsMainThread());
 
-  ReentrantMonitorAutoEnter lock(mReentrantMonitor);
-
   switch (mNotifiedSecurityState)
   {
     case lis_broken_security:
     case lis_mixed_security:
     case lis_high_security:
       break;
 
     default:
       NS_NOTREACHED("if this is reached you must add more entries to the switch");
     case lis_no_security:
       *_result = nullptr;
       return NS_OK;
   }
- 
+
   *_result = mSSLStatus;
   NS_IF_ADDREF(*_result);
 
   return NS_OK;
 }
--- a/security/manager/ssl/nsSecureBrowserUIImpl.h
+++ b/security/manager/ssl/nsSecureBrowserUIImpl.h
@@ -34,35 +34,33 @@ class nsIChannel;
 
 
 class nsSecureBrowserUIImpl : public nsISecureBrowserUI,
                               public nsIWebProgressListener,
                               public nsSupportsWeakReference,
                               public nsISSLStatusProvider
 {
 public:
-  
+
   nsSecureBrowserUIImpl();
-  
+
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIWEBPROGRESSLISTENER
   NS_DECL_NSISECUREBROWSERUI
   NS_DECL_NSISSLSTATUSPROVIDER
 
 protected:
   virtual ~nsSecureBrowserUIImpl() {};
 
-  mozilla::ReentrantMonitor mReentrantMonitor;
-  
   nsWeakPtr mWindow;
   nsWeakPtr mDocShell;
   nsCOMPtr<nsINetUtil> mIOService;
   nsCOMPtr<nsIURI> mCurrentURI;
   nsCOMPtr<nsISecurityEventSink> mToplevelEventSink;
-  
+
   enum lockIconState {
     lis_no_security,
     lis_broken_security,
     lis_mixed_security,
     lis_high_security
   };
 
   lockIconState mNotifiedSecurityState;
@@ -75,17 +73,16 @@ protected:
   bool mIsViewSource;
 
   int32_t mDocumentRequestsInProgress;
   int32_t mSubRequestsBrokenSecurity;
   int32_t mSubRequestsNoSecurity;
   bool mRestoreSubrequests;
   bool mOnLocationChangeSeen;
 #ifdef DEBUG
-  /* related to mReentrantMonitor */
   mozilla::Atomic<int32_t> mOnStateLocationChangeReentranceDetection;
 #endif
 
   static already_AddRefed<nsISupports> ExtractSecurityInfo(nsIRequest* aRequest);
   nsresult MapInternalToExternalState(uint32_t* aState, lockIconState lock, bool ev);
   void UpdateSecurityState(nsIRequest* aRequest, bool withNewLocation,
                            bool withUpdateStatus);
   void TellTheWorld(nsIRequest* aRequest);