Bug 1404422 - Part 1d. Ensure imgRequestProxy::PerformClone consisten…

…tly adds the clone to the expected load group. r=tnikkel

Historically imgRequestProxy::PerformClone would only add the cloned
request to the (original proxy's) document's load group if the request
was still being validated. Now it adds the cloned request to the given
document's load group before requesting the notifications, unless the
request has already been completed. We ensure that any removals from
the load group occur outside the current execution context.

Legacy listeners may use imgRequestProxy::SyncClone to request
notifications on the image state. Ideally they would not, but they do
not work as expected with the asynchronous notifications all new callers
must use. While in theory this would suggest their code is re-entrant,
not all of it is. In particular we need to be sensitive about when we
remove a request from a load group.

image/imgRequestProxy.cpp

@@ -828,14 +828,19 @@ imgRequestProxy::PerformClone(imgINotificationObserver* aObserver,
   *aClone = nullptr;
   RefPtr<imgRequestProxy> clone = NewClonedProxy();
 
+  nsCOMPtr<nsILoadGroup> loadGroup;
+  if (aLoadingDocument) {
+    loadGroup = aLoadingDocument->GetDocumentLoadGroup();
+  }
+
   // It is important to call |SetLoadFlags()| before calling |Init()| because
   // |Init()| adds the request to the loadgroup.
   // When a request is added to a loadgroup, its load flags are merged
   // with the load flags of the loadgroup.
   // XXXldb That's not true anymore.  Stuff from imgLoader adds the
   // request to the loadgroup.
   clone->SetLoadFlags(mLoadFlags);
-  nsresult rv = clone->Init(mBehaviour->GetOwner(), mLoadGroup,
+  nsresult rv = clone->Init(mBehaviour->GetOwner(), loadGroup,
                             aLoadingDocument, mURI, aObserver);
   if (NS_FAILED(rv)) {
     return rv;
@@ -848,20 +853,44 @@ imgRequestProxy::PerformClone(imgINotificationObserver* aObserver,
 
   if (GetOwner() && GetOwner()->GetValidator()) {
     // Note that if we have a validator, we don't want to issue notifications at
-    // here because we want to defer until that completes.
+    // here because we want to defer until that completes. AddProxy will add us
+    // to the load group; we cannot avoid that in this case, because we don't
+    // know when the validation will complete, and if it will cause us to
+    // discard our cached state anyways. We are probably already blocked by the
+    // original LoadImage(WithChannel) request in any event.
     clone->SetNotificationsDeferred(true);
     GetOwner()->GetValidator()->AddProxy(clone);
-  } else if (aSyncNotify) {
-    // This is wrong!!! We need to notify asynchronously, but there's code that
-    // assumes that we don't. This will be fixed in bug 580466. Note that if we
-    // have a validator, we won't issue notifications anyways because they are
-    // deferred, so there is no point in requesting.
-    clone->SyncNotifyListener();
   } else {
-    // Without a validator, we can request asynchronous notifications
-    // immediately. If there was a validator, this would override the deferral
-    // and that would be incorrect.
-    clone->NotifyListener();
+    // We only want to add the request to the load group of the owning document
+    // if it is still in progress. Some callers cannot handle a supurious load
+    // group removal (e.g. print preview) so we must be careful. On the other
+    // hand, if after cloning, the original request proxy is cancelled /
+    // destroyed, we need to ensure that any clones still block the load group
+    // if it is incomplete.
+    bool addToLoadGroup = mIsInLoadGroup;
+    if (!addToLoadGroup) {
+      RefPtr<ProgressTracker> tracker = clone->GetProgressTracker();
+      addToLoadGroup = tracker && !(tracker->GetProgress() & FLAG_LOAD_COMPLETE);
+    }
+
+    if (addToLoadGroup) {
+      clone->AddToLoadGroup();
+    }
+
+    if (aSyncNotify) {
+      // This is wrong!!! We need to notify asynchronously, but there's code
+      // that assumes that we don't. This will be fixed in bug 580466. Note that
+      // if we have a validator, we won't issue notifications anyways because
+      // they are deferred, so there is no point in requesting.
+      clone->mForceDispatchLoadGroup = true;
+      clone->SyncNotifyListener();
+      clone->mForceDispatchLoadGroup = false;
+    } else {
+      // Without a validator, we can request asynchronous notifications
+      // immediately. If there was a validator, this would override the deferral
+      // and that would be incorrect.
+      clone->NotifyListener();
+    }
   }
 
   return NS_OK;