Make nsRefreshDriver hold a reference to its observers while notifying them. (Bug 531585) r=bzbarsky
☠☠ backed out by f32e7f33b015 ☠ ☠
authorL. David Baron <dbaron@dbaron.org>
Mon, 21 Dec 2009 16:46:25 -0500
changeset 36523 cfa10b01b1f657b7ac20ccaef5a666ffc334881c
parent 36522 8b22441911b036039ea3927cb2859ac4d67ae8b5
child 36524 833f5ebf46519109cc3168afe0e14851f05d6b9c
child 36566 f32e7f33b01530d2dba7594c3899a1922da92f3c
push idunknown
push userunknown
push dateunknown
reviewersbzbarsky
bugs531585
milestone1.9.3a1pre
Make nsRefreshDriver hold a reference to its observers while notifying them. (Bug 531585) r=bzbarsky
layout/base/nsRefreshDriver.cpp
layout/base/nsRefreshDriver.h
--- a/layout/base/nsRefreshDriver.cpp
+++ b/layout/base/nsRefreshDriver.cpp
@@ -39,16 +39,17 @@
  * Code to notify things that animate before a refresh, at an appropriate
  * refresh rate.  (Perhaps temporary, until replaced by compositor.)
  */
 
 #include "nsRefreshDriver.h"
 #include "nsPresContext.h"
 #include "nsComponentManagerUtils.h"
 #include "prlog.h"
+#include "nsAutoPtr.h"
 
 /*
  * TODO:
  * Once this is hooked in to suppressing updates when the presentation
  * is not visible, we need to hook it up to FlushPendingNotifications so
  * that we flush when necessary.
  */
 
@@ -189,20 +190,32 @@ nsRefreshDriver::Notify(nsITimer *aTimer
   }
   nsCOMPtr<nsIPresShell> presShell = mPresContext->GetPresShell();
   if (!presShell) {
     // Things are being destroyed.
     StopTimer();
     return NS_OK;
   }
 
+  /*
+   * The timer holds a reference to |this| while calling |Notify|.
+   * However, implementations of |WillRefresh| are permitted to destroy
+   * the pres context, which will cause our |mPresContext| to become
+   * null.  If this happens, we must stop notifying observers.
+   */
   for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(mObservers); ++i) {
     ObserverArray::EndLimitedIterator etor(mObservers[i]);
     while (etor.HasMore()) {
-      etor.GetNext()->WillRefresh(mMostRecentRefresh);
+      nsRefPtr<nsARefreshObserver> obs = etor.GetNext();
+      obs->WillRefresh(mMostRecentRefresh);
+      
+      if (!mPresContext || !mPresContext->GetPresShell()) {
+        StopTimer();
+        return NS_OK;
+      }
     }
     if (i == 0) {
       // This is the Flush_Style case.
       // FIXME: Maybe we should only flush if the WillRefresh calls did
       // something?  It's probably ok as-is, though, especially as we
       // hook up more things here (or to the replacement of this class).
       // FIXME: We should probably flush for other sets of observers
       // too.  But we should only flush layout once nsRefreshDriver is
--- a/layout/base/nsRefreshDriver.h
+++ b/layout/base/nsRefreshDriver.h
@@ -53,16 +53,25 @@ class nsPresContext;
 
 /**
  * An abstract base class to be implemented by callers wanting to be
  * notified at refresh times.  When nothing needs to be painted, callers
  * may not be notified.
  */
 class nsARefreshObserver {
 public:
+  // AddRef and Release signatures that match nsISupports.  Implementors
+  // must implement reference counting, and those that do implement
+  // nsISupports will already have methods with the correct signature.
+  //
+  // The refresh driver does NOT hold references to refresh observers
+  // except while it is notifying them.  
+  NS_IMETHOD_(nsrefcnt) AddRef(void) = 0;
+  NS_IMETHOD_(nsrefcnt) Release(void) = 0;
+
   virtual void WillRefresh(mozilla::TimeStamp aTime) = 0;
 };
 
 class nsRefreshDriver : public nsITimerCallback {
 public:
   nsRefreshDriver(nsPresContext *aPresContext);
   ~nsRefreshDriver();
 
@@ -88,16 +97,19 @@ public:
    * The flush type affects:
    *   + the order in which the observers are notified (lowest flush
    *     type to highest, in order registered)
    *   + (in the future) which observers are suppressed when the display
    *     doesn't require current position data or isn't currently
    *     painting, and, correspondingly, which get notified when there
    *     is a flush during such suppression
    * and it must be either Flush_Style, Flush_Layout, or Flush_Display.
+   *
+   * The refresh driver does NOT own a reference to these observers;
+   * they must remove themselves before they are destroyed.
    */
   PRBool AddRefreshObserver(nsARefreshObserver *aObserver,
                             mozFlushType aFlushType);
   PRBool RemoveRefreshObserver(nsARefreshObserver *aObserver,
                                mozFlushType aFlushType);
 
   /**
    * Tell the refresh driver that it is done driving refreshes and