Bug 1359833 - Part 1. ProgressTracker should select an event target f…

…rom observers and expose to callers. r=tnikkel

image/IProgressObserver.h

@@ -10,6 +10,8 @@
 #include "nsISupports.h"
 #include "nsRect.h"
 
+class nsIEventTarget;
+
 namespace mozilla {
 namespace image {
 
@@ -47,6 +49,11 @@ class IProgressObserver : public SupportsWeakPtr<IProgressObserver>
   virtual bool NotificationsDeferred() const = 0;
   virtual void SetNotificationsDeferred(bool aDeferNotifications) = 0;
 
+  virtual already_AddRefed<nsIEventTarget> GetEventTarget() const
+  {
+    return nullptr;
+  }
+
 protected:
   virtual ~IProgressObserver() { }
 };

image/ProgressTracker.cpp

@@ -16,6 +16,7 @@
 
 #include "mozilla/Assertions.h"
 #include "mozilla/Services.h"
+#include "mozilla/SystemGroup.h"
 
 using mozilla::WeakPtr;
 
@@ -69,10 +70,20 @@ CheckProgressConsistency(Progress aOldProgress, Progress aNewProgress, bool aIsM
   }
 }
 
+ProgressTracker::ProgressTracker()
+  : mMutex("ProgressTracker::mMutex")
+  , mImage(nullptr)
+  , mEventTarget(WrapNotNull(nsCOMPtr<nsIEventTarget>(SystemGroup::EventTargetFor(TaskCategory::Other))))
+  , mObserversWithTargets(0)
+  , mObservers(new ObserverTable)
+  , mProgress(NoProgress)
+  , mIsMultipart(false)
+{ }
+
 void
 ProgressTracker::SetImage(Image* aImage)
 {
-  MutexAutoLock lock(mImageMutex);
+  MutexAutoLock lock(mMutex);
   MOZ_ASSERT(aImage, "Setting null image");
   MOZ_ASSERT(!mImage, "Setting image when we already have one");
   mImage = aImage;
@@ -81,7 +92,7 @@ ProgressTracker::SetImage(Image* aImage)
 void
 ProgressTracker::ResetImage()
 {
-  MutexAutoLock lock(mImageMutex);
+  MutexAutoLock lock(mMutex);
   MOZ_ASSERT(mImage, "Resetting image when it's already null!");
   mImage = nullptr;
 }
@@ -193,7 +204,7 @@ ProgressTracker::Notify(IProgressObserver* aObserver)
     runnable->AddObserver(aObserver);
   } else {
     mRunnable = new AsyncNotifyRunnable(this, aObserver);
-    NS_DispatchToCurrentThread(mRunnable);
+    mEventTarget->Dispatch(mRunnable, NS_DISPATCH_NORMAL);
   }
 }
 
@@ -251,7 +262,7 @@ ProgressTracker::NotifyCurrentState(IProgressObserver* aObserver)
 
   nsCOMPtr<nsIRunnable> ev = new AsyncNotifyCurrentStateRunnable(this,
                                                                  aObserver);
-  NS_DispatchToCurrentThread(ev);
+  mEventTarget->Dispatch(ev.forget(), NS_DISPATCH_NORMAL);
 }
 
 /**
@@ -448,19 +459,47 @@ ProgressTracker::EmulateRequestFinished(IProgressObserver* aObserver)
   }
 }
 
+already_AddRefed<nsIEventTarget>
+ProgressTracker::GetEventTarget() const
+{
+  MutexAutoLock lock(mMutex);
+  nsCOMPtr<nsIEventTarget> target = mEventTarget;
+  return target.forget();
+}
+
 void
 ProgressTracker::AddObserver(IProgressObserver* aObserver)
 {
   MOZ_ASSERT(NS_IsMainThread());
   RefPtr<IProgressObserver> observer = aObserver;
 
+  nsCOMPtr<nsIEventTarget> target = observer->GetEventTarget();
+  if (target) {
+    if (mObserversWithTargets == 0) {
+      // On the first observer with a target (i.e. listener), always accept its
+      // event target; this may be for a specific DocGroup, or it may be the
+      // unlabelled main thread target.
+      MutexAutoLock lock(mMutex);
+      mEventTarget = WrapNotNull(target);
+    } else if (mEventTarget.get() != target.get()) {
+      // If a subsequent observer comes in with a different target, we need to
+      // switch to use the unlabelled main thread target, if we haven't already.
+      MutexAutoLock lock(mMutex);
+      nsCOMPtr<nsIEventTarget> mainTarget(do_GetMainThread());
+      mEventTarget = WrapNotNull(mainTarget);
+    }
+    ++mObserversWithTargets;
+  }
+
   mObservers.Write([=](ObserverTable* aTable) {
     MOZ_ASSERT(!aTable->Get(observer, nullptr),
                "Adding duplicate entry for image observer");
 
     WeakPtr<IProgressObserver> weakPtr = observer.get();
     aTable->Put(observer, weakPtr);
   });
+
+  MOZ_ASSERT(mObserversWithTargets <= ObserverCount());
 }
 
 bool
@@ -470,10 +509,33 @@ ProgressTracker::RemoveObserver(IProgressObserver* aObserver)
   RefPtr<IProgressObserver> observer = aObserver;
 
   // Remove the observer from the list.
-  bool removed = mObservers.Write([=](ObserverTable* aTable) {
+  bool removed = mObservers.Write([observer](ObserverTable* aTable) {
     return aTable->Remove(observer);
   });
 
+  // Sometimes once an image is decoded, and all of its observers removed, a new
+  // document may request the same image. Thus we need to clear our event target
+  // state when the last observer is removed, so that we select the most
+  // appropriate event target when a new observer is added. Since the event
+  // target may have changed (e.g. due to the scheduler group going away before
+  // we were removed), so we should be cautious comparing this target against
+  // anything at this stage.
+  if (removed) {
+    nsCOMPtr<nsIEventTarget> target = observer->GetEventTarget();
+    if (target) {
+      MOZ_ASSERT(mObserversWithTargets > 0);
+      --mObserversWithTargets;
+
+      if (mObserversWithTargets == 0) {
+        MutexAutoLock lock(mMutex);
+        nsCOMPtr<nsIEventTarget> target(SystemGroup::EventTargetFor(TaskCategory::Other));
+        mEventTarget = WrapNotNull(target);
+      }
+    }
+
+    MOZ_ASSERT(mObserversWithTargets <= ObserverCount());
+  }
+
   // Observers can get confused if they don't get all the proper teardown
   // notifications. Part ways on good terms.
   if (removed && !aObserver->NotificationsDeferred()) {

image/ProgressTracker.h

@@ -8,6 +8,7 @@
 #define mozilla_image_ProgressTracker_h
 
 #include "CopyOnWrite.h"
+#include "mozilla/NotNull.h"
 #include "mozilla/Mutex.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/WeakPtr.h"
@@ -113,18 +114,12 @@ class ProgressTracker : public mozilla::SupportsWeakPtr<ProgressTracker>
   MOZ_DECLARE_WEAKREFERENCE_TYPENAME(ProgressTracker)
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ProgressTracker)
 
-  ProgressTracker()
-    : mImageMutex("ProgressTracker::mImage")
-    , mImage(nullptr)
-    , mObservers(new ObserverTable)
-    , mProgress(NoProgress)
-    , mIsMultipart(false)
-  { }
+  ProgressTracker();
 
-  bool HasImage() const { MutexAutoLock lock(mImageMutex); return mImage; }
+  bool HasImage() const { MutexAutoLock lock(mMutex); return mImage; }
   already_AddRefed<Image> GetImage() const
   {
-    MutexAutoLock lock(mImageMutex);
+    MutexAutoLock lock(mMutex);
     RefPtr<Image> image = mImage;
     return image.forget();
   }
@@ -192,6 +187,9 @@ class ProgressTracker : public mozilla::SupportsWeakPtr<ProgressTracker>
   bool RemoveObserver(IProgressObserver* aObserver);
   uint32_t ObserverCount() const;
 
+  // Get the event target we should currently dispatch events to.
+  already_AddRefed<nsIEventTarget> GetEventTarget() const;
+
   // Resets our weak reference to our image. Image subclasses should call this
   // in their destructor.
   void ResetImage();
@@ -220,11 +218,29 @@ class ProgressTracker : public mozilla::SupportsWeakPtr<ProgressTracker>
   // The runnable, if any, that we've scheduled to deliver async notifications.
   nsCOMPtr<nsIRunnable> mRunnable;
 
+  // mMutex protects access to mImage and mEventTarget.
+  mutable Mutex mMutex;
+
   // mImage is a weak ref; it should be set to null when the image goes out of
-  // scope. mImageMutex protects mImage.
-  mutable Mutex mImageMutex;
+  // scope.
   Image* mImage;
 
+  // mEventTarget is the current, best effort event target to dispatch
+  // notifications to from the decoder threads. It will change as observers are
+  // added and removed (see mObserversWithTargets).
+  NotNull<nsCOMPtr<nsIEventTarget>> mEventTarget;
+
+  // How many observers have been added that have an explicit event target.
+  // When the first observer is added with an explicit event target, we will
+  // default to that as long as all observers use the same target. If a new
+  // observer is added which has a different event target, we will switch to
+  // using the unlabeled main thread event target which is safe for all
+  // observers. If all observers with explicit event targets are removed, we
+  // will revert back to the initial event target (for SystemGroup). An
+  // observer without an explicit event target does not care what context it
+  // is dispatched in, and thus does not impact the state.
+  uint32_t mObserversWithTargets;
+
   // Hashtable of observers attached to the image. Each observer represents a
   // consumer using the image. Main thread only.
   CopyOnWrite<ObserverTable> mObservers;