Bug 942164 - Store weak references to imgRequest consumers. r=seth, a=bajaj
authorJosh Matthews <josh@joshmatthews.net>
Thu, 12 Dec 2013 16:17:35 -0500
changeset 174988 d7c2fcbc1c35656b4757750848f4863eff8c7121
parent 174987 0d22ff0e03204e57066ad45f45a964eb088e07fe
child 174989 b1aca8e61c5cf5ed2173616d33bd77aa13a99780
push id3224
push userlsblakk@mozilla.com
push dateTue, 04 Feb 2014 01:06:49 +0000
treeherdermozilla-beta@60c04d0987f1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersseth, bajaj
bugs942164
milestone28.0a2
Bug 942164 - Store weak references to imgRequest consumers. r=seth, a=bajaj
image/src/imgRequest.cpp
image/src/imgRequestProxy.h
image/src/imgStatusTracker.cpp
image/src/imgStatusTracker.h
--- a/image/src/imgRequest.cpp
+++ b/image/src/imgRequest.cpp
@@ -632,17 +632,17 @@ NS_IMETHODIMP imgRequest::OnStartRequest
       // Image object not created until OnDataAvailable, so forward to static
       // DecodePool directly.
       nsCOMPtr<nsIEventTarget> target = RasterImage::GetEventTarget();
       rv = retargetable->RetargetDeliveryTo(target);
     }
     PR_LOG(GetImgLog(), PR_LOG_WARNING,
            ("[this=%p] imgRequest::OnStartRequest -- "
             "RetargetDeliveryTo rv %d=%s\n",
-            this, NS_SUCCEEDED(rv) ? "succeeded" : "failed", rv));
+            this, rv, NS_SUCCEEDED(rv) ? "succeeded" : "failed"));
   }
 
   return NS_OK;
 }
 
 /* void onStopRequest (in nsIRequest request, in nsISupports ctxt, in nsresult status); */
 NS_IMETHODIMP imgRequest::OnStopRequest(nsIRequest *aRequest, nsISupports *ctxt, nsresult status)
 {
--- a/image/src/imgRequestProxy.h
+++ b/image/src/imgRequestProxy.h
@@ -2,16 +2,17 @@
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef imgRequestProxy_h__
 #define imgRequestProxy_h__
 
+#include "mozilla/WeakPtr.h"
 #include "imgIRequest.h"
 #include "nsISecurityInfoProvider.h"
 
 #include "nsILoadGroup.h"
 #include "nsISupportsPriority.h"
 #include "nsITimedChannel.h"
 #include "nsCOMPtr.h"
 #include "nsAutoPtr.h"
@@ -38,17 +39,18 @@ namespace image {
 class Image;
 class ImageURL;
 } // namespace image
 } // namespace mozilla
 
 class imgRequestProxy : public imgIRequest,
                         public nsISupportsPriority,
                         public nsISecurityInfoProvider,
-                        public nsITimedChannel
+                        public nsITimedChannel,
+                        public mozilla::SupportsWeakPtr<imgRequestProxy>
 {
 public:
   typedef mozilla::image::ImageURL ImageURL;
   NS_DECL_ISUPPORTS
   NS_DECL_IMGIREQUEST
   NS_DECL_NSIREQUEST
   NS_DECL_NSISUPPORTSPRIORITY
   NS_DECL_NSISECURITYINFOPROVIDER
--- a/image/src/imgStatusTracker.cpp
+++ b/image/src/imgStatusTracker.cpp
@@ -13,16 +13,17 @@
 #include "ImageLogging.h"
 #include "nsNetUtil.h"
 #include "nsIObserverService.h"
 
 #include "mozilla/Assertions.h"
 #include "mozilla/Services.h"
 
 using namespace mozilla::image;
+using mozilla::WeakPtr;
 
 class imgStatusTrackerObserver : public imgDecoderObserver
 {
 public:
   imgStatusTrackerObserver(imgStatusTracker* aTracker)
   : mTracker(aTracker->asWeakPtr())
   {
     MOZ_ASSERT(aTracker);
@@ -138,17 +139,17 @@ public:
   {
     LOG_SCOPE(GetImgLog(), "imgStatusTrackerObserver::OnError");
     nsRefPtr<imgStatusTracker> tracker = mTracker.get();
     if (!tracker) { return; }
     tracker->RecordError();
   }
 
 private:
-  mozilla::WeakPtr<imgStatusTracker> mTracker;
+  WeakPtr<imgStatusTracker> mTracker;
 };
 
 // imgStatusTracker methods
 
 imgStatusTracker::imgStatusTracker(Image* aImage)
   : mImage(aImage),
     mState(0),
     mImageStatus(imgIRequest::STATUS_NONE),
@@ -352,27 +353,27 @@ imgStatusTracker::NotifyCurrentState(img
 
   // We don't keep track of
   nsCOMPtr<nsIRunnable> ev = new imgStatusNotifyRunnable(this, proxy);
   NS_DispatchToCurrentThread(ev);
 }
 
 #define NOTIFY_IMAGE_OBSERVERS(func) \
   do { \
-    nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(proxies); \
+    ProxyArray::ForwardIterator iter(proxies); \
     while (iter.HasMore()) { \
-      nsRefPtr<imgRequestProxy> proxy = iter.GetNext(); \
-      if (!proxy->NotificationsDeferred()) { \
+      nsRefPtr<imgRequestProxy> proxy = iter.GetNext().get(); \
+      if (proxy && !proxy->NotificationsDeferred()) { \
         proxy->func; \
       } \
     } \
   } while (false);
 
 /* static */ void
-imgStatusTracker::SyncNotifyState(nsTObserverArray<imgRequestProxy*>& proxies,
+imgStatusTracker::SyncNotifyState(ProxyArray& proxies,
                                   bool hasImage, uint32_t state,
                                   nsIntRect& dirtyRect, bool hadLastPart)
 {
   MOZ_ASSERT(NS_IsMainThread());
   // OnStartRequest
   if (state & stateRequestStarted)
     NOTIFY_IMAGE_OBSERVERS(OnStartRequest());
 
@@ -500,23 +501,23 @@ imgStatusTracker::SyncNotifyDifference(c
 
   nsIntRect invalidRect = mInvalidRect.Union(diff.invalidRect);
 
   SyncNotifyState(mConsumers, !!mImage, diff.diffState, invalidRect, mHadLastPart);
 
   mInvalidRect.SetEmpty();
 
   if (diff.unblockedOnload) {
-    nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mConsumers);
+    ProxyArray::ForwardIterator iter(mConsumers);
     while (iter.HasMore()) {
       // Hold on to a reference to this proxy, since notifying the state can
       // cause it to disappear.
-      nsRefPtr<imgRequestProxy> proxy = iter.GetNext();
+      nsRefPtr<imgRequestProxy> proxy = iter.GetNext().get();
 
-      if (!proxy->NotificationsDeferred()) {
+      if (proxy && !proxy->NotificationsDeferred()) {
         SendUnblockOnload(proxy);
       }
     }
   }
 
   if (diff.foundError) {
     FireFailureNotification();
   }
@@ -545,18 +546,18 @@ imgStatusTracker::SyncNotify(imgRequestP
 
   nsIntRect r;
   if (mImage) {
     // XXX - Should only send partial rects here, but that needs to
     // wait until we fix up the observer interface
     r = mImage->FrameRect(imgIContainer::FRAME_CURRENT);
   }
 
-  nsTObserverArray<imgRequestProxy*> array;
-  array.AppendElement(proxy);
+  ProxyArray array;
+  array.AppendElement(proxy->asWeakPtr());
   SyncNotifyState(array, !!mImage, mState, r, mHadLastPart);
 }
 
 void
 imgStatusTracker::EmulateRequestFinished(imgRequestProxy* aProxy,
                                          nsresult aStatus)
 {
   MOZ_ASSERT(NS_IsMainThread(),
@@ -577,17 +578,17 @@ imgStatusTracker::EmulateRequestFinished
     aProxy->OnStopRequest(true);
   }
 }
 
 void
 imgStatusTracker::AddConsumer(imgRequestProxy* aConsumer)
 {
   MOZ_ASSERT(NS_IsMainThread());
-  mConsumers.AppendElementUnlessExists(aConsumer);
+  mConsumers.AppendElementUnlessExists(aConsumer->asWeakPtr());
 }
 
 // XXX - The last argument should go away.
 bool
 imgStatusTracker::RemoveConsumer(imgRequestProxy* aConsumer, nsresult aStatus)
 {
   MOZ_ASSERT(NS_IsMainThread());
   // Remove the proxy from the list.
@@ -605,16 +606,30 @@ imgStatusTracker::RemoveConsumer(imgRequ
   if (aConsumer->NotificationsDeferred() && runnable) {
     runnable->RemoveProxy(aConsumer);
     aConsumer->SetNotificationsDeferred(false);
   }
 
   return removed;
 }
 
+bool
+imgStatusTracker::FirstConsumerIs(imgRequestProxy* aConsumer)
+{
+  MOZ_ASSERT(NS_IsMainThread(), "Use mConsumers on main thread only");
+  ProxyArray::ForwardIterator iter(mConsumers);
+  while (iter.HasMore()) {
+    nsRefPtr<imgRequestProxy> proxy = iter.GetNext().get();
+    if (proxy) {
+      return proxy.get() == aConsumer;
+    }
+  }
+  return false;
+}
+
 void
 imgStatusTracker::RecordCancel()
 {
   if (!(mImageStatus & imgIRequest::STATUS_LOAD_PARTIAL))
     mImageStatus = imgIRequest::STATUS_ERROR;
 }
 
 void
@@ -776,19 +791,22 @@ imgStatusTracker::SendUnlockedDraw(imgRe
     aProxy->OnUnlockedDraw();
 }
 
 void
 imgStatusTracker::OnUnlockedDraw()
 {
   MOZ_ASSERT(NS_IsMainThread());
   RecordUnlockedDraw();
-  nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mConsumers);
+  ProxyArray::ForwardIterator iter(mConsumers);
   while (iter.HasMore()) {
-    SendUnlockedDraw(iter.GetNext());
+    nsRefPtr<imgRequestProxy> proxy = iter.GetNext().get();
+    if (proxy) {
+      SendUnlockedDraw(proxy);
+    }
   }
 }
 
 void
 imgStatusTracker::RecordFrameChanged(const nsIntRect* aDirtyRect)
 {
   NS_ABORT_IF_FALSE(mImage,
                     "RecordFrameChanged called before we have an Image");
@@ -833,19 +851,22 @@ imgStatusTracker::SendStartRequest(imgRe
     aProxy->OnStartRequest();
 }
 
 void
 imgStatusTracker::OnStartRequest()
 {
   MOZ_ASSERT(NS_IsMainThread());
   RecordStartRequest();
-  nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mConsumers);
+  ProxyArray::ForwardIterator iter(mConsumers);
   while (iter.HasMore()) {
-    SendStartRequest(iter.GetNext());
+    nsRefPtr<imgRequestProxy> proxy = iter.GetNext().get();
+    if (proxy) {
+      SendStartRequest(proxy);
+    }
   }
 }
 
 void
 imgStatusTracker::RecordStopRequest(bool aLastPart,
                                     nsresult aStatus)
 {
   mHadLastPart = aLastPart;
@@ -904,80 +925,95 @@ imgStatusTracker::OnStopRequest(bool aLa
     NS_DispatchToMainThread(
       new OnStopRequestEvent(this, aLastPart, aStatus));
     return;
   }
   bool preexistingError = mImageStatus == imgIRequest::STATUS_ERROR;
 
   RecordStopRequest(aLastPart, aStatus);
   /* notify the kids */
-  nsTObserverArray<imgRequestProxy*>::ForwardIterator srIter(mConsumers);
+  ProxyArray::ForwardIterator srIter(mConsumers);
   while (srIter.HasMore()) {
-    SendStopRequest(srIter.GetNext(), aLastPart, aStatus);
+    nsRefPtr<imgRequestProxy> proxy = srIter.GetNext().get();
+    if (proxy) {
+      SendStopRequest(proxy, aLastPart, aStatus);
+    }
   }
 
   if (NS_FAILED(aStatus) && !preexistingError) {
     FireFailureNotification();
   }
 }
 
 void
 imgStatusTracker::OnDiscard()
 {
   MOZ_ASSERT(NS_IsMainThread());
   RecordDiscard();
 
   /* notify the kids */
-  nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mConsumers);
+  ProxyArray::ForwardIterator iter(mConsumers);
   while (iter.HasMore()) {
-    SendDiscard(iter.GetNext());
+    nsRefPtr<imgRequestProxy> proxy = iter.GetNext().get();
+    if (proxy) {
+      SendDiscard(proxy);
+    }
   }
 }
 
 void
 imgStatusTracker::FrameChanged(const nsIntRect* aDirtyRect)
 {
   MOZ_ASSERT(NS_IsMainThread());
   RecordFrameChanged(aDirtyRect);
 
   /* notify the kids */
-  nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mConsumers);
+  ProxyArray::ForwardIterator iter(mConsumers);
   while (iter.HasMore()) {
-    SendFrameChanged(iter.GetNext(), aDirtyRect);
+    nsRefPtr<imgRequestProxy> proxy = iter.GetNext().get();
+    if (proxy) {
+      SendFrameChanged(proxy, aDirtyRect);
+    }
   }
 }
 
 void
 imgStatusTracker::OnStopFrame()
 {
   MOZ_ASSERT(NS_IsMainThread());
   RecordStopFrame();
 
   /* notify the kids */
-  nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mConsumers);
+  ProxyArray::ForwardIterator iter(mConsumers);
   while (iter.HasMore()) {
-    SendStopFrame(iter.GetNext());
+    nsRefPtr<imgRequestProxy> proxy = iter.GetNext().get();
+    if (proxy) {
+      SendStopFrame(proxy);
+    }
   }
 }
 
 void
 imgStatusTracker::OnDataAvailable()
 {
   if (!NS_IsMainThread()) {
     // Note: SetHasImage calls Image::Lock and Image::IncrementAnimationCounter
     // so subsequent calls or dispatches which Unlock or Decrement~ should
     // be issued after this to avoid race conditions.
     NS_DispatchToMainThread(
       NS_NewRunnableMethod(this, &imgStatusTracker::OnDataAvailable));
     return;
   }
   // Notify any imgRequestProxys that are observing us that we have an Image.
-  nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mConsumers);
+  ProxyArray::ForwardIterator iter(mConsumers);
   while (iter.HasMore()) {
-    iter.GetNext()->SetHasImage();
+    nsRefPtr<imgRequestProxy> proxy = iter.GetNext().get();
+    if (proxy) {
+      proxy->SetHasImage();
+    }
   }
 }
 
 void
 imgStatusTracker::RecordBlockOnload()
 {
   MOZ_ASSERT(!(mState & stateBlockingOnload));
   mState |= stateBlockingOnload;
@@ -1016,19 +1052,22 @@ imgStatusTracker::MaybeUnblockOnload()
     return;
   }
   if (!(mState & stateBlockingOnload)) {
     return;
   }
 
   RecordUnblockOnload();
 
-  nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mConsumers);
+  ProxyArray::ForwardIterator iter(mConsumers);
   while (iter.HasMore()) {
-    SendUnblockOnload(iter.GetNext());
+    nsRefPtr<imgRequestProxy> proxy = iter.GetNext().get();
+    if (proxy) {
+      SendUnblockOnload(proxy);
+    }
   }
 }
 
 void
 imgStatusTracker::RecordError()
 {
   mImageStatus = imgIRequest::STATUS_ERROR;
 }
--- a/image/src/imgStatusTracker.h
+++ b/image/src/imgStatusTracker.h
@@ -4,28 +4,28 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef imgStatusTracker_h__
 #define imgStatusTracker_h__
 
 class imgDecoderObserver;
 class imgIContainer;
-class imgRequestProxy;
 class imgStatusNotifyRunnable;
 class imgRequestNotifyRunnable;
 class imgStatusTrackerObserver;
 class nsIRunnable;
 
 #include "mozilla/RefPtr.h"
 #include "mozilla/WeakPtr.h"
 #include "nsCOMPtr.h"
 #include "nsTObserverArray.h"
 #include "nsThreadUtils.h"
 #include "nsRect.h"
+#include "imgRequestProxy.h"
 
 namespace mozilla {
 namespace image {
 
 class Image;
 
 struct ImageStatusDiff
 {
@@ -168,20 +168,17 @@ public:
   size_t ConsumerCount() const {
     MOZ_ASSERT(NS_IsMainThread(), "Use mConsumers on main thread only");
     return mConsumers.Length();
   }
 
   // This is intentionally non-general because its sole purpose is to support an
   // some obscure network priority logic in imgRequest. That stuff could probably
   // be improved, but it's too scary to mess with at the moment.
-  bool FirstConsumerIs(imgRequestProxy* aConsumer) {
-    MOZ_ASSERT(NS_IsMainThread(), "Use mConsumers on main thread only");
-    return mConsumers.SafeElementAt(0, nullptr) == aConsumer;
-  }
+  bool FirstConsumerIs(imgRequestProxy* aConsumer);
 
   void AdoptConsumers(imgStatusTracker* aTracker) {
     MOZ_ASSERT(NS_IsMainThread(), "Use mConsumers on main thread only");
     MOZ_ASSERT(aTracker);
     mConsumers = aTracker->mConsumers;
   }
 
   // Returns whether we are in the process of loading; that is, whether we have
@@ -290,44 +287,45 @@ public:
   // Notify for the changes captured in an ImageStatusDiff. Because this may
   // result in recursive notifications, no decoding locks may be held.
   // Called on the main thread only.
   void SyncNotifyDifference(const mozilla::image::ImageStatusDiff& aDiff);
 
   nsIntRect GetInvalidRect() const { return mInvalidRect; }
 
 private:
+  typedef nsTObserverArray<mozilla::WeakPtr<imgRequestProxy>> ProxyArray;
   friend class imgStatusNotifyRunnable;
   friend class imgRequestNotifyRunnable;
   friend class imgStatusTrackerObserver;
   friend class imgStatusTrackerInit;
   imgStatusTracker(const imgStatusTracker& aOther);
 
   // Main thread only because it deals with the observer service.
   void FireFailureNotification();
 
   // Main thread only, since imgRequestProxy calls are expected on the main
   // thread, and mConsumers is not threadsafe.
-  static void SyncNotifyState(nsTObserverArray<imgRequestProxy*>& proxies,
+  static void SyncNotifyState(ProxyArray& proxies,
                               bool hasImage, uint32_t state,
                               nsIntRect& dirtyRect, bool hadLastPart);
 
   nsCOMPtr<nsIRunnable> mRequestRunnable;
 
   // The invalid area of the most recent frame we know about. (All previous
   // frames are assumed to be fully valid.)
   nsIntRect mInvalidRect;
 
   // This weak ref should be set null when the image goes out of scope.
   mozilla::image::Image* mImage;
 
   // List of proxies attached to the image. Each proxy represents a consumer
   // using the image. Array and/or individual elements should only be accessed
   // on the main thread.
-  nsTObserverArray<imgRequestProxy*> mConsumers;
+  ProxyArray mConsumers;
 
   mozilla::RefPtr<imgDecoderObserver> mTrackerObserver;
 
   uint32_t mState;
   uint32_t mImageStatus;
   bool mIsMultipart    : 1;
   bool mHadLastPart    : 1;
   bool mHasBeenDecoded : 1;