Bug 1239822: Part 1 - Add a close method to windowless browsers, and only destroy when safe. r=bz

--HG--
extra : commitid : 77O0Cyp53O5
extra : rebase_source : 3a129236674f4aa868eafaec1a949c621cd9fae6

docshell/test/chrome/test_bug846906.xul

@@ -21,15 +21,21 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=846906
                                   .getService(Components.interfaces.nsIAppShellService);
   ok(appShellService, "Should be able to get app shell service");
 
-  var webNavigation = appShellService.createWindowlessBrowser();
-  ok(webNavigation, "Should be able to create windowless browser");
+  var windowlessBrowser = appShellService.createWindowlessBrowser();
+  ok(windowlessBrowser, "Should be able to create windowless browser");
 
-  var interfaceRequestor = webNavigation.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
+  ok(windowlessBrowser instanceof Components.interfaces.nsIWindowlessBrowser,
+     "Windowless browser should implement nsIWindowlessBrowser");
+
+  var webNavigation = windowlessBrowser.QueryInterface(Components.interfaces.nsIWebNavigation);
+  ok(webNavigation, "Windowless browser should implement nsIWebNavigation");
+
+  var interfaceRequestor = windowlessBrowser.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
   ok(interfaceRequestor, "Should be able to query interface requestor interface");
 
   var docShell = interfaceRequestor.getInterface(Components.interfaces.nsIDocShell);
   ok(docShell, "Should be able to get doc shell interface");
- 
+
   var document = webNavigation.document;
   ok(document, "Should be able to get document");
 
@@ -55,12 +61,30 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=846906
         is(rect.width, 1024);
         is(rect.height, 768);
 
+        windowlessBrowser.close();
+
+        // Once the browser is closed, nsIWebNavigation and
+        // nsIInterfaceRequestor methods should no longer be accessible.
+        try {
+          windowlessBrowser.getInterface(Components.interfaces.nsIDocShell);
+          ok(false);
+        } catch (e) {
+          is(e.result, Components.results.NS_ERROR_NULL_POINTER);
+        }
+
+        try {
+          windowlessBrowser.document;
+          ok(false);
+        } catch (e) {
+          is(e.result, Components.results.NS_ERROR_NULL_POINTER);
+        }
+
         SimpleTest.finish();
     };
     iframe.setAttribute("src", "http://mochi.test:8888/chrome/docshell/test/chrome/bug846906.html");
   };
   document.documentElement.appendChild(iframe);
-    
+
   ]]>
   </script>
 

xpfe/appshell/moz.build

@@ -9,6 +9,7 @@ MOCHITEST_CHROME_MANIFESTS += ['test/chrome.ini']
 XPIDL_SOURCES += [
     'nsIAppShellService.idl',
     'nsIPopupWindowManager.idl',
+    'nsIWindowlessBrowser.idl',
     'nsIWindowMediator.idl',
     'nsIWindowMediatorListener.idl',
     'nsIXULBrowserWindow.idl',

xpfe/appshell/nsAppShellService.cpp

@@ -34,10 +34,13 @@
 #include "nsIScriptContext.h"
 
 #include "nsAppShellService.h"
+#include "nsContentUtils.h"
+#include "nsThreadUtils.h"
 #include "nsISupportsPrimitives.h"
 #include "nsIChromeRegistry.h"
 #include "nsILoadContext.h"
 #include "nsIWebNavigation.h"
+#include "nsIWindowlessBrowser.h"
 
 #include "mozilla/Attributes.h"
 #include "mozilla/Preferences.h"
@@ -399,43 +402,100 @@ WebBrowserChrome2Stub::Blur()
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
-// This is the "stub" we return from CreateWindowlessBrowser - it exists
-// purely to keep a strong reference to the browser and the container to
-// prevent the container being collected while the stub remains alive.
-class WindowlessBrowserStub final : public nsIWebNavigation,
-                                    public nsIInterfaceRequestor
+class BrowserDestroyer final : public nsRunnable
 {
 public:
-  WindowlessBrowserStub(nsIWebBrowser *aBrowser, nsISupports *aContainer) {
-    mBrowser = aBrowser;
+  BrowserDestroyer(nsIWebBrowser *aBrowser, nsISupports *aContainer) :
+    mBrowser(aBrowser),
+    mContainer(aContainer)
+  {
+  }
+
+  NS_IMETHOD
+  Run() override
+  {
+    // Explicitly destroy the browser, in case this isn't the last reference.
+    nsCOMPtr<nsIBaseWindow> window = do_QueryInterface(mBrowser);
+    return window->Destroy();
+  }
+
+protected:
+  virtual ~BrowserDestroyer() {}
+
+private:
+  nsCOMPtr<nsIWebBrowser> mBrowser;
+  nsCOMPtr<nsISupports> mContainer;
+};
+
+// This is the "stub" we return from CreateWindowlessBrowser - it exists
+// to manage the lifetimes of the nsIWebBrowser and container window.
+// In particular, it keeps a strong reference to both, to prevent them from
+// being collected while this object remains alive, and ensures that they
+// aren't destroyed when it's not safe to run scripts.
+class WindowlessBrowser final : public nsIWindowlessBrowser,
+                                public nsIInterfaceRequestor
+{
+public:
+  WindowlessBrowser(nsIWebBrowser *aBrowser, nsISupports *aContainer) :
+    mBrowser(aBrowser),
+    mContainer(aContainer),
+    mClosed(false)
+  {
     mWebNavigation = do_QueryInterface(aBrowser);
     mInterfaceRequestor = do_QueryInterface(aBrowser);
-    mContainer = aContainer;
   }
   NS_DECL_ISUPPORTS
-  NS_FORWARD_NSIWEBNAVIGATION(mWebNavigation->)
-  NS_FORWARD_NSIINTERFACEREQUESTOR(mInterfaceRequestor->)
+  NS_FORWARD_SAFE_NSIWEBNAVIGATION(mWebNavigation)
+  NS_FORWARD_SAFE_NSIINTERFACEREQUESTOR(mInterfaceRequestor)
+
+  NS_IMETHOD
+  Close() override
+  {
+    NS_ENSURE_TRUE(!mClosed, NS_ERROR_UNEXPECTED);
+    NS_ASSERTION(nsContentUtils::IsSafeToRunScript(),
+                 "WindowlessBrowser::Close called when not safe to run scripts");
+
+    mClosed = true;
+
+    mWebNavigation = nullptr;
+    mInterfaceRequestor = nullptr;
+
+    nsCOMPtr<nsIBaseWindow> window = do_QueryInterface(mBrowser);
+    return window->Destroy();
+  }
+
+protected:
+  virtual ~WindowlessBrowser()
+  {
+    if (mClosed) {
+      return;
+    }
+
+    NS_WARNING("Windowless browser was not closed prior to destruction");
+
+    // The docshell destructor needs to dispatch events, and can only run
+    // when it's safe to run scripts. If this was triggered by GC, it may
+    // not always be safe to run scripts, in which cases we need to delay
+    // destruction until it is.
+    nsCOMPtr<nsIRunnable> runnable = new BrowserDestroyer(mBrowser, mContainer);
+    nsContentUtils::AddScriptRunner(runnable);
+  }
+
 private:
-  ~WindowlessBrowserStub() {}
   nsCOMPtr<nsIWebBrowser> mBrowser;
   nsCOMPtr<nsIWebNavigation> mWebNavigation;
   nsCOMPtr<nsIInterfaceRequestor> mInterfaceRequestor;
   // we don't use the container but just hold a reference to it.
   nsCOMPtr<nsISupports> mContainer;
+
+  bool mClosed;
 };
 
-NS_INTERFACE_MAP_BEGIN(WindowlessBrowserStub)
-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIWebNavigation)
-  NS_INTERFACE_MAP_ENTRY(nsIWebNavigation)
-  NS_INTERFACE_MAP_ENTRY(nsIInterfaceRequestor)
-NS_INTERFACE_MAP_END
-
-NS_IMPL_ADDREF(WindowlessBrowserStub)
-NS_IMPL_RELEASE(WindowlessBrowserStub)
+NS_IMPL_ISUPPORTS(WindowlessBrowser, nsIWindowlessBrowser, nsIWebNavigation, nsIInterfaceRequestor)
 
 
 NS_IMETHODIMP
-nsAppShellService::CreateWindowlessBrowser(bool aIsChrome, nsIWebNavigation **aResult)
+nsAppShellService::CreateWindowlessBrowser(bool aIsChrome, nsIWindowlessBrowser **aResult)
 {
   /* First, we create an instance of nsWebBrowser. Instances of this class have
    * an associated doc shell, which is what we're interested in.
@@ -480,7 +540,7 @@ nsAppShellService::CreateWindowlessBrowser(bool aIsChrome, nsIWebNavigation **aR
   window->Create();
 
   nsISupports *isstub = NS_ISUPPORTS_CAST(nsIWebBrowserChrome2*, stub);
-  RefPtr<nsIWebNavigation> result = new WindowlessBrowserStub(browser, isstub);
+  RefPtr<nsIWindowlessBrowser> result = new WindowlessBrowser(browser, isstub);
   nsCOMPtr<nsIDocShell> docshell = do_GetInterface(result);
   docshell->SetInvisible(true);
 

xpfe/appshell/nsIAppShellService.idl

@@ -6,7 +6,7 @@
 #include "nsISupports.idl"
 
 interface nsIXULWindow;
-interface nsIWebNavigation;
+interface nsIWindowlessBrowser;
 interface nsIURI;
 interface nsIDOMWindow;
 interface nsIAppShell;
@@ -18,7 +18,7 @@ interface nsITabParent;
 #include "js/TypeDecls.h"
 %}
 
-[scriptable, uuid(83f23c7e-6ce0-433f-9fe2-f287ae8c6e0c)]
+[scriptable, uuid(0cc1c790-6873-4b31-944f-71d3725e373d)]
 interface nsIAppShellService : nsISupports
 {
   /**
@@ -53,7 +53,7 @@ interface nsIAppShellService : nsISupports
    * representation.
    * @param aIsChrome Set true if you want to use it for chrome content.
    */
-  nsIWebNavigation createWindowlessBrowser([optional] in bool aIsChrome);
+  nsIWindowlessBrowser createWindowlessBrowser([optional] in bool aIsChrome);
 
   [noscript]
   void createHiddenWindow();

xpfe/appshell/nsIWindowlessBrowser.idl

@@ -0,0 +1,27 @@
+/* -*- Mode: IDL; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*-
+ *
+ * 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/. */
+
+#include "nsIWebNavigation.idl"
+
+/**
+ * This interface represents a nsIWebBrowser instance with no associated OS
+ * window. Its main function is to manage the lifetimes of those windows.
+ * A strong reference to this object must be held until the window is
+ * ready to be destroyed.
+ */
+[scriptable, uuid(abb46f48-abfc-41bf-aa9a-7feccefcf977)]
+interface nsIWindowlessBrowser : nsIWebNavigation
+{
+  /**
+   * "Closes" the windowless browser and destroys its associated nsIWebBrowser
+   * and docshell.
+   *
+   * This method *must* be called for every windowless browser before its last
+   * reference is released.
+   */
+  void close();
+};
+