Bug 1383682 - Part 1. Split off imgRequestProxy notification deferrals for validation. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Wed, 07 Feb 2018 07:27:27 -0500
changeset 402698 c657d87a001166bf42e47580c37919b8b018238d
parent 402697 b8a6c14ad2804b410fe11a341360f13652649629
child 402699 62497967f092c6c76778ddc1534f110d4583eeb9
push id99627
push useraosmond@gmail.com
push dateWed, 07 Feb 2018 12:27:42 +0000
treeherdermozilla-inbound@7d098669f8cb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1383682
milestone60.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 1383682 - Part 1. Split off imgRequestProxy notification deferrals for validation. r=tnikkel When cache validation is in progress, imgRequestProxy defers its notifications to its listener until the validation is complete. This is because the cache may be discarded, and the current state will change. It attempted to share the same flags with notification deferrals used by ProgressTracker to indicate that there is a pending notification, but this has problematic/confusing. Hence this patch creates dedicated flags for notification deferrals due to cache validation.
image/ProgressTracker.cpp
image/imgLoader.cpp
image/imgLoader.h
image/imgRequestProxy.cpp
image/imgRequestProxy.h
--- a/image/ProgressTracker.cpp
+++ b/image/ProgressTracker.cpp
@@ -166,16 +166,21 @@ class AsyncNotifyRunnable : public Runna
     nsTArray<RefPtr<IProgressObserver>> mObservers;
 };
 
 void
 ProgressTracker::Notify(IProgressObserver* aObserver)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
+  if (aObserver->NotificationsDeferred()) {
+    // There is a pending notification, or the observer isn't ready yet.
+    return;
+  }
+
   if (MOZ_LOG_TEST(gImgLog, LogLevel::Debug)) {
     RefPtr<Image> image = GetImage();
     if (image && image->GetURI()) {
       RefPtr<ImageURL> uri(image->GetURI());
       nsAutoCString spec;
       uri->GetSpec(spec);
       LOG_FUNC_WITH_PARAM(gImgLog,
                           "ProgressTracker::Notify async", "uri", spec.get());
@@ -236,16 +241,21 @@ class AsyncNotifyCurrentStateRunnable : 
     RefPtr<Image> mImage;
 };
 
 void
 ProgressTracker::NotifyCurrentState(IProgressObserver* aObserver)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
+  if (aObserver->NotificationsDeferred()) {
+    // There is a pending notification, or the observer isn't ready yet.
+    return;
+  }
+
   if (MOZ_LOG_TEST(gImgLog, LogLevel::Debug)) {
     RefPtr<Image> image = GetImage();
     nsAutoCString spec;
     if (image && image->GetURI()) {
       image->GetURI()->GetSpec(spec);
     }
     LOG_FUNC_WITH_PARAM(gImgLog,
                         "ProgressTracker::NotifyCurrentState", "uri", spec.get());
--- a/image/imgLoader.cpp
+++ b/image/imgLoader.cpp
@@ -1763,17 +1763,17 @@ imgLoader::ValidateRequestWithNewChannel
 
     if (*aProxyRequest) {
       imgRequestProxy* proxy = static_cast<imgRequestProxy*>(*aProxyRequest);
 
       // We will send notifications from imgCacheValidator::OnStartRequest().
       // In the mean time, we must defer notifications because we are added to
       // the imgRequest's proxy list, and we can get extra notifications
       // resulting from methods such as StartDecoding(). See bug 579122.
-      proxy->SetNotificationsDeferred(true);
+      proxy->MarkValidating();
 
       // Attach the proxy without notifying
       request->GetValidator()->AddProxy(proxy);
     }
 
     return NS_SUCCEEDED(rv);
 
   }
@@ -1829,17 +1829,17 @@ imgLoader::ValidateRequestWithNewChannel
   newChannel->SetNotificationCallbacks(hvc);
 
   request->SetValidator(hvc);
 
   // We will send notifications from imgCacheValidator::OnStartRequest().
   // In the mean time, we must defer notifications because we are added to
   // the imgRequest's proxy list, and we can get extra notifications
   // resulting from methods such as StartDecoding(). See bug 579122.
-  req->SetNotificationsDeferred(true);
+  req->MarkValidating();
 
   // Add the proxy without notifying
   hvc->AddProxy(req);
 
   mozilla::net::PredictorLearn(aURI, aInitialDocumentURI,
                                nsINetworkPredictor::LEARN_LOAD_SUBRESOURCE, aLoadGroup);
   rv = newChannel->AsyncOpen2(listener);
   if (NS_WARN_IF(NS_FAILED(rv))) {
@@ -2931,23 +2931,55 @@ imgCacheValidator::~imgCacheValidator()
 
 void
 imgCacheValidator::AddProxy(imgRequestProxy* aProxy)
 {
   // aProxy needs to be in the loadgroup since we're validating from
   // the network.
   aProxy->AddToLoadGroup();
 
-  mProxies.AppendObject(aProxy);
+  mProxies.AppendElement(aProxy);
 }
 
 void
 imgCacheValidator::RemoveProxy(imgRequestProxy* aProxy)
 {
-  mProxies.RemoveObject(aProxy);
+  mProxies.RemoveElement(aProxy);
+}
+
+void
+imgCacheValidator::UpdateProxies()
+{
+  // We have finished validating the request, so we can safely take ownership
+  // of the proxy list. imgRequestProxy::SyncNotifyListener can mutate the list
+  // if imgRequestProxy::CancelAndForgetObserver is called by its owner. Note
+  // that any potential notifications should still be suppressed in
+  // imgRequestProxy::ChangeOwner because we haven't cleared the validating
+  // flag yet, and thus they will remain deferred.
+  AutoTArray<RefPtr<imgRequestProxy>, 4> proxies(Move(mProxies));
+
+  for (auto& proxy : proxies) {
+    // First update the state of all proxies before notifying any of them
+    // to ensure a consistent state (e.g. in case the notification causes
+    // other proxies to be touched indirectly.)
+    MOZ_ASSERT(proxy->IsValidating());
+    MOZ_ASSERT(proxy->NotificationsDeferred(),
+               "Proxies waiting on cache validation should be "
+               "deferring notifications!");
+    if (mNewRequest) {
+      proxy->ChangeOwner(mNewRequest);
+    }
+    proxy->ClearValidating();
+  }
+
+  for (auto& proxy : proxies) {
+    // Notify synchronously, because we're already in OnStartRequest, an
+    // asynchronously-called function.
+    proxy->SyncNotifyListener();
+  }
 }
 
 /** nsIRequestObserver methods **/
 
 NS_IMETHODIMP
 imgCacheValidator::OnStartRequest(nsIRequest* aRequest, nsISupports* ctxt)
 {
   // We may be holding on to a document, so ensure that it's released.
@@ -2977,43 +3009,30 @@ imgCacheValidator::OnStartRequest(nsIReq
     mRequest->GetFinalURI(getter_AddRefs(finalURI));
 
     bool sameURI = false;
     if (channelURI && finalURI) {
       channelURI->Equals(finalURI, &sameURI);
     }
 
     if (isFromCache && sameURI) {
-      uint32_t count = mProxies.Count();
-      for (int32_t i = count-1; i>=0; i--) {
-        imgRequestProxy* proxy = static_cast<imgRequestProxy*>(mProxies[i]);
-
-        // Proxies waiting on cache validation should be deferring
-        // notifications. Undefer them.
-        MOZ_ASSERT(proxy->NotificationsDeferred(),
-                   "Proxies waiting on cache validation should be "
-                   "deferring notifications!");
-        proxy->SetNotificationsDeferred(false);
-
-        // Notify synchronously, because we're already in OnStartRequest, an
-        // asynchronously-called function.
-        proxy->SyncNotifyListener();
-      }
-
       // We don't need to load this any more.
       aRequest->Cancel(NS_BINDING_ABORTED);
 
+      // Clear the validator before updating the proxies. The notifications may
+      // clone an existing request, and its state could be inconsistent.
       mRequest->SetLoadId(context);
       mRequest->SetValidator(nullptr);
 
       mRequest = nullptr;
 
       mNewRequest = nullptr;
       mNewEntry = nullptr;
 
+      UpdateProxies();
       return NS_OK;
     }
   }
 
   // We can't load out of cache. We have to create a whole new request for the
   // data that's coming in off the channel.
   nsCOMPtr<nsIURI> uri;
   {
@@ -3030,16 +3049,18 @@ imgCacheValidator::OnStartRequest(nsIReq
 
   int32_t corsmode = mRequest->GetCORSMode();
   ReferrerPolicy refpol = mRequest->GetReferrerPolicy();
   nsCOMPtr<nsIPrincipal> triggeringPrincipal = mRequest->GetTriggeringPrincipal();
 
   // Doom the old request's cache entry
   mRequest->RemoveFromCache();
 
+  // Clear the validator before updating the proxies. The notifications may
+  // clone an existing request, and its state could be inconsistent.
   mRequest->SetValidator(nullptr);
   mRequest = nullptr;
 
   // We use originalURI here to fulfil the imgIRequest contract on GetURI.
   nsCOMPtr<nsIURI> originalURI;
   channel->GetOriginalURI(getter_AddRefs(originalURI));
   nsresult rv =
     mNewRequest->Init(originalURI, uri, mHadInsecureRedirect, aRequest, channel,
@@ -3050,27 +3071,17 @@ imgCacheValidator::OnStartRequest(nsIReq
 
   mDestListener = new ProxyListener(mNewRequest);
 
   // Try to add the new request into the cache. Note that the entry must be in
   // the cache before the proxies' ownership changes, because adding a proxy
   // changes the caching behaviour for imgRequests.
   mImgLoader->PutIntoCache(mNewRequest->CacheKey(), mNewEntry);
 
-  uint32_t count = mProxies.Count();
-  for (int32_t i = count-1; i>=0; i--) {
-    imgRequestProxy* proxy = static_cast<imgRequestProxy*>(mProxies[i]);
-    proxy->ChangeOwner(mNewRequest);
-
-    // Notify synchronously, because we're already in OnStartRequest, an
-    // asynchronously-called function.
-    proxy->SetNotificationsDeferred(false);
-    proxy->SyncNotifyListener();
-  }
-
+  UpdateProxies();
   mNewRequest = nullptr;
   mNewEntry = nullptr;
 
   return mDestListener->OnStartRequest(aRequest, ctxt);
 }
 
 NS_IMETHODIMP
 imgCacheValidator::OnStopRequest(nsIRequest* aRequest,
--- a/image/imgLoader.h
+++ b/image/imgLoader.h
@@ -563,25 +563,26 @@ public:
   NS_DECL_NSITHREADRETARGETABLESTREAMLISTENER
   NS_DECL_NSISTREAMLISTENER
   NS_DECL_NSIREQUESTOBSERVER
   NS_DECL_NSICHANNELEVENTSINK
   NS_DECL_NSIINTERFACEREQUESTOR
   NS_DECL_NSIASYNCVERIFYREDIRECTCALLBACK
 
 private:
+  void UpdateProxies();
   virtual ~imgCacheValidator();
 
   nsCOMPtr<nsIStreamListener> mDestListener;
   RefPtr<nsProgressNotificationProxy> mProgressProxy;
   nsCOMPtr<nsIAsyncVerifyRedirectCallback> mRedirectCallback;
   nsCOMPtr<nsIChannel> mRedirectChannel;
 
   RefPtr<imgRequest> mRequest;
-  nsCOMArray<imgIRequest> mProxies;
+  AutoTArray<RefPtr<imgRequestProxy>, 4> mProxies;
 
   RefPtr<imgRequest> mNewRequest;
   RefPtr<imgCacheEntry> mNewEntry;
 
   nsCOMPtr<nsISupports> mContext;
 
   imgLoader* mImgLoader;
 
--- a/image/imgRequestProxy.cpp
+++ b/image/imgRequestProxy.cpp
@@ -116,16 +116,17 @@ imgRequestProxy::imgRequestProxy() :
   mLockCount(0),
   mAnimationConsumers(0),
   mCanceled(false),
   mIsInLoadGroup(false),
   mForceDispatchLoadGroup(false),
   mListenerIsStrongRef(false),
   mDecodeRequested(false),
   mDeferNotifications(false),
+  mValidating(false),
   mHadListener(false),
   mHadDispatch(false)
 {
   /* member initializers and constructor code */
   LOG_FUNC(gImgLog, "imgRequestProxy::imgRequestProxy");
 }
 
 imgRequestProxy::~imgRequestProxy()
@@ -158,25 +159,23 @@ imgRequestProxy::~imgRequestProxy()
   ClearAnimationConsumers();
 
   // Explicitly set mListener to null to ensure that the RemoveProxy
   // call below can't send |this| to an arbitrary listener while |this|
   // is being destroyed.  This is all belt-and-suspenders in view of the
   // above assert.
   NullOutListener();
 
-  if (GetOwner()) {
-    /* Call RemoveProxy with a successful status.  This will keep the
-       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);
-  }
+  /* Call RemoveProxy with a successful status.  This will keep the
+     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;
+  RemoveFromOwner(NS_OK);
 
   RemoveFromLoadGroup();
   LOG_FUNC(gImgLog, "imgRequestProxy::~imgRequestProxy");
 }
 
 nsresult
 imgRequestProxy::Init(imgRequest* aOwner,
                       nsILoadGroup* aLoadGroup,
@@ -232,38 +231,54 @@ imgRequestProxy::ChangeOwner(imgRequest*
 
   // If we're holding animation requests, undo them.
   uint32_t oldAnimationConsumers = mAnimationConsumers;
   ClearAnimationConsumers();
 
   GetOwner()->RemoveProxy(this, NS_OK);
 
   mBehaviour->SetOwner(aNewOwner);
+  MOZ_ASSERT(!GetValidator(), "New owner cannot be validating!");
 
   // If we were locked, apply the locks here
   for (uint32_t i = 0; i < oldLockCount; i++) {
     LockImage();
   }
 
   // If we had animation requests, restore them here. Note that we
   // do this *after* RemoveProxy, which clears out animation consumers
   // (see bug 601723).
   for (uint32_t i = 0; i < oldAnimationConsumers; i++) {
     IncrementAnimationConsumers();
   }
 
   AddToOwner(nullptr);
+  return NS_OK;
+}
+
+void
+imgRequestProxy::MarkValidating()
+{
+  MOZ_ASSERT(GetValidator());
+  mValidating = true;
+}
+
+void
+imgRequestProxy::ClearValidating()
+{
+  MOZ_ASSERT(mValidating);
+  MOZ_ASSERT(!GetValidator());
+  mValidating = false;
 
   // If we'd previously requested a synchronous decode, request a decode on the
   // new image.
   if (mDecodeRequested) {
+    mDecodeRequested = false;
     StartDecoding(imgIContainer::FLAG_NONE);
   }
-
-  return NS_OK;
 }
 
 bool
 imgRequestProxy::IsOnEventTarget() const
 {
   // Ensure we are in some main thread context because the scheduler group
   // methods are only safe to call on the main thread.
   MOZ_ASSERT(NS_IsMainThread());
@@ -346,21 +361,38 @@ imgRequestProxy::AddToOwner(nsIDocument*
       MOZ_ASSERT(mEventTarget);
     }
   }
 
   if (mListener && !mEventTarget) {
     mEventTarget = do_GetMainThread();
   }
 
-  if (!GetOwner()) {
+  imgRequest* owner = GetOwner();
+  if (!owner) {
     return;
   }
 
-  GetOwner()->AddProxy(this);
+  owner->AddProxy(this);
+}
+
+void
+imgRequestProxy::RemoveFromOwner(nsresult aStatus)
+{
+  imgRequest* owner = GetOwner();
+  if (owner) {
+    if (mValidating) {
+      imgCacheValidator* validator = owner->GetValidator();
+      MOZ_ASSERT(validator);
+      validator->RemoveProxy(this);
+      mValidating = false;
+    }
+
+    owner->RemoveProxy(this, aStatus);
+  }
 }
 
 void
 imgRequestProxy::AddToLoadGroup()
 {
   NS_ASSERTION(!mIsInLoadGroup, "Whaa, we're already in the loadgroup!");
   MOZ_ASSERT(!mForceDispatchLoadGroup);
 
@@ -489,20 +521,17 @@ imgRequestProxy::Cancel(nsresult status)
 
   nsCOMPtr<nsIRunnable> ev = new imgCancelRunnable(this, status);
   return DispatchWithTargetIfAvailable(ev.forget());
 }
 
 void
 imgRequestProxy::DoCancel(nsresult status)
 {
-  if (GetOwner()) {
-    GetOwner()->RemoveProxy(this, status);
-  }
-
+  RemoveFromOwner(status);
   RemoveFromLoadGroup();
   NullOutListener();
 }
 
 NS_IMETHODIMP
 imgRequestProxy::CancelAndForgetObserver(nsresult aStatus)
 {
   // If mCanceled is true but mListener is non-null, that means
@@ -514,27 +543,17 @@ imgRequestProxy::CancelAndForgetObserver
   if (mCanceled && !mListener) {
     return NS_ERROR_FAILURE;
   }
 
   LOG_SCOPE(gImgLog, "imgRequestProxy::CancelAndForgetObserver");
 
   mCanceled = true;
   mForceDispatchLoadGroup = true;
-
-  imgRequest* owner = GetOwner();
-  if (owner) {
-    imgCacheValidator* validator = owner->GetValidator();
-    if (validator) {
-      validator->RemoveProxy(this);
-    }
-
-    owner->RemoveProxy(this, aStatus);
-  }
-
+  RemoveFromOwner(aStatus);
   RemoveFromLoadGroup();
   mForceDispatchLoadGroup = false;
 
   NullOutListener();
 
   return NS_OK;
 }
 
@@ -854,25 +873,26 @@ imgRequestProxy::PerformClone(imgINotifi
     return rv;
   }
 
   // Assign to *aClone before calling Notify so that if the caller expects to
   // only be notified for requests it's already holding pointers to it won't be
   // surprised.
   NS_ADDREF(*aClone = clone);
 
-  if (GetOwner() && GetOwner()->GetValidator()) {
+  imgCacheValidator* validator = GetValidator();
+  if (validator) {
     // Note that if we have a validator, we don't want to issue notifications at
     // 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);
+    clone->MarkValidating();
+    validator->AddProxy(clone);
   } else {
     // 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;
@@ -1265,16 +1285,26 @@ imgRequestProxy::HasImage() const
 }
 
 imgRequest*
 imgRequestProxy::GetOwner() const
 {
   return mBehaviour->GetOwner();
 }
 
+imgCacheValidator*
+imgRequestProxy::GetValidator() const
+{
+  imgRequest* owner = GetOwner();
+  if (!owner) {
+    return nullptr;
+  }
+  return owner->GetValidator();
+}
+
 ////////////////// imgRequestProxyStatic methods
 
 class StaticBehaviour : public ProxyBehaviour
 {
 public:
   explicit StaticBehaviour(mozilla::image::Image* aImage) : mImage(aImage) {}
 
   already_AddRefed<mozilla::image::Image>
--- a/image/imgRequestProxy.h
+++ b/image/imgRequestProxy.h
@@ -25,16 +25,17 @@
 #define NS_IMGREQUESTPROXY_CID \
 { /* 20557898-1dd2-11b2-8f65-9c462ee2bc95 */         \
      0x20557898,                                     \
      0x1dd2,                                         \
      0x11b2,                                         \
     {0x8f, 0x65, 0x9c, 0x46, 0x2e, 0xe2, 0xbc, 0x95} \
 }
 
+class imgCacheValidator;
 class imgINotificationObserver;
 class imgStatusNotifyRunnable;
 class ProxyBehaviour;
 
 namespace mozilla {
 namespace dom {
 class TabGroup;
 }
@@ -106,25 +107,31 @@ public:
   virtual void Notify(int32_t aType,
                       const mozilla::gfx::IntRect* aRect = nullptr) override;
   virtual void OnLoadComplete(bool aLastPart) override;
 
   // Other, internal-only methods:
   virtual void SetHasImage() override;
 
   // Whether we want notifications from ProgressTracker to be deferred until
-  // an event it has scheduled has been fired.
+  // an event it has scheduled has been fired and/or validation is complete.
   virtual bool NotificationsDeferred() const override
   {
-    return mDeferNotifications;
+    return IsValidating() || mDeferNotifications;
   }
   virtual void SetNotificationsDeferred(bool aDeferNotifications) override
   {
     mDeferNotifications = aDeferNotifications;
   }
+  bool IsValidating() const
+  {
+    return mValidating;
+  }
+  void MarkValidating();
+  void ClearValidating();
 
   bool IsOnEventTarget() const;
   already_AddRefed<nsIEventTarget> GetEventTarget() const override;
 
   // Removes all animation consumers that were created with
   // IncrementAnimationConsumers. This is necessary since we need
   // to do it before the proxy itself is destroyed. See
   // imgRequest::RemoveProxy
@@ -189,16 +196,17 @@ protected:
       return nullptr;
     }
     return GetOwner()->GetTimedChannel();
   }
 
   already_AddRefed<Image> GetImage() const;
   bool HasImage() const;
   imgRequest* GetOwner() const;
+  imgCacheValidator* GetValidator() const;
 
   nsresult PerformClone(imgINotificationObserver* aObserver,
                         nsIDocument* aLoadingDocument,
                         bool aSyncNotify,
                         imgRequestProxy** aClone);
 
   virtual imgRequestProxy* NewClonedProxy();
 
@@ -207,16 +215,17 @@ public:
 
 protected:
   mozilla::UniquePtr<ProxyBehaviour> mBehaviour;
 
 private:
   friend class imgCacheValidator;
 
   void AddToOwner(nsIDocument* aLoadingDocument);
+  void RemoveFromOwner(nsresult aStatus);
 
   nsresult DispatchWithTargetIfAvailable(already_AddRefed<nsIRunnable> aEvent);
   void DispatchWithTarget(already_AddRefed<nsIRunnable> aEvent);
 
   // The URI of our request.
   RefPtr<ImageURL> mURI;
 
   // mListener is only promised to be a weak ref (see imgILoader.idl),
@@ -237,16 +246,17 @@ private:
   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 mValidating : 1;
   bool mHadListener : 1;
   bool mHadDispatch : 1;
 };
 
 // Used for static image proxies for which no requests are available, so
 // certain behaviours must be overridden to compensate.
 class imgRequestProxyStatic : public imgRequestProxy
 {