Bug 577939 - Port Bug 519216 [Removing a progress listener while it's being called affects subsequent listeners] and followup bug 577320 to SeaMonkey. r,sr=Neil
☠☠ backed out by b168750e523a ☠ ☠
authorMisak Khachatryan <misak.bugzilla@gmail.com>
Mon, 16 Aug 2010 02:42:21 +0500
changeset 6165 7d97c516fbe25153f55651a55f27e30d35720754
parent 6164 0bb7e5e21c184223aec1245997f4692b5fba773c
child 6166 b168750e523a3c20e34759663e748047eb171b12
push idunknown
push userunknown
push dateunknown
reviewersNeil
bugs577939, 519216, 577320
Bug 577939 - Port Bug 519216 [Removing a progress listener while it's being called affects subsequent listeners] and followup bug 577320 to SeaMonkey. r,sr=Neil
suite/browser/tabbrowser.xml
suite/browser/test/Makefile.in
suite/browser/test/browser/browser_alltabslistener.js
suite/browser/test/browser/browser_bug519216.js
suite/common/tests/browser/browser_522545.js
--- a/suite/browser/tabbrowser.xml
+++ b/suite/browser/tabbrowser.xml
@@ -353,69 +353,87 @@
         <parameter name="aBrowser"/>
         <body>
           <![CDATA[
             return aBrowser ? aBrowser.parentNode : this.mCurrentBrowser.parentNode;
           ]]>
         </body>
       </method>
 
+      <method name="_callProgressListeners">
+        <parameter name="aBrowser"/>
+        <parameter name="aMethod"/>
+        <parameter name="aArguments"/>
+        <parameter name="aCallGlobalListeners"/>
+        <parameter name="aCallTabsListeners"/>
+        <body><![CDATA[
+          if (!aBrowser)
+            aBrowser = this.mCurrentBrowser;
+
+          if (aCallGlobalListeners != false &&
+              aBrowser == this.mCurrentBrowser) {
+            this.mProgressListeners.forEach(function (p) {
+              if (aMethod in p) {
+                try {
+                  p[aMethod].apply(p, aArguments);
+                } catch (e) {
+                  // don't inhibit other listeners
+                  Components.utils.reportError(e);
+                }
+              }
+            });
+          }
+
+          if (aCallTabsListeners != false) {
+            aArguments.unshift(aBrowser);
+
+            this.mTabsProgressListeners.forEach(function (p) {
+              if (aMethod in p) {
+                try {
+                  p[aMethod].apply(p, aArguments);
+                } catch (e) {
+                  // don't inhibit other listeners
+                  Components.utils.reportError(e);
+                }
+              }
+            });
+          }
+        ]]></body>
+      </method>
+
       <!-- A web progress listener object definition for a given tab. -->
       <method name="mTabProgressListener">
         <parameter name="aTab"/>
         <parameter name="aBrowser"/>
         <parameter name="aStartsBlank"/>
         <body>
         <![CDATA[
           return ({
             mTabBrowser: this,
             mTab: aTab,
             mBrowser: aBrowser,
             mBlank: aStartsBlank,
             mFeeds: [],
 
-            onProgressChange : function (aWebProgress, aRequest,
-                                         aCurSelfProgress, aMaxSelfProgress,
-                                         aCurTotalProgress, aMaxTotalProgress)
-            {
+            onProgressChange: function (aWebProgress, aRequest,
+                                        aCurSelfProgress, aMaxSelfProgress,
+                                        aCurTotalProgress, aMaxTotalProgress) {
               if (aMaxTotalProgress > 0)
                 this.mTab.setAttribute("progress", Math.floor(aCurTotalProgress * 9.9 / aMaxTotalProgress));
 
               if (this.mBlank)
                 return;
 
-              if (this.mTabBrowser.mCurrentTab == this.mTab) {
-                this.mTabBrowser.mProgressListeners.forEach(
-                  function notifyProgressChange(element) {
-                    try {
-                      element.onProgressChange(aWebProgress, aRequest,
-                                               aCurSelfProgress, aMaxSelfProgress,
-                                               aCurTotalProgress, aMaxTotalProgress);
-                    } catch (e) {
-                      Components.utils.reportError(e);
-                    }
-                  }
-                );
-              }
-
-              this.mTabBrowser.mTabsProgressListeners.forEach(
-                function notifyProgressChange(element) {
-                  try {
-                    element.onProgressChange(this.mBrowser, aWebProgress, aRequest,
-                                             aCurSelfProgress, aMaxSelfProgress,
-                                             aCurTotalProgress, aMaxTotalProgress);
-                  } catch (e) {
-                    Components.utils.reportError(e);
-                  }
-                }
-              , this);
+              this.mTabBrowser._callProgressListeners(this.mBrowser, "onProgressChange",
+                                                      [aWebProgress, aRequest,
+                                                      aCurSelfProgress, aMaxSelfProgress,
+                                                      aCurTotalProgress, aMaxTotalProgress]);
             },
 
-            onStateChange : function(aWebProgress, aRequest, aStateFlags, aStatus)
-            {
+            onStateChange: function (aWebProgress, aRequest, aStateFlags, aStatus) {
               if (!aRequest)
                 return;
 
               var oldBlank = this.mBlank;
 
               const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
               const nsIChannel = Components.interfaces.nsIChannel;
 
@@ -490,47 +508,25 @@
 
                 if (this.mTabBrowser.mCurrentTab == this.mTab)
                   this.mTabBrowser.mIsBusy = false;
               }
 
               if (oldBlank)
                 return;
 
-              if (this.mTabBrowser.mCurrentTab == this.mTab) {
-                this.mTabBrowser.mProgressListeners.forEach(
-                  function notifyStateChange(element) {
-                    try {
-                      element.onStateChange(aWebProgress, aRequest,
-                                            aStateFlags, aStatus);
-                    } catch (e) {
-                      Components.utils.reportError(e);
-                    }
-                  }
-                );
-              }
-
-              this.mTabBrowser.mTabsProgressListeners.forEach(
-                function notifyStateChange(element) {
-                  try {
-                    element.onStateChange(this.mBrowser, aWebProgress,
-                                          aRequest, aStateFlags, aStatus);
-                  } catch (e) {
-                    Components.utils.reportError(e);
-                  }
-                }
-              , this);
+              this.mTabBrowser._callProgressListeners(this.mBrowser, "onStateChange",
+                                                      [aWebProgress, aRequest, aStateFlags, aStatus]);
             },
 
             // The first location change is gotoIndex called from mInstallSH,
             // the second one is considered a user action.
             mLocationChangeCount : 0,
 
-            onLocationChange : function(aWebProgress, aRequest, aLocation)
-            {
+            onLocationChange: function(aWebProgress, aRequest, aLocation) {
               if (aRequest && aWebProgress.DOMWindow == this.mBrowser.contentWindow) {
                 this.mBrowser.mIconURL = "";
                 this.mFeeds = [];
 
                 if (this.mLocationChangeCount > 0 ||
                     aLocation.spec != "about:blank")
                   ++this.mLocationChangeCount;
 
@@ -546,83 +542,35 @@
 
               if (this.mBlank)
                 return;
 
               if (this.mTabBrowser.mCurrentTab == this.mTab)
                 this.mTabBrowser.updateUrlBar(aWebProgress, aRequest, aLocation,
                                               null, this.mBrowser, this.mFeeds);
 
-              this.mTabBrowser.mTabsProgressListeners.forEach(
-                function notifyLocationChange(element) {
-                  try {
-                    element.onLocationChange(this.mBrowser, aWebProgress, aRequest, aLocation);
-                  } catch (e) {
-                    Components.utils.reportError(e);
-                  }
-                }
-              , this);
+              this.mTabBrowser._callProgressListeners(this.mBrowser, "onLocationChange",
+                                                      [aWebProgress, aRequest, aLocation],
+                                                      false);
             },
 
-            onStatusChange : function(aWebProgress, aRequest, aStatus, aMessage)
-            {
+            onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) {
               if (this.mBlank)
                 return;
 
-              if (this.mTabBrowser.mCurrentTab == this.mTab) {
-                this.mTabBrowser.mProgressListeners.forEach(
-                  function notifyStatusChange(element) {
-                    try {
-                      element.onStatusChange(aWebProgress, aRequest,
-                                             aStatus, aMessage);
-                    } catch (e) {
-                      Components.utils.reportError(e);
-                    }
-                  }
-                );
-              }
-
-              this.mTabBrowser.mTabsProgressListeners.forEach(
-                function notifyStatusChange(element) {
-                  try {
-                    element.onStatusChange(this.mBrowser, aWebProgress, aRequest,
-                                           aStatus, aMessage);
-                  } catch (e) {
-                    Components.utils.reportError(e);
-                  }
-                }
-              , this);
+              this.mTabBrowser._callProgressListeners(this.mBrowser, "onStatusChange",
+                                                      [aWebProgress, aRequest, aStatus, aMessage]);
             },
 
-            onSecurityChange : function(aWebProgress, aRequest, aState)
-            {
-              if (this.mTabBrowser.mCurrentTab == this.mTab) {
-                this.mTabBrowser.mProgressListeners.forEach(
-                  function notifySecurityChange(element) {
-                    try {
-                      element.onSecurityChange(aWebProgress, aRequest, aState);
-                    } catch (e) {
-                      Components.utils.reportError(e);
-                    }
-                  }
-                );
-              }
-
-              this.mTabBrowser.mTabsProgressListeners.forEach(
-                function notifySecurityChange(element) {
-                  try {
-                    element.onSecurityChange(this.mBrowser, aWebProgress, aRequest, aState);
-                  } catch (e) {
-                    Components.utils.reportError(e);
-                  }
-                }
-              , this);
+            onSecurityChange: function (aWebProgress, aRequest, aState) {
+              this.mTabBrowser._callProgressListeners(this.mBrowser, "onSecurityChange",
+                                                      [aWebProgress, aRequest, aState]);
             },
 
-            onRefreshAttempted : function(aWebProgress, aURI, aDelay, aSameURI)
+            onRefreshAttempted: function(aWebProgress, aURI, aDelay, aSameURI)
             {
               var allowRefresh = true;
               if (this.mTabBrowser.mCurrentTab == this.mTab) {
                 this.mTabBrowser.mProgressListeners.forEach(
                   function notifyRefreshAttempted(element) {
                     if (element && "onRefreshAttempted" in element) {
                       try {
                         if (!element.onRefreshAttempted(aWebProgress, aURI, aDelay, aSameURI))
@@ -645,23 +593,22 @@
                       Components.utils.reportError(e);
                     }
                   }
                 }
               , this);
               return allowRefresh;
             },
 
-            addFeed : function(aLink)
+            addFeed: function(aLink)
             {
               this.mFeeds.push(aLink);
             },
 
-            QueryInterface : function(aIID)
-            {
+            QueryInterface: function(aIID) {
               if (aIID.equals(Components.interfaces.nsIWebProgressListener) ||
                   aIID.equals(Components.interfaces.nsIWebProgressListener2) ||
                   aIID.equals(Components.interfaces.nsISupportsWeakReference) ||
                   aIID.equals(Components.interfaces.nsISupports))
                 return this;
               throw Components.results.NS_NOINTERFACE;
             }
           });
@@ -737,41 +684,18 @@
 
             if (aURI && this.mFaviconService) {
               this.mFaviconService.setAndLoadFaviconForPage(browser.currentURI,
                                                             aURI, false);
             }
 
             this.updateIcon(aTab);
 
-            if (browser == this.mCurrentBrowser) {
-              this.mProgressListeners.forEach(
-                function notifyLinkIconAvailable(p) {
-                  if ("onLinkIconAvailable" in p)
-                    try {
-                      p.onLinkIconAvailable(browser.mIconURL);
-                    } catch (e) {
-                      // don't inhibit other listeners
-                      Components.utils.reportError(e);
-                    }
-                }
-              );
-            }
-            this.mTabsProgressListeners.forEach(
-              function notifyLinkIconAvailable(p) {
-                if ("onLinkIconAvailable" in p)
-                  try {
-                    // tab listener needs browser as first argument
-                    p.onLinkIconAvailable(browser, browser.mIconURL);
-                  } catch (e) {
-                    // don't inhibit other listeners
-                    Components.utils.reportError(e);
-                  }
-              }
-            );
+            this._callProgressListeners(browser, "onLinkIconAvailable",
+                                        [browser.mIconURL]);
           ]]>
         </body>
       </method>
 
       <method name="getIcon">
         <parameter name="aTab"/>
         <body>
           <![CDATA[
@@ -1093,49 +1017,33 @@
             this.fastFind.setDocShell(this.mCurrentBrowser.docShell);
 
             // If the new tab is busy, and our current state is not busy, then
             // we need to fire a start to all progress listeners.
             const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
             if (this.mCurrentTab.hasAttribute("busy") && !this.mIsBusy) {
               this.mIsBusy = true;
 
-              var webProgress = this.mCurrentBrowser.webProgress;
-              this.mProgressListeners.forEach(
-                function notifyStateChangeBusy(element) {
-                  try {
-                    element.onStateChange(webProgress, null,
-                                          nsIWebProgressListener.STATE_START |
-                                            nsIWebProgressListener.STATE_IS_NETWORK,
-                                          0);
-                  } catch (e) {
-                    Components.utils.reportError(e);
-                  }
-                }
-              );
+              this._callProgressListeners(null, "onStateChange",
+                                          [this.mCurrentBrowser.webProgress, null,
+                                           nsIWebProgressListener.STATE_START |
+                                           nsIWebProgressListener.STATE_IS_NETWORK, 0],
+                                          true, false);
             }
 
             // If the new tab is not busy, and our current state is busy, then
             // we need to fire a stop to all progress listeners.
             if (!this.mCurrentTab.hasAttribute("busy") && this.mIsBusy) {
               this.mIsBusy = false;
 
-              webProgress = this.mCurrentBrowser.webProgress;
-              this.mProgressListeners.forEach(
-                function notifyStateChangeNotBusy(element) {
-                  try {
-                    element.onStateChange(webProgress, null,
-                                          nsIWebProgressListener.STATE_STOP |
-                                            nsIWebProgressListener.STATE_IS_NETWORK,
-                                          0);
-                  } catch (e) {
-                    Components.utils.reportError(e);
-                  }
-                }
-              );
+              this._callProgressListeners(null, "onStateChange",
+                                          [this.mCurrentBrowser.webProgress, null,
+                                           nsIWebProgressListener.STATE_STOP |
+                                           nsIWebProgressListener.STATE_IS_NETWORK, 0],
+                                          true, false);
             }
 
             // We've selected the new tab, so go ahead and notify listeners
             var event = document.createEvent("Events");
             event.initEvent("TabSelect", true, false);
             this.mCurrentTab.dispatchEvent(event);
 
             if (document.commandDispatcher.focusedElement && 
--- a/suite/browser/test/Makefile.in
+++ b/suite/browser/test/Makefile.in
@@ -68,16 +68,17 @@ include $(topsrcdir)/config/rules.mk
                  plugin_test.html \
                  plugin_both.html \
                  plugin_both2.html \
                  browser_scope.js \
                  browser_alltabslistener.js \
                  alltabslistener.html \
                  browser_relatedTabs.js \
                  browser_selectTabAtIndex.js \
+                 browser_bug519216.js \
     $(NULL)
 
 ifneq (cocoa,$(MOZ_WIDGET_TOOLKIT))
 _BROWSER_FILES += browser_bug462289.js
 endif
 
 libs:: $(addprefix mochitest/, $(_TEST_FILES))
 	$(INSTALL) $(foreach f,$^,"$f") $(MOZDEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
--- a/suite/browser/test/browser/browser_alltabslistener.js
+++ b/suite/browser/test/browser/browser_alltabslistener.js
@@ -33,21 +33,16 @@ var gFrontProgressListener = {
     info("FrontProgress: " + state + " 0x" + aState.toString(16));
     ok(gFrontNotificationsPos < gFrontNotifications.length, "Got an expected notification for the front notifications listener");
     is(state, gFrontNotifications[gFrontNotificationsPos], "Got a notification for the front notifications listener");
     gFrontNotificationsPos++;
   }
 }
 
 var gAllProgressListener = {
-  onProgressChange: function (aBrowser, aWebProgress, aRequest,
-                              aCurSelfProgress, aMaxSelfProgress,
-                              aCurTotalProgress, aMaxTotalProgress) {
-  },
-
   onStateChange: function (aBrowser, aWebProgress, aRequest, aStateFlags, aStatus) {
     var state = "onStateChange";
     info("AllProgress: " + state + " 0x" + aStateFlags.toString(16));
     is(aBrowser, gTestBrowser, state + " notification came from the correct browser");
     ok(gAllNotificationsPos < gAllNotifications.length, "Got an expected notification for the all notifications listener");
     is(state, gAllNotifications[gAllNotificationsPos], "Got a notification for the all notifications listener");
     gAllNotificationsPos++;
 
new file mode 100644
--- /dev/null
+++ b/suite/browser/test/browser/browser_bug519216.js
@@ -0,0 +1,48 @@
+function test() {
+  waitForExplicitFinish();
+  gBrowser.stop();
+  gBrowser.addProgressListener(progressListener1);
+  gBrowser.addProgressListener(progressListener2);
+  gBrowser.addProgressListener(progressListener3);
+  gBrowser.loadURI("data:text/plain,bug519216");
+}
+
+var calledListener1 = false;
+var progressListener1 = {
+  onLocationChange: function onLocationChange() {
+    calledListener1 = true;
+    gBrowser.removeProgressListener(this);
+  }
+};
+
+var calledListener2 = false;
+var progressListener2 = {
+  onLocationChange: function onLocationChange() {
+    ok(calledListener1, "called progressListener1 before progressListener2");
+    calledListener2 = true;
+    gBrowser.removeProgressListener(this);
+  }
+};
+
+var progressListener3 = {
+  onLocationChange: function onLocationChange() {
+    ok(calledListener2, "called progressListener2 before progressListener3");
+    gBrowser.removeProgressListener(this);
+    gBrowser.addProgressListener(progressListener4);
+    executeSoon(function () {
+      expectListener4 = true;
+      gBrowser.reload();
+    });
+  }
+};
+
+var expectListener4 = false;
+var progressListener4 = {
+  onLocationChange: function onLocationChange() {
+    ok(expectListener4, "didn't call progressListener4 for the first location change");
+    gBrowser.removeProgressListener(this);
+    gBrowser.addTab();
+    gBrowser.removeCurrentTab();
+    finish();
+  }
+};
--- a/suite/common/tests/browser/browser_522545.js
+++ b/suite/common/tests/browser/browser_522545.js
@@ -57,21 +57,17 @@ function test() {
   function waitForBrowserState(aState, aSetStateCallback) {
     var locationChanges = 0;
     getBrowser().addTabsProgressListener({
       onLocationChange: function (aBrowser) {
         if (++locationChanges == aState.windows[0].tabs.length) {
           getBrowser().removeTabsProgressListener(this);
           executeSoon(aSetStateCallback);
         }
-      },
-      onProgressChange: function () {},
-      onSecurityChange: function () {},
-      onStateChange: function () {},
-      onStatusChange: function () {}
+      }
     });
     ss.setBrowserState(JSON.stringify(aState));
   }
 
   // This tests the following use case:
   // User opens a new tab which gets focus. The user types something into the
   // address bar, then crashes or quits.
   function test_newTabFocused() {
@@ -206,21 +202,17 @@ function test() {
     // be in a non-userTypedValue case, while others should still have
     // userTypedValue and userTypedClear set.
     getBrowser().addTabsProgressListener({
       onLocationChange: function (aBrowser) {
         if (uris.indexOf(aBrowser.currentURI.spec) > -1) {
           getBrowser().removeTabsProgressListener(this);
           firstLocationChange();
         }
-      },
-      onProgressChange: function () {},
-      onSecurityChange: function () {},
-      onStateChange: function () {},
-      onStatusChange: function () {}
+      }
     });
 
     function firstLocationChange() {
       let state = JSON.parse(ss.getBrowserState());
       let hasUTV = state.windows[0].tabs.some(function(aTab) {
         return aTab.userTypedValue && aTab.userTypedClear && !aTab.entries.length;
       });