Bug 832943 keep explicit reference to our error handling web listeners to avoid unexpected gc, r=felipe

browser/base/content/browser-social.js

@@ -129,7 +129,7 @@ let SocialUI = {
           break;
         case "social:frameworker-error":
           if (this.enabled && Social.provider.origin == data) {
-            SocialSidebar.setSidebarErrorMessage("frameworker-error");
+            SocialSidebar.setSidebarErrorMessage();
           }
           break;
 
@@ -452,20 +452,6 @@ let SocialFlyout = {
     panel.appendChild(iframe);
   },
 
-  setUpProgressListener: function SF_setUpProgressListener() {
-    if (!this._progressListenerSet) {
-      this._progressListenerSet = true;
-      // Force a layout flush by calling .clientTop so
-      // that the docShell of this frame is created
-      this.panel.firstChild.clientTop;
-      this.panel.firstChild.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
-                                    .getInterface(Ci.nsIWebProgress)
-                                    .addProgressListener(new SocialErrorListener("flyout"),
-                                                         Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
-                                                         Ci.nsIWebProgress.NOTIFY_LOCATION);
-    }
-  },
-
   setFlyoutErrorMessage: function SF_setFlyoutErrorMessage() {
     let iframe = this.panel.firstChild;
     if (!iframe)
@@ -481,7 +467,10 @@ let SocialFlyout = {
     panel.hidePopup();
     if (!panel.firstChild)
       return
-    panel.removeChild(panel.firstChild);
+    let iframe = panel.firstChild;
+    if (iframe.socialErrorListener)
+      iframe.socialErrorListener.remove();
+    panel.removeChild(iframe);
   },
 
   onShown: function(aEvent) {
@@ -560,7 +549,10 @@ let SocialFlyout = {
       panel.moveTo(box.screenX, box.screenY + yAdjust);
     } else {
       panel.openPopup(anchor, "start_before", 0, yOffset, false, false);
-      this.setUpProgressListener();
+      // Force a layout flush by calling .clientTop so
+      // that the docShell of this frame is created
+      panel.firstChild.clientTop;
+      Social.setErrorListener(iframe, this.setFlyoutErrorMessage.bind(this))
     }
     this.yOffset = yOffset;
   }
@@ -946,13 +938,12 @@ var SocialToolbar = {
     socialToolbarItem.appendChild(toolbarButtons);
 
     for (let frame of createdFrames) {
+      if (frame.socialErrorListener) {
+        frame.socialErrorListener.remove();
+      }
       if (frame.docShell) {
         frame.docShell.isActive = false;
-        frame.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
-                      .getInterface(Ci.nsIWebProgress)
-                      .addProgressListener(new SocialErrorListener("notification-panel"),
-                                           Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
-                                           Ci.nsIWebProgress.NOTIFY_LOCATION);
+        Social.setErrorListener(frame, this.setPanelErrorMessage.bind(this));
       }
     }
   },
@@ -1077,18 +1068,10 @@ var SocialSidebar = {
   // Called once, after window load, when the Social.provider object is initialized
   init: function SocialSidebar_init() {
     let sbrowser = document.getElementById("social-sidebar-browser");
-    this.errorListener = new SocialErrorListener("sidebar");
-    this.configureSidebarDocShell(sbrowser.docShell);
-    this.update();
-  },
-
-  configureSidebarDocShell: function SocialSidebar_configureDocShell(aDocShell) {
+    Social.setErrorListener(sbrowser, this.setSidebarErrorMessage.bind(this));
     // setting isAppTab causes clicks on untargeted links to open new tabs
-    aDocShell.isAppTab = true;
-    aDocShell.QueryInterface(Ci.nsIWebProgress)
-             .addProgressListener(SocialSidebar.errorListener,
-                                  Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
-                                  Ci.nsIWebProgress.NOTIFY_LOCATION);
+    sbrowser.docShell.isAppTab = true;
+    this.update();
   },
 
   // Whether the sidebar can be shown for this window.
@@ -1145,7 +1128,7 @@ var SocialSidebar = {
     } else {
       sbrowser.setAttribute("origin", Social.provider.origin);
       if (Social.provider.errorState == "frameworker-error") {
-        SocialSidebar.setSidebarErrorMessage("frameworker-error");
+        SocialSidebar.setSidebarErrorMessage();
         return;
       }
 
@@ -1178,84 +1161,14 @@ var SocialSidebar = {
 
   _unloadTimeoutId: 0,
 
-  setSidebarErrorMessage: function(aType) {
+  setSidebarErrorMessage: function() {
     let sbrowser = document.getElementById("social-sidebar-browser");
-    switch (aType) {
-      case "sidebar-error":
-        let url = encodeURIComponent(Social.provider.sidebarURL);
-        sbrowser.loadURI("about:socialerror?mode=tryAgain&url=" + url, null, null);
-        break;
-
-      case "frameworker-error":
-        sbrowser.setAttribute("src", "about:socialerror?mode=workerFailure");
-        break;
+    // a frameworker error "trumps" a sidebar error.
+    if (Social.provider.errorState == "frameworker-error") {
+      sbrowser.setAttribute("src", "about:socialerror?mode=workerFailure");
+    } else {
+      let url = encodeURIComponent(Social.provider.sidebarURL);
+      sbrowser.loadURI("about:socialerror?mode=tryAgain&url=" + url, null, null);
     }
   }
 }
-
-// Error handling class used to listen for network errors in the social frames
-// and replace them with a social-specific error page
-function SocialErrorListener(aType) {
-  this.type = aType;
-}
-
-SocialErrorListener.prototype = {
-  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
-                                         Ci.nsISupportsWeakReference,
-                                         Ci.nsISupports]),
-
-  onStateChange: function SPL_onStateChange(aWebProgress, aRequest, aState, aStatus) {
-    let failure = false;
-    if ((aState & Ci.nsIWebProgressListener.STATE_STOP)) {
-      if (aRequest instanceof Ci.nsIHttpChannel) {
-        try {
-          // Change the frame to an error page on 4xx (client errors)
-          // and 5xx (server errors)
-          failure = aRequest.responseStatus >= 400 &&
-                    aRequest.responseStatus < 600;
-        } catch (e) {}
-      }
-    }
-
-    // Calling cancel() will raise some OnStateChange notifications by itself,
-    // so avoid doing that more than once
-    if (failure && aStatus != Components.results.NS_BINDING_ABORTED) {
-      aRequest.cancel(Components.results.NS_BINDING_ABORTED);
-      this.setErrorMessage(aWebProgress);
-    }
-  },
-
-  onLocationChange: function SPL_onLocationChange(aWebProgress, aRequest, aLocation, aFlags) {
-    let failure = aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE;
-    if (failure && Social.provider.errorState != "frameworker-error") {
-      aRequest.cancel(Components.results.NS_BINDING_ABORTED);
-      window.setTimeout(function(self) {
-        self.setErrorMessage(aWebProgress);
-      }, 0, this);
-    }
-  },
-
-  onProgressChange: function SPL_onProgressChange() {},
-  onStatusChange: function SPL_onStatusChange() {},
-  onSecurityChange: function SPL_onSecurityChange() {},
-
-  setErrorMessage: function(aWebProgress) {
-    switch (this.type) {
-      case "flyout":
-        SocialFlyout.setFlyoutErrorMessage();
-        break;
-
-      case "sidebar":
-        // a frameworker error "trumps" a sidebar error.
-        let reason = Social.provider.errorState || "sidebar-error";
-        SocialSidebar.setSidebarErrorMessage(reason);
-        break;
-
-      case "notification-panel":
-        let frame = aWebProgress.QueryInterface(Ci.nsIDocShell)
-                                .chromeEventHandler;
-        SocialToolbar.setPanelErrorMessage(frame);
-        break;
-    }
-  }
-};

browser/base/content/socialchat.xml

@@ -355,6 +355,7 @@
           if (this.selectedChat == aChatbox) {
             this._selectAnotherChat();
           }
+          aChatbox.iframe.socialErrorListener.remove();
           this.removeChild(aChatbox);
           // child might have been collapsed.
           let menuitem = this.menuitemMap.get(aChatbox);
@@ -392,6 +393,11 @@
           // to null in DOMContentLoaded, so null means DOMContentLoaded has
           // already fired and new callbacks can be made immediately.
           aChatBox._callbacks = [aCallback];
+          var tmp = {};
+          Components.utils.import("resource://gre/modules/Social.jsm", tmp);
+          tmp.Social.setErrorListener(aChatBox.iframe, function(iframe) {
+            iframe.webNavigation.loadURI("about:socialerror?mode=compactInfo", null, null, null, null);
+          });
           let iframeWindow = aChatBox.iframe.contentWindow;
           aChatBox.addEventListener("DOMContentLoaded", function DOMContentLoaded() {
             aChatBox.removeEventListener("DOMContentLoaded", DOMContentLoaded);

browser/base/content/test/social/Makefile.in

@@ -20,6 +20,7 @@ _BROWSER_FILES = \
                  browser_social_isVisible.js \
                  browser_social_chatwindow.js \
                  browser_social_multiprovider.js \
+                 browser_social_errorPage.js \
                  social_panel.html \
                  social_share_image.png \
                  social_sidebar.html \

browser/base/content/test/social/browser_social_errorPage.js

@@ -0,0 +1,164 @@
+/* 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/. */
+
+function gc() {
+  Cu.forceGC();
+  let wu =  window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
+                  .getInterface(Components.interfaces.nsIDOMWindowUtils);
+  wu.garbageCollect();
+}
+
+// Support for going on and offline.
+// (via browser/base/content/test/browser_bookmark_titles.js)
+let origProxyType = Services.prefs.getIntPref('network.proxy.type');
+
+function goOffline() {
+  // Simulate a network outage with offline mode. (Localhost is still
+  // accessible in offline mode, so disable the test proxy as well.)
+  if (!Services.io.offline)
+    BrowserOffline.toggleOfflineStatus();
+  Services.prefs.setIntPref('network.proxy.type', 0);
+  // LOAD_FLAGS_BYPASS_CACHE isn't good enough. So clear the cache.
+  Services.cache.evictEntries(Services.cache.STORE_ANYWHERE);
+}
+
+function goOnline(callback) {
+  Services.prefs.setIntPref('network.proxy.type', origProxyType);
+  if (Services.io.offline)
+    BrowserOffline.toggleOfflineStatus();
+  if (callback)
+    callback();
+}
+
+function openPanel(url, panelCallback, loadCallback) {
+  // open a flyout
+  SocialFlyout.open(url, 0, panelCallback);
+  SocialFlyout.panel.firstChild.addEventListener("load", function panelLoad() {
+    SocialFlyout.panel.firstChild.removeEventListener("load", panelLoad, true);
+    loadCallback();
+  }, true);
+}
+
+function openChat(url, panelCallback, loadCallback) {
+  // open a chat window
+  SocialChatBar.openChat(Social.provider, url, panelCallback);
+  SocialChatBar.chatbar.firstChild.addEventListener("DOMContentLoaded", function panelLoad() {
+    SocialChatBar.chatbar.firstChild.removeEventListener("DOMContentLoaded", panelLoad, true);
+    loadCallback();
+  }, true);
+}
+
+function onSidebarLoad(callback) {
+  let sbrowser = document.getElementById("social-sidebar-browser");
+  sbrowser.addEventListener("load", function load() {
+    sbrowser.removeEventListener("load", load, true);
+    callback();
+  }, true);
+}
+
+let manifest = { // normal provider
+  name: "provider 1",
+  origin: "https://example.com",
+  sidebarURL: "https://example.com/browser/browser/base/content/test/social/social_sidebar.html",
+  workerURL: "https://example.com/browser/browser/base/content/test/social/social_worker.js",
+  iconURL: "https://example.com/browser/browser/base/content/test/moz.png"
+};
+
+function test() {
+  waitForExplicitFinish();
+  // we don't want the sidebar to auto-load in these tests..
+  Services.prefs.setBoolPref("social.sidebar.open", false);
+  registerCleanupFunction(function() {
+    Services.prefs.clearUserPref("social.sidebar.open");
+  });
+
+  runSocialTestWithProvider(manifest, function (finishcb) {
+    runSocialTests(tests, undefined, goOnline, finishcb);
+  });
+}
+
+var tests = {
+  testSidebar: function(next) {
+    let sbrowser = document.getElementById("social-sidebar-browser");
+    onSidebarLoad(function() {
+      ok(sbrowser.contentDocument.location.href.indexOf("about:socialerror?")==0, "is on social error page");
+      gc();
+      // Add a new load listener, then find and click the "try again" button.
+      onSidebarLoad(function() {
+        // should still be on the error page.
+        ok(sbrowser.contentDocument.location.href.indexOf("about:socialerror?")==0, "is still on social error page");
+        // go online and try again - this should work.
+        goOnline();
+        onSidebarLoad(function() {
+          // should now be on the correct page.
+          is(sbrowser.contentDocument.location.href, manifest.sidebarURL, "is now on social sidebar page");
+          next();
+        });
+        sbrowser.contentDocument.getElementById("btnTryAgain").click();
+      });
+      sbrowser.contentDocument.getElementById("btnTryAgain").click();
+    });
+    // go offline then attempt to load the sidebar - it should fail.
+    goOffline();
+    Services.prefs.setBoolPref("social.sidebar.open", true);
+  },
+
+  testFlyout: function(next) {
+    let panelCallbackCount = 0;
+    let panel = document.getElementById("social-flyout-panel");
+    // go offline and open a flyout.
+    goOffline();
+    openPanel(
+      "https://example.com/browser/browser/base/content/test/social/social_panel.html",
+      function() { // the panel api callback
+        panelCallbackCount++;
+      },
+      function() { // the "load" callback.
+        executeSoon(function() {
+          todo_is(panelCallbackCount, 0, "Bug 833207 - should be no callback when error page loads.");
+          ok(panel.firstChild.contentDocument.location.href.indexOf("about:socialerror?")==0, "is on social error page");
+          // Bug 832943 - the listeners previously stopped working after a GC, so
+          // force a GC now and try again.
+          gc();
+          openPanel(
+            "https://example.com/browser/browser/base/content/test/social/social_panel.html",
+            function() { // the panel api callback
+              panelCallbackCount++;
+            },
+            function() { // the "load" callback.
+              executeSoon(function() {
+                todo_is(panelCallbackCount, 0, "Bug 833207 - should be no callback when error page loads.");
+                ok(panel.firstChild.contentDocument.location.href.indexOf("about:socialerror?")==0, "is on social error page");
+                gc();
+                SocialFlyout.unload();
+                next();
+              });
+            }
+          );
+        });
+      }
+    );
+  },
+
+  testChatWindow: function(next) {
+    let panelCallbackCount = 0;
+    // go offline and open a flyout.
+    goOffline();
+    openChat(
+      "https://example.com/browser/browser/base/content/test/social/social_chat.html",
+      function() { // the panel api callback
+        panelCallbackCount++;
+      },
+      function() { // the "load" callback.
+        executeSoon(function() {
+          todo_is(panelCallbackCount, 0, "Bug 833207 - should be no callback when error page loads.");
+          ok(SocialChatBar.chatbar.selectedChat.iframe.contentDocument.location.href.indexOf("about:socialerror?")==0, "is on social error page");
+          SocialChatBar.chatbar.selectedChat.close();
+          next();
+        });
+      }
+    );
+  }
+}
+

browser/modules/Social.jsm

@@ -284,9 +284,81 @@ this.Social = {
     port.close();
   },
 
-  _sharedUrls: {}
+  _sharedUrls: {},
+
+  setErrorListener: function(iframe, errorHandler) {
+    if (iframe.socialErrorListener)
+      return iframe.socialErrorListener;
+    return new SocialErrorListener(iframe, errorHandler);
+  }
 };
 
 function schedule(callback) {
   Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);
 }
+
+
+// Error handling class used to listen for network errors in the social frames
+// and replace them with a social-specific error page
+function SocialErrorListener(iframe, errorHandler) {
+  this.setErrorMessage = errorHandler;
+  this.iframe = iframe;
+  iframe.socialErrorListener = this;
+  iframe.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
+                                   .getInterface(Ci.nsIWebProgress)
+                                   .addProgressListener(this,
+                                                        Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
+                                                        Ci.nsIWebProgress.NOTIFY_LOCATION);
+}
+
+SocialErrorListener.prototype = {
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
+                                         Ci.nsISupportsWeakReference,
+                                         Ci.nsISupports]),
+
+  remove: function() {
+    this.iframe.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
+                                     .getInterface(Ci.nsIWebProgress)
+                                     .removeProgressListener(this);
+    delete this.iframe.socialErrorListener;
+  },
+
+  onStateChange: function SPL_onStateChange(aWebProgress, aRequest, aState, aStatus) {
+    let failure = false;
+    if ((aState & Ci.nsIWebProgressListener.STATE_STOP)) {
+      if (aRequest instanceof Ci.nsIHttpChannel) {
+        try {
+          // Change the frame to an error page on 4xx (client errors)
+          // and 5xx (server errors)
+          failure = aRequest.responseStatus >= 400 &&
+                    aRequest.responseStatus < 600;
+        } catch (e) {}
+      }
+    }
+
+    // Calling cancel() will raise some OnStateChange notifications by itself,
+    // so avoid doing that more than once
+    if (failure && aStatus != Components.results.NS_BINDING_ABORTED) {
+      aRequest.cancel(Components.results.NS_BINDING_ABORTED);
+      Social.provider.errorState = "content-error";
+      this.setErrorMessage(aWebProgress.QueryInterface(Ci.nsIDocShell)
+                              .chromeEventHandler);
+    }
+  },
+
+  onLocationChange: function SPL_onLocationChange(aWebProgress, aRequest, aLocation, aFlags) {
+    let failure = aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE;
+    if (failure && Social.provider.errorState != "frameworker-error") {
+      aRequest.cancel(Components.results.NS_BINDING_ABORTED);
+      Social.provider.errorState = "content-error";
+      schedule(function() {
+        this.setErrorMessage(aWebProgress.QueryInterface(Ci.nsIDocShell)
+                              .chromeEventHandler);
+      }.bind(this));
+    }
+  },
+
+  onProgressChange: function SPL_onProgressChange() {},
+  onStatusChange: function SPL_onStatusChange() {},
+  onSecurityChange: function SPL_onSecurityChange() {},
+};

toolkit/components/social/FrameWorker.jsm

@@ -220,7 +220,7 @@ FrameWorker.prototype = {
       }
       catch (e) {
         Cu.reportError("FrameWorker: Error setting up event listener for chrome side of the worker: " + e + "\n" + e.stack);
-        notifyWorkerError();
+        notifyWorkerError(worker);
         return;
       }