Bug 1404422 - Part 1c. Refactor how an imgRequestProxy is added/removed from its load group. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Wed, 01 Nov 2017 06:59:10 -0400
changeset 442878 1b420c5a7b11eaf2fa1849f90769659f5146a8e9
parent 442877 a943db5eaa646573e38426a18d059ffd956bd3e7
child 442879 fe1b72af47407a2d5cbb2198d55521738d43994d
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1404422
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1404422 - Part 1c. Refactor how an imgRequestProxy is added/removed from its load group. r=tnikkel There should be no functional change here, but we rely upon the new structure in the next patch in the series. This separates out the notions of removing a request from the load group (which is always final, and must be executed outside of synchronous calls from the owner of the imgRequestProxy) and wanting to readd a request to the load group as a background request (for multipart images). The most important addition is mForceDispatchLoadGroup which if true when imgRequestProxy::RemoveFromLoadGroup is called, will dispatch the removal from the load group instead of executing it inline. This ensures safety for any callers (e.g. to CancelAndForgetObserver) as above.
image/imgRequest.cpp
image/imgRequestProxy.cpp
image/imgRequestProxy.h
xpcom/base/ErrorList.py
--- a/image/imgRequest.cpp
+++ b/image/imgRequest.cpp
@@ -285,22 +285,16 @@ imgRequest::RemoveProxy(imgRequestProxy*
 
       this->Cancel(NS_BINDING_ABORTED);
     }
 
     /* break the cycle from the cache entry. */
     mCacheEntry = nullptr;
   }
 
-  // If a proxy is removed for a reason other than its owner being
-  // changed, remove the proxy from the loadgroup.
-  if (aStatus != NS_IMAGELIB_CHANGING_OWNER) {
-    proxy->RemoveFromLoadGroup(true);
-  }
-
   return NS_OK;
 }
 
 void
 imgRequest::CancelAndAbort(nsresult aStatus)
 {
   LOG_SCOPE(gImgLog, "imgRequest::CancelAndAbort");
 
--- a/image/imgRequestProxy.cpp
+++ b/image/imgRequestProxy.cpp
@@ -110,16 +110,17 @@ imgRequestProxy::imgRequestProxy() :
   mBehaviour(new RequestBehaviour),
   mURI(nullptr),
   mListener(nullptr),
   mLoadFlags(nsIRequest::LOAD_NORMAL),
   mLockCount(0),
   mAnimationConsumers(0),
   mCanceled(false),
   mIsInLoadGroup(false),
+  mForceDispatchLoadGroup(false),
   mListenerIsStrongRef(false),
   mDecodeRequested(false),
   mDeferNotifications(false),
   mHadListener(false),
   mHadDispatch(false)
 {
   /* member initializers and constructor code */
   LOG_FUNC(gImgLog, "imgRequestProxy::imgRequestProxy");
@@ -165,16 +166,17 @@ imgRequestProxy::~imgRequestProxy()
        channel, if still downloading data, from being canceled if 'this' is
        the last observer.  This allows the image to continue to download and
        be cached even if no one is using it currently.
     */
     mCanceled = true;
     GetOwner()->RemoveProxy(this, NS_OK);
   }
 
+  RemoveFromLoadGroup();
   LOG_FUNC(gImgLog, "imgRequestProxy::~imgRequestProxy");
 }
 
 nsresult
 imgRequestProxy::Init(imgRequest* aOwner,
                       nsILoadGroup* aLoadGroup,
                       nsIDocument* aLoadingDocument,
                       ImageURL* aURI,
@@ -225,17 +227,17 @@ imgRequestProxy::ChangeOwner(imgRequest*
   while (mLockCount) {
     UnlockImage();
   }
 
   // If we're holding animation requests, undo them.
   uint32_t oldAnimationConsumers = mAnimationConsumers;
   ClearAnimationConsumers();
 
-  GetOwner()->RemoveProxy(this, NS_IMAGELIB_CHANGING_OWNER);
+  GetOwner()->RemoveProxy(this, NS_OK);
 
   mBehaviour->SetOwner(aNewOwner);
 
   // If we were locked, apply the locks here
   for (uint32_t i = 0; i < oldLockCount; i++) {
     LockImage();
   }
 
@@ -353,46 +355,103 @@ imgRequestProxy::AddToOwner(nsIDocument*
 
   GetOwner()->AddProxy(this);
 }
 
 void
 imgRequestProxy::AddToLoadGroup()
 {
   NS_ASSERTION(!mIsInLoadGroup, "Whaa, we're already in the loadgroup!");
+  MOZ_ASSERT(!mForceDispatchLoadGroup);
 
+  /* While in theory there could be a dispatch outstanding to remove this
+     request from the load group, in practice we only add to the load group
+     (when previously not in a load group) at initialization. */
   if (!mIsInLoadGroup && mLoadGroup) {
+    LOG_FUNC(gImgLog, "imgRequestProxy::AddToLoadGroup");
     mLoadGroup->AddRequest(this, nullptr);
     mIsInLoadGroup = true;
   }
 }
 
 void
-imgRequestProxy::RemoveFromLoadGroup(bool releaseLoadGroup)
+imgRequestProxy::RemoveFromLoadGroup()
 {
-  if (!mIsInLoadGroup) {
+  if (!mIsInLoadGroup || !mLoadGroup) {
     return;
   }
 
+  /* Sometimes we may not be able to remove ourselves from the load group in
+     the current context. This is because our listeners are not re-entrant (e.g.
+     we are in the middle of CancelAndForgetObserver or SyncClone). */
+  if (mForceDispatchLoadGroup) {
+    LOG_FUNC(gImgLog, "imgRequestProxy::RemoveFromLoadGroup -- dispatch");
+
+    /* We take away the load group from the request temporarily; this prevents
+       additional dispatches via RemoveFromLoadGroup occurring, as well as
+       MoveToBackgroundInLoadGroup from removing and readding. This is safe
+       because we know that once we get here, blocking the load group at all is
+       unnecessary. */
+    mIsInLoadGroup = false;
+    nsCOMPtr<nsILoadGroup> loadGroup = Move(mLoadGroup);
+    RefPtr<imgRequestProxy> self(this);
+    DispatchWithTargetIfAvailable(NS_NewRunnableFunction(
+      "imgRequestProxy::RemoveFromLoadGroup",
+      [self, loadGroup]() -> void {
+        loadGroup->RemoveRequest(self, nullptr, NS_OK);
+      }));
+    return;
+  }
+
+  LOG_FUNC(gImgLog, "imgRequestProxy::RemoveFromLoadGroup");
+
   /* calling RemoveFromLoadGroup may cause the document to finish
      loading, which could result in our death.  We need to make sure
      that we stay alive long enough to fight another battle... at
-     least until we exit this function.
-  */
+     least until we exit this function. */
   nsCOMPtr<imgIRequest> kungFuDeathGrip(this);
-
   mLoadGroup->RemoveRequest(this, nullptr, NS_OK);
+  mLoadGroup = nullptr;
   mIsInLoadGroup = false;
-
-  if (releaseLoadGroup) {
-    // We're done with the loadgroup, release it.
-    mLoadGroup = nullptr;
-  }
 }
 
+void
+imgRequestProxy::MoveToBackgroundInLoadGroup()
+{
+  /* Even if we are still in the load group, we may have taken away the load
+     group reference itself because we are in the process of leaving the group.
+     In that case, there is no need to background the request. */
+  if (!mLoadGroup) {
+    return;
+  }
+
+  /* There is no need to dispatch if we only need to add ourselves to the load
+     group without removal. It is the removal which causes the problematic
+     callbacks (see RemoveFromLoadGroup). */
+  if (mIsInLoadGroup && mForceDispatchLoadGroup) {
+    LOG_FUNC(gImgLog, "imgRequestProxy::MoveToBackgroundInLoadGroup -- dispatch");
+
+    RefPtr<imgRequestProxy> self(this);
+    DispatchWithTargetIfAvailable(NS_NewRunnableFunction(
+      "imgRequestProxy::MoveToBackgroundInLoadGroup",
+      [self]() -> void {
+        self->MoveToBackgroundInLoadGroup();
+      }));
+    return;
+  }
+
+  LOG_FUNC(gImgLog, "imgRequestProxy::MoveToBackgroundInLoadGroup");
+  nsCOMPtr<imgIRequest> kungFuDeathGrip(this);
+  if (mIsInLoadGroup) {
+    mLoadGroup->RemoveRequest(this, nullptr, NS_OK);
+  }
+
+  mLoadFlags |= nsIRequest::LOAD_BACKGROUND;
+  mLoadGroup->AddRequest(this, nullptr);
+}
 
 /**  nsIRequest / imgIRequest methods **/
 
 NS_IMETHODIMP
 imgRequestProxy::GetName(nsACString& aName)
 {
   aName.Truncate();
 
@@ -432,16 +491,17 @@ imgRequestProxy::Cancel(nsresult status)
 
 void
 imgRequestProxy::DoCancel(nsresult status)
 {
   if (GetOwner()) {
     GetOwner()->RemoveProxy(this, status);
   }
 
+  RemoveFromLoadGroup();
   NullOutListener();
 }
 
 NS_IMETHODIMP
 imgRequestProxy::CancelAndForgetObserver(nsresult aStatus)
 {
   // If mCanceled is true but mListener is non-null, that means
   // someone called Cancel() on us but the imgCancelRunnable is still
@@ -452,32 +512,23 @@ imgRequestProxy::CancelAndForgetObserver
   if (mCanceled && !mListener) {
     return NS_ERROR_FAILURE;
   }
 
   LOG_SCOPE(gImgLog, "imgRequestProxy::CancelAndForgetObserver");
 
   mCanceled = true;
 
-  // Now cheat and make sure our removal from loadgroup happens async
-  bool oldIsInLoadGroup = mIsInLoadGroup;
-  mIsInLoadGroup = false;
-
   if (GetOwner()) {
     GetOwner()->RemoveProxy(this, aStatus);
   }
 
-  mIsInLoadGroup = oldIsInLoadGroup;
-
-  if (mIsInLoadGroup) {
-    nsCOMPtr<nsIRunnable> ev =
-      NewRunnableMethod("imgRequestProxy::DoRemoveFromLoadGroup",
-                        this, &imgRequestProxy::DoRemoveFromLoadGroup);
-    DispatchWithTargetIfAvailable(ev.forget());
-  }
+  mForceDispatchLoadGroup = true;
+  RemoveFromLoadGroup();
+  mForceDispatchLoadGroup = false;
 
   NullOutListener();
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 imgRequestProxy::StartDecoding(uint32_t aFlags)
@@ -999,22 +1050,22 @@ imgRequestProxy::OnLoadComplete(bool aLa
     listener->Notify(this, imgINotificationObserver::LOAD_COMPLETE, nullptr);
   }
 
   // If we're expecting more data from a multipart channel, re-add ourself
   // to the loadgroup so that the document doesn't lose track of the load.
   // If the request is already a background request and there's more data
   // coming, we can just leave the request in the loadgroup as-is.
   if (aLastPart || (mLoadFlags & nsIRequest::LOAD_BACKGROUND) == 0) {
-    RemoveFromLoadGroup(aLastPart);
-    // More data is coming, so change the request to be a background request
-    // and put it back in the loadgroup.
-    if (!aLastPart) {
-      mLoadFlags |= nsIRequest::LOAD_BACKGROUND;
-      AddToLoadGroup();
+    if (aLastPart) {
+      RemoveFromLoadGroup();
+    } else {
+      // More data is coming, so change the request to be a background request
+      // and put it back in the loadgroup.
+      MoveToBackgroundInLoadGroup();
     }
   }
 
   if (mListenerIsStrongRef && aLastPart) {
     NS_PRECONDITION(mListener, "How did that happen?");
     // Drop our strong ref to the listener now that we're done with
     // everything.  Note that this can cancel us and other fun things
     // like that.  Don't add anything in this method after this point.
--- a/image/imgRequestProxy.h
+++ b/image/imgRequestProxy.h
@@ -78,18 +78,19 @@ public:
                 ImageURL* aURI,
                 imgINotificationObserver* aObserver);
 
   nsresult ChangeOwner(imgRequest* aNewOwner); // this will change mOwner.
                                                // Do not call this if the
                                                // previous owner has already
                                                // sent notifications out!
 
+  // Add the request to the load group, if any. This should only be called once
+  // during initialization.
   void AddToLoadGroup();
-  void RemoveFromLoadGroup(bool releaseLoadGroup);
 
   inline bool HasObserver() const {
     return mListener != nullptr;
   }
 
   // Asynchronously notify this proxy's listener of the current state of the
   // image, and, if we have an imgRequest mOwner, any status changes that
   // happen between the time this function is called and the time the
@@ -163,26 +164,28 @@ protected:
         return NS_OK;
       }
 
     private:
       RefPtr<imgRequestProxy> mOwner;
       nsresult mStatus;
   };
 
+  /* Remove from and forget the load group. */
+  void RemoveFromLoadGroup();
+
+  /* Remove from the load group and readd as a background request. */
+  void MoveToBackgroundInLoadGroup();
+
   /* Finish up canceling ourselves */
   void DoCancel(nsresult status);
 
   /* Do the proper refcount management to null out mListener */
   void NullOutListener();
 
-  void DoRemoveFromLoadGroup() {
-    RemoveFromLoadGroup(true);
-  }
-
   // Return the ProgressTracker associated with mOwner and/or mImage. It may
   // live either on mOwner or mImage, depending on whether
   //   (a) we have an mOwner at all
   //   (b) whether mOwner has instantiated its image yet
   already_AddRefed<ProgressTracker> GetProgressTracker() const;
 
   nsITimedChannel* TimedChannel()
   {
@@ -231,16 +234,17 @@ private:
   RefPtr<mozilla::dom::TabGroup> mTabGroup;
   nsCOMPtr<nsIEventTarget> mEventTarget;
 
   nsLoadFlags mLoadFlags;
   uint32_t    mLockCount;
   uint32_t    mAnimationConsumers;
   bool mCanceled : 1;
   bool mIsInLoadGroup : 1;
+  bool mForceDispatchLoadGroup : 1;
   bool mListenerIsStrongRef : 1;
   bool mDecodeRequested : 1;
 
   // Whether we want to defer our notifications by the non-virtual Observer
   // interfaces as image loads proceed.
   bool mDeferNotifications : 1;
   bool mHadListener : 1;
   bool mHadDispatch : 1;
--- a/xpcom/base/ErrorList.py
+++ b/xpcom/base/ErrorList.py
@@ -694,17 +694,16 @@ with modules["DOM"]:
 
 
 
 # =======================================================================
 # 15: NS_ERROR_MODULE_IMGLIB
 # =======================================================================
 with modules["IMGLIB"]:
     errors["NS_IMAGELIB_SUCCESS_LOAD_FINISHED"] = SUCCESS(0)
-    errors["NS_IMAGELIB_CHANGING_OWNER"] = SUCCESS(1)
 
     errors["NS_IMAGELIB_ERROR_FAILURE"] = FAILURE(5)
     errors["NS_IMAGELIB_ERROR_NO_DECODER"] = FAILURE(6)
     errors["NS_IMAGELIB_ERROR_NOT_FINISHED"] = FAILURE(7)
     errors["NS_IMAGELIB_ERROR_NO_ENCODER"] = FAILURE(9)