Bug 572520: step 7, delay otherwise-synchronous notifications, and send them asynchronously later. r=jrmuizel,bholley sr=bz

modules/libpr0n/public/imgIDecoderObserver.idl

@@ -86,9 +86,6 @@ interface imgIDecoderObserver : imgIContainerObserver
    * called at the same time that nsIRequestObserver::onStartRequest would be
    * (used only for observers of imgIRequest objects, which are nsIRequests,
    * not imgIDecoder objects)
-   *
-   * Unlike nsIRequestObserver::onStartRequest, this can be called
-   * synchronously.
    */
   void onStartRequest(in imgIRequest aRequest);
 
@@ -158,9 +155,6 @@ interface imgIDecoderObserver : imgIContainerObserver
    * called at the same time that nsIRequestObserver::onStopRequest would be
    * (used only for observers of imgIRequest objects, which are nsIRequests,
    * not imgIDecoder objects)
-   *
-   * Unlike nsIRequestObserver::onStartRequest, this can be called
-   * synchronously.
    */
   void onStopRequest(in imgIRequest aRequest, in boolean aIsLastPart);
 

modules/libpr0n/src/imgContainer.cpp

@@ -2329,8 +2329,8 @@ imgContainer::RequestDecode()
     return NS_OK;
 
   // If we get this far, dispatch the worker. We do this instead of starting
-  // any immediate decoding so that actions like tabbing-over to a tab with
-  // large undecoded images don't incur an annoying lag.
+  // any immediate decoding to guarantee that all our decode notifications are
+  // dispatched asynchronously, and to ensure we stay responsive.
   return mWorker->Dispatch();
 }
 

modules/libpr0n/src/imgLoader.cpp

@@ -1677,7 +1677,13 @@ NS_IMETHODIMP imgLoader::LoadImage(nsIURI *aURI,
     // the proxy will be removed from the loadgroup.
     proxy->AddToLoadGroup();
 
-    proxy->NotifyListener();
+    // If we're loading off the network, explicitly don't notify our proxy,
+    // because necko (or things called from necko, such as imgCacheValidator)
+    // are going to call our notifications asynchronously, and we can't make it
+    // further asynchronous because observers might rely on imagelib completing
+    // its work between the channel's OnStartRequest and OnStopRequest.
+    if (!newChannel)
+      proxy->NotifyListener();
 
     return rv;
   }
@@ -1759,12 +1765,19 @@ NS_IMETHODIMP imgLoader::LoadImageWithChannel(nsIChannel *channel, imgIDecoderOb
   nsCOMPtr<nsILoadGroup> loadGroup;
   channel->GetLoadGroup(getter_AddRefs(loadGroup));
 
+  // XXX: It looks like the wrong load flags are being passed in...
+  requestFlags &= 0xFFFF;
+
   if (request) {
     // we have this in our cache already.. cancel the current (document) load
 
     channel->Cancel(NS_ERROR_PARSED_DATA_CACHED); // this should fire an OnStopRequest
 
     *listener = nsnull; // give them back a null nsIStreamListener
+
+    rv = CreateNewProxyForRequest(request, loadGroup, aObserver,
+                                  requestFlags, nsnull, _retval);
+    static_cast<imgRequestProxy*>(*_retval)->NotifyListener();
   } else {
     if (!NewRequestAndEntry(uri, getter_AddRefs(request), getter_AddRefs(entry)))
       return NS_ERROR_OUT_OF_MEMORY;
@@ -1789,15 +1802,18 @@ NS_IMETHODIMP imgLoader::LoadImageWithChannel(nsIChannel *channel, imgIDecoderOb
 
     // Try to add the new request into the cache.
     PutIntoCache(uri, entry);
+
+    rv = CreateNewProxyForRequest(request, loadGroup, aObserver,
+                                  requestFlags, nsnull, _retval);
+
+    // Explicitly don't notify our proxy, because we're loading off the
+    // network, and necko (or things called from necko, such as
+    // imgCacheValidator) are going to call our notifications asynchronously,
+    // and we can't make it further asynchronous because observers might rely
+    // on imagelib completing its work between the channel's OnStartRequest and
+    // OnStopRequest.
   }
 
-  // XXX: It looks like the wrong load flags are being passed in...
-  requestFlags &= 0xFFFF;
-
-  rv = CreateNewProxyForRequest(request, loadGroup, aObserver,
-                                requestFlags, nsnull, _retval);
-  static_cast<imgRequestProxy*>(*_retval)->NotifyListener();
-
   return rv;
 }
 
@@ -2018,7 +2034,10 @@ NS_IMETHODIMP imgCacheValidator::OnStartRequest(nsIRequest *aRequest, nsISupport
       PRUint32 count = mProxies.Count();
       for (PRInt32 i = count-1; i>=0; i--) {
         imgRequestProxy *proxy = static_cast<imgRequestProxy *>(mProxies[i]);
-        proxy->NotifyListener();
+
+        // Notify synchronously, because we're already in OnStartRequest, an
+        // asynchronously-called function.
+        proxy->SyncNotifyListener();
       }
 
       mRequest->SetLoadId(mContext);
@@ -2078,7 +2097,10 @@ NS_IMETHODIMP imgCacheValidator::OnStartRequest(nsIRequest *aRequest, nsISupport
   for (PRInt32 i = count-1; i>=0; i--) {
     imgRequestProxy *proxy = static_cast<imgRequestProxy *>(mProxies[i]);
     proxy->ChangeOwner(request);
-    proxy->NotifyListener();
+
+    // Notify synchronously, because we're already in OnStartRequest, an
+    // asynchronously-called function.
+    proxy->SyncNotifyListener();
   }
 
   NS_RELEASE(request);

modules/libpr0n/src/imgRequest.h

@@ -41,7 +41,7 @@
 #ifndef imgRequest_h__
 #define imgRequest_h__
 
-#include "imgIContainer.h"
+#include "imgContainer.h"
 #include "imgIDecoder.h"
 #include "imgIDecoderObserver.h"
 
@@ -68,6 +68,7 @@ class imgCacheValidator;
 class imgRequestProxy;
 class imgCacheEntry;
 class imgMemoryReporter;
+class imgRequestNotifyRunnable;
 
 class imgRequest : public imgIDecoderObserver,
                    public nsIStreamListener,
@@ -122,6 +123,7 @@ private:
   friend class imgLoader;
   friend class imgCacheValidator;
   friend class imgCacheExpirationTracker;
+  friend class imgRequestNotifyRunnable;
 
   inline void SetLoadId(void *aLoadId) {
     mLoadId = aLoadId;

modules/libpr0n/src/imgRequestProxy.cpp

@@ -68,7 +68,8 @@ imgRequestProxy::imgRequestProxy() :
   mCanceled(PR_FALSE),
   mIsInLoadGroup(PR_FALSE),
   mListenerIsStrongRef(PR_FALSE),
-  mDecodeRequested(PR_FALSE)
+  mDecodeRequested(PR_FALSE),
+  mDeferNotifications(PR_FALSE)
 {
   /* member initializers and constructor code */
 
@@ -443,7 +444,9 @@ NS_IMETHODIMP imgRequestProxy::Clone(imgIDecoderObserver* aObserver,
   // surprised.
   NS_ADDREF(*aClone = clone);
 
-  clone->NotifyListener();
+  // 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.
+  clone->SyncNotifyListener();
 
   return NS_OK;
 }
@@ -723,11 +726,27 @@ void imgRequestProxy::SetPrincipal(nsIPrincipal *aPrincipal)
 
 void imgRequestProxy::NotifyListener()
 {
-  // It would be nice to notify the observer directly instead of through the
-  // proxy, but there are several places we do extra processing when we receive
-  // notifications (like OnStopRequest()), and we need to check mCanceled
-  // everywhere too.
+  // It would be nice to notify the observer directly in the status tracker
+  // instead of through the proxy, but there are several places we do extra
+  // processing when we receive notifications (like OnStopRequest()), and we
+  // need to check mCanceled everywhere too.
 
-  if (mListener)
-    mImage->GetStatusTracker().Notify(this);
+  if (mOwner) {
+    // Send the notifications to our listener asynchronously.
+    mImage->GetStatusTracker().Notify(mOwner, this);
+  } else {
+    // We don't have an imgRequest, so we can only notify the clone of our
+    // current state, but we still have to do that asynchronously.
+    mImage->GetStatusTracker().NotifyCurrentState(this);
+  }
+}
+
+void imgRequestProxy::SyncNotifyListener()
+{
+  // It would be nice to notify the observer directly in the status tracker
+  // instead of through the proxy, but there are several places we do extra
+  // processing when we receive notifications (like OnStopRequest()), and we
+  // need to check mCanceled everywhere too.
+
+  mImage->GetStatusTracker().SyncNotify(this);
 }

modules/libpr0n/src/imgRequestProxy.h

@@ -64,6 +64,9 @@
     {0x8f, 0x65, 0x9c, 0x46, 0x2e, 0xe2, 0xbc, 0x95} \
 }
 
+class imgRequestNotifyRunnable;
+class imgStatusNotifyRunnable;
+
 class imgRequestProxy : public imgIRequest, public nsISupportsPriority, public nsISecurityInfoProvider
 {
 public:
@@ -93,11 +96,21 @@ public:
 
   void SetPrincipal(nsIPrincipal *aPrincipal);
 
-  // Notify this proxy's listener of the current state of the request.
+  // Asynchronously notify this proxy's listener of the current state of the
+  // image, and, if we have an imgRequest mOwner, any status changes that
+  // happen between the time this function is called and the time the
+  // notification is scheduled.
   void NotifyListener();
 
+  // Synchronously notify this proxy's listener of the current state of the
+  // image. Only use this function if you are currently servicing an
+  // asynchronously-called function.
+  void SyncNotifyListener();
+
 protected:
   friend class imgStatusTracker;
+  friend class imgStatusNotifyRunnable;
+  friend class imgRequestNotifyRunnable;
 
   class imgCancelRunnable;
   friend class imgCancelRunnable;
@@ -119,6 +132,21 @@ protected:
       nsresult mStatus;
   };
 
+  // The following notification functions are protected to ensure that (friend
+  // class) imgStatusTracker is the only class allowed to send us
+  // notifications.
+
+  // Whether we want notifications from imgStatusTracker to be deferred until
+  // an event it has scheduled has been fired.
+  PRBool NotificationsDeferred() const
+  {
+    return mDeferNotifications;
+  }
+  void SetNotificationsDeferred(PRBool aDeferNotifications)
+  {
+    mDeferNotifications = aDeferNotifications;
+  }
+
   /* non-virtual imgIDecoderObserver methods */
   void OnStartDecode   ();
   void OnStartContainer(imgIContainer *aContainer);
@@ -180,6 +208,10 @@ private:
   PRPackedBool mIsInLoadGroup;
   PRPackedBool mListenerIsStrongRef;
   PRPackedBool mDecodeRequested;
+
+  // Whether we want to defer our notifications by the non-virtual Observer
+  // interfaces as image loads proceed.
+  PRPackedBool mDeferNotifications;
 };
 
 #endif // imgRequestProxy_h__

modules/libpr0n/src/imgStatusTracker.cpp

@@ -75,9 +75,77 @@ imgStatusTracker::GetImageStatus() const
   return mImageStatus;
 }
 
-void
-imgStatusTracker::Notify(imgRequestProxy* proxy)
+// A helper class to allow us to call SyncNotify asynchronously.
+class imgRequestNotifyRunnable : public nsRunnable
 {
+  public:
+    imgRequestNotifyRunnable(imgRequest* request, imgRequestProxy* requestproxy)
+      : mRequest(request), mProxy(requestproxy)
+    {}
+
+    NS_IMETHOD Run()
+    {
+      mProxy->SetNotificationsDeferred(PR_FALSE);
+
+      mRequest->mImage->GetStatusTracker().SyncNotify(mProxy);
+      return NS_OK;
+    }
+
+  private:
+    nsRefPtr<imgRequest> mRequest;
+    nsRefPtr<imgRequestProxy> mProxy;
+};
+
+void
+imgStatusTracker::Notify(imgRequest* request, imgRequestProxy* proxy)
+{
+  proxy->SetNotificationsDeferred(PR_TRUE);
+
+  nsCOMPtr<nsIRunnable> ev = new imgRequestNotifyRunnable(request, proxy);
+  NS_DispatchToCurrentThread(ev);
+}
+
+// A helper class to allow us to call SyncNotify asynchronously for a given,
+// fixed, state.
+class imgStatusNotifyRunnable : public nsRunnable
+{
+  public:
+    imgStatusNotifyRunnable(imgStatusTracker status,
+                            imgRequestProxy* requestproxy)
+      : mStatus(status), mImage(status.mImage), mProxy(requestproxy)
+    {}
+
+    NS_IMETHOD Run()
+    {
+      mProxy->SetNotificationsDeferred(PR_FALSE);
+
+      mStatus.SyncNotify(mProxy);
+      return NS_OK;
+    }
+
+  private:
+    imgStatusTracker mStatus;
+    // We have to hold on to a reference to the tracker's image, just in case
+    // it goes away while we're in the event queue.
+    nsRefPtr<imgIContainer> mImage;
+    nsRefPtr<imgRequestProxy> mProxy;
+};
+
+void
+imgStatusTracker::NotifyCurrentState(imgRequestProxy* proxy)
+{
+  proxy->SetNotificationsDeferred(PR_TRUE);
+
+  nsCOMPtr<nsIRunnable> ev = new imgStatusNotifyRunnable(*this, proxy);
+  NS_DispatchToCurrentThread(ev);
+}
+
+void
+imgStatusTracker::SyncNotify(imgRequestProxy* proxy)
+{
+  NS_ABORT_IF_FALSE(!proxy->NotificationsDeferred(),
+    "Calling imgStatusTracker::Notify() on a proxy that doesn't want notifications!");
+
   nsCOMPtr<imgIRequest> kungFuDeathGrip(proxy);
 
   // OnStartRequest
@@ -178,7 +246,8 @@ imgStatusTracker::RecordStartDecode()
 void
 imgStatusTracker::SendStartDecode(imgRequestProxy* aProxy)
 {
-  aProxy->OnStartDecode();
+  if (!aProxy->NotificationsDeferred())
+    aProxy->OnStartDecode();
 }
 
 void
@@ -194,7 +263,7 @@ imgStatusTracker::SendStartContainer(imgRequestProxy* aProxy, imgIContainer* aCo
   // We only want to send onStartContainer once, but we might get multiple
   // OnStartContainer calls (e.g. from multipart/x-mixed-replace).
   PRBool alreadySent = (mState & stateHasSize) != 0;
-  if (!alreadySent)
+  if (!alreadySent && !aProxy->NotificationsDeferred())
     aProxy->OnStartContainer(aContainer);
 }
 
@@ -208,7 +277,8 @@ imgStatusTracker::RecordStartFrame(PRUint32 aFrame)
 void
 imgStatusTracker::SendStartFrame(imgRequestProxy* aProxy, PRUint32 aFrame)
 {
-  aProxy->OnStartFrame(aFrame);
+  if (!aProxy->NotificationsDeferred())
+    aProxy->OnStartFrame(aFrame);
 }
 
 void
@@ -222,7 +292,8 @@ void
 imgStatusTracker::SendDataAvailable(imgRequestProxy* aProxy, PRBool aCurrentFrame,
                                          const nsIntRect* aRect)
 {
-  aProxy->OnDataAvailable(aCurrentFrame, aRect);
+  if (!aProxy->NotificationsDeferred())
+    aProxy->OnDataAvailable(aCurrentFrame, aRect);
 }
 
 
@@ -236,7 +307,8 @@ imgStatusTracker::RecordStopFrame(PRUint32 aFrame)
 void
 imgStatusTracker::SendStopFrame(imgRequestProxy* aProxy, PRUint32 aFrame)
 {
-  aProxy->OnStopFrame(aFrame);
+  if (!aProxy->NotificationsDeferred())
+    aProxy->OnStopFrame(aFrame);
 }
 
 void
@@ -269,8 +341,9 @@ imgStatusTracker::SendStopDecode(imgRequestProxy* aProxy, nsresult aStatus,
 {
   // See imgRequest::OnStopDecode for more information on why we call
   // OnStopContainer from here this, and why imgRequestProxy::OnStopDecode() is
-  // called from OnStopRequest().
-  aProxy->OnStopContainer(mImage);
+  // called from OnStopRequest() and SyncNotify().
+  if (!aProxy->NotificationsDeferred())
+    aProxy->OnStopContainer(mImage);
 }
 
 void
@@ -289,7 +362,8 @@ imgStatusTracker::RecordDiscard()
 void
 imgStatusTracker::SendDiscard(imgRequestProxy* aProxy)
 {
-  aProxy->OnDiscard();
+  if (!aProxy->NotificationsDeferred())
+    aProxy->OnDiscard();
 }
 
 /* non-virtual imgIContainerObserver methods */
@@ -304,7 +378,8 @@ void
 imgStatusTracker::SendFrameChanged(imgRequestProxy* aProxy, imgIContainer* aContainer,
                                    nsIntRect* aDirtyRect)
 {
-  aProxy->FrameChanged(aContainer, aDirtyRect);
+  if (!aProxy->NotificationsDeferred())
+    aProxy->FrameChanged(aContainer, aDirtyRect);
 }
 
 /* non-virtual sort-of-nsIRequestObserver methods */
@@ -327,7 +402,8 @@ imgStatusTracker::RecordStartRequest()
 void
 imgStatusTracker::SendStartRequest(imgRequestProxy* aProxy)
 {
-  aProxy->OnStartRequest();
+  if (!aProxy->NotificationsDeferred())
+    aProxy->OnStartRequest();
 }
 
 void
@@ -346,6 +422,8 @@ imgStatusTracker::SendStopRequest(imgRequestProxy* aProxy, PRBool aLastPart, nsr
 {
   // See bug 505385 and imgRequest::OnStopDecode for more information on why
   // OnStopDecode is called with OnStopRequest.
-  aProxy->OnStopDecode(GetResultFromImageStatus(mImageStatus), nsnull);
-  aProxy->OnStopRequest(aLastPart);
+  if (!aProxy->NotificationsDeferred()) {
+    aProxy->OnStopDecode(GetResultFromImageStatus(mImageStatus), nsnull);
+    aProxy->OnStopRequest(aLastPart);
+  }
 }

modules/libpr0n/src/imgStatusTracker.h

@@ -44,6 +44,7 @@ class nsIntRect;
 class imgIContainer;
 class imgRequest;
 class imgRequestProxy;
+class imgStatusNotifyRunnable;
 
 #include "prtypes.h"
 #include "nscore.h"
@@ -60,22 +61,41 @@ enum {
 /*
  * The image status tracker is a class that encapsulates all the loading and
  * decoding status about an image (imgContainer), and makes it possible to send
- * notifications to imgRequestProxys. When a new proxy needs to be notified of
- * the current state of an image, simply call the Notify() method on this class
- * with the relevant proxy as its argument.
+ * notifications to imgRequestProxys, both synchronously (i.e., the status now)
+ * and asynchronously (the status later).
+ *
+ * When a new proxy needs to be notified of the current state of an image, call
+ * the Notify() method on this class with the relevant proxy as its argument,
+ * and the notifications will be replayed to the proxy asynchronously.
  */
 
 class imgStatusTracker
 {
 public:
   // aImage is the image that this status tracker will pass to the
-  // imgRequestProxys in Notify() and EmulateRequestFinished(), and must be
+  // imgRequestProxys in SyncNotify() and EmulateRequestFinished(), and must be
   // alive as long as this instance is, because we hold a weak reference to it.
   imgStatusTracker(imgIContainer* aImage);
 
+  // Schedule an asynchronous "replaying" of all the notifications that would
+  // have to happen to put us in the current state.
+  // We will also take note of any notifications that happen between the time
+  // Notify() is called and when we call SyncNotify on |proxy|, and replay them
+  // as well.
+  void Notify(imgRequest* request, imgRequestProxy* proxy);
+
+  // Schedule an asynchronous "replaying" of all the notifications that would
+  // have to happen to put us in the state we are in right now.
+  // Unlike Notify(), does *not* take into account future notifications.
+  // This is only useful if you do not have an imgRequest, e.g., if you are a
+  // static request returned from imgIRequest::GetStaticRequest().
+  void NotifyCurrentState(imgRequestProxy* proxy);
+
   // "Replay" all of the notifications that would have to happen to put us in
   // the state we're currently in.
-  void Notify(imgRequestProxy* proxy);
+  // Only use this if you're already servicing an asynchronous call (e.g.
+  // OnStartRequest).
+  void SyncNotify(imgRequestProxy* proxy);
 
   // Send all notifications that would be necessary to make |proxy| believe the
   // request is finished downloading and decoding.
@@ -135,6 +155,8 @@ public:
   void SendStopRequest(imgRequestProxy* aProxy, PRBool aLastPart, nsresult aStatus);
 
 private:
+  friend class imgStatusNotifyRunnable;
+
   // A weak pointer to the imgIContainer, because the container owns us, and we
   // can't create a cycle.
   imgIContainer* mImage;