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

@@ -290,12 +290,6 @@ imgRequest::RemoveProxy(imgRequestProxy* proxy, nsresult aStatus)
     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;
 }
 

image/imgRequestProxy.cpp

@@ -115,6 +115,7 @@ imgRequestProxy::imgRequestProxy() :
   mAnimationConsumers(0),
   mCanceled(false),
   mIsInLoadGroup(false),
+  mForceDispatchLoadGroup(false),
   mListenerIsStrongRef(false),
   mDecodeRequested(false),
   mDeferNotifications(false),
@@ -170,6 +171,7 @@ imgRequestProxy::~imgRequestProxy()
     GetOwner()->RemoveProxy(this, NS_OK);
   }
 
+  RemoveFromLoadGroup();
   LOG_FUNC(gImgLog, "imgRequestProxy::~imgRequestProxy");
 }
 
@@ -230,7 +232,7 @@ imgRequestProxy::ChangeOwner(imgRequest* aNewOwner)
   uint32_t oldAnimationConsumers = mAnimationConsumers;
   ClearAnimationConsumers();
 
-  GetOwner()->RemoveProxy(this, NS_IMAGELIB_CHANGING_OWNER);
+  GetOwner()->RemoveProxy(this, NS_OK);
 
   mBehaviour->SetOwner(aNewOwner);
 
@@ -358,36 +360,93 @@ 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 **/
 
@@ -437,6 +496,7 @@ imgRequestProxy::DoCancel(nsresult status)
     GetOwner()->RemoveProxy(this, status);
   }
 
+  RemoveFromLoadGroup();
   NullOutListener();
 }
 
@@ -457,22 +517,13 @@ imgRequestProxy::CancelAndForgetObserver(nsresult aStatus)
 
   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();
 
@@ -1004,12 +1055,12 @@ imgRequestProxy::OnLoadComplete(bool aLastPart)
   // 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();
     }
   }
 

image/imgRequestProxy.h

@@ -83,8 +83,9 @@ public:
                                                // 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;
@@ -168,16 +169,18 @@ protected:
       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
@@ -236,6 +239,7 @@ private:
   uint32_t    mAnimationConsumers;
   bool mCanceled : 1;
   bool mIsInLoadGroup : 1;
+  bool mForceDispatchLoadGroup : 1;
   bool mListenerIsStrongRef : 1;
   bool mDecodeRequested : 1;
 

xpcom/base/ErrorList.py

@@ -699,7 +699,6 @@ with modules["DOM"]:
 # =======================================================================
 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)