Bug 1471989 - Clear JSStackFrame's JS object pointer when the window goes away. r=bzbarsky

JSStackFrames are C++ objects that are exposed to chrome JS and keep
alive content JS. This means that if chrome JS leaks a stack frame
then a window can be leaked.

The basic idea of this patch is to think of JSStackFrames as
cross-compartment wrappers, and do a "hueyfix" on them by dropping the
content JS reference when the associated content window is closed.

To do that, this patch modifies the realm private to keep a list of
all live JSStackFrames that have been created with objects in that
realm. When we nuke that realm, we also clear out all of the JS
pointers from the registered stack frames on that realm.

This adds a hash table lookup to the JSStackFrame ctor and dtor, which
is hopefully not too much overhead.

The test works by intentionally leaking a JSStackFrame from chrome JS
and making sure that the window still goes away.

Differential Revision: https://phabricator.services.mozilla.com/D14880

--HG--
extra : moz-landing-system : lando

dom/base/WindowDestroyedEvent.cpp

@@ -109,6 +109,9 @@ WindowDestroyedEvent::Run() {
         JS::Rooted<JSObject*> obj(cx, currentInner->FastGetGlobalJSObject());
         if (obj && !js::IsSystemRealm(js::GetNonCCWObjectRealm(obj))) {
           JS::Realm* realm = js::GetNonCCWObjectRealm(obj);
+
+          xpc::NukeJSStackFrames(realm);
+
           nsCOMPtr<nsIPrincipal> pc =
               nsJSPrincipals::get(JS::GetRealmPrincipals(realm));
 

dom/bindings/Exceptions.cpp

@@ -10,6 +10,7 @@
 #include "js/TypeDecls.h"
 #include "jsapi.h"
 #include "js/SavedFrameAPI.h"
+#include "xpcpublic.h"
 #include "mozilla/CycleCollectedJSContext.h"
 #include "mozilla/dom/BindingUtils.h"
 #include "mozilla/dom/DOMException.h"
@@ -197,7 +198,7 @@ already_AddRefed<nsIStackFrame> GetCurrentJSStack(int32_t aMaxDepth) {
 
 namespace exceptions {
 
-class JSStackFrame : public nsIStackFrame {
+class JSStackFrame final : public nsIStackFrame, public xpc::JSStackFrameBase {
  public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(JSStackFrame)
@@ -209,6 +210,12 @@ class JSStackFrame : public nsIStackFrame {
  private:
   virtual ~JSStackFrame();
 
+  void Clear() override { mStack = nullptr; }
+
+  // Remove this frame from the per-realm list of live frames,
+  // and clear out the stack pointer.
+  void UnregisterAndClear();
+
   JS::Heap<JSObject*> mStack;
   nsString mFormattedStack;
 
@@ -246,15 +253,29 @@ JSStackFrame::JSStackFrame(JS::Handle<JSObject*> aStack)
   MOZ_ASSERT(JS::IsUnwrappedSavedFrame(mStack));
 
   mozilla::HoldJSObjects(this);
+
+  xpc::RegisterJSStackFrame(js::GetNonCCWObjectRealm(aStack), this);
 }
 
-JSStackFrame::~JSStackFrame() { mozilla::DropJSObjects(this); }
+JSStackFrame::~JSStackFrame() {
+  UnregisterAndClear();
+  mozilla::DropJSObjects(this);
+}
+
+void JSStackFrame::UnregisterAndClear() {
+  if (!mStack) {
+    return;
+  }
+
+  xpc::UnregisterJSStackFrame(js::GetNonCCWObjectRealm(mStack), this);
+  Clear();
+}
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(JSStackFrame)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(JSStackFrame)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mCaller)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mAsyncCaller)
-  tmp->mStack = nullptr;
+  tmp->UnregisterAndClear();
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(JSStackFrame)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCaller)

js/xpconnect/src/XPCJSRuntime.cpp

@@ -382,6 +382,51 @@ static bool PrincipalImmuneToScriptPolicy(nsIPrincipal* aPrincipal) {
   return false;
 }
 
+void RealmPrivate::RegisterStackFrame(JSStackFrameBase* aFrame) {
+  mJSStackFrames.PutEntry(aFrame);
+}
+
+void RealmPrivate::UnregisterStackFrame(JSStackFrameBase* aFrame) {
+  mJSStackFrames.RemoveEntry(aFrame);
+}
+
+void RealmPrivate::NukeJSStackFrames() {
+  for (auto iter = mJSStackFrames.Iter(); !iter.Done(); iter.Next()) {
+    iter.Get()->GetKey()->Clear();
+  }
+
+  mJSStackFrames.Clear();
+}
+
+void xpc::RegisterJSStackFrame(JS::Realm* aRealm,
+                               JSStackFrameBase* aStackFrame) {
+  RealmPrivate* realmPrivate = RealmPrivate::Get(aRealm);
+  if (!realmPrivate) {
+    return;
+  }
+
+  realmPrivate->RegisterStackFrame(aStackFrame);
+}
+
+void xpc::UnregisterJSStackFrame(JS::Realm* aRealm,
+                                 JSStackFrameBase* aStackFrame) {
+  RealmPrivate* realmPrivate = RealmPrivate::Get(aRealm);
+  if (!realmPrivate) {
+    return;
+  }
+
+  realmPrivate->UnregisterStackFrame(aStackFrame);
+}
+
+void xpc::NukeJSStackFrames(JS::Realm* aRealm) {
+  RealmPrivate* realmPrivate = RealmPrivate::Get(aRealm);
+  if (!realmPrivate) {
+    return;
+  }
+
+  realmPrivate->NukeJSStackFrames();
+}
+
 Scriptability::Scriptability(JS::Realm* realm)
     : mScriptBlocks(0),
       mDocShellAllowsScript(true),

js/xpconnect/src/xpcprivate.h

@@ -2915,11 +2915,19 @@ class RealmPrivate {
     locationURI = aLocationURI;
   }
 
+  // JSStackFrames are tracked on a per-realm basis so they
+  // can be cleared when the associated window goes away.
+  void RegisterStackFrame(JSStackFrameBase* aFrame);
+  void UnregisterStackFrame(JSStackFrameBase* aFrame);
+  void NukeJSStackFrames();
+
  private:
   nsCString location;
   nsCOMPtr<nsIURI> locationURI;
 
   bool TryParseLocationURI(LocationHint aType, nsIURI** aURI);
+
+  nsTHashtable<nsPtrHashKey<JSStackFrameBase>> mJSStackFrames;
 };
 
 inline XPCWrappedNativeScope* ObjectScope(JSObject* obj) {

js/xpconnect/src/xpcpublic.h

@@ -714,6 +714,15 @@ bool IfaceID2JSValue(JSContext* aCx, const nsXPTInterfaceInfo& aInfo,
 bool ContractID2JSValue(JSContext* aCx, JSString* aContract,
                         JS::MutableHandleValue aVal);
 
+class JSStackFrameBase {
+ public:
+  virtual void Clear() = 0;
+};
+
+void RegisterJSStackFrame(JS::Realm* aRealm, JSStackFrameBase* aStackFrame);
+void UnregisterJSStackFrame(JS::Realm* aRealm, JSStackFrameBase* aStackFrame);
+void NukeJSStackFrames(JS::Realm* aRealm);
+
 }  // namespace xpc
 
 namespace mozilla {

js/xpconnect/tests/browser/browser.ini

@@ -1,4 +1,6 @@
 [DEFAULT]
 support-files =
+  browser_consoleStack.html
   browser_deadObjectOnUnload.html
 [browser_dead_object.js]
+[browser_exception_leak.js]

js/xpconnect/tests/browser/browser_consoleStack.html

@@ -0,0 +1,21 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+Test page for https://bugzilla.mozilla.org/show_bug.cgi?id=1471989
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test page for Bug 1471989</title>
+</head>
+<body onUnload="onUnload();">
+<p><span id="samplepage">sample page</span></p>
+<script type="application/javascript">
+  // Get something sent to ConsoleStorageAPI that has a stack.
+  console.trace("whatever");
+
+  function onUnload() {
+    console.log('in unload');
+  }
+</script>
+</body>
+</html>

js/xpconnect/tests/browser/browser_exception_leak.js

@@ -0,0 +1,52 @@
+/* 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/.
+ */
+
+// For bug 1471989, test that an exception saved by chrome code can't leak the page.
+
+add_task(async function test() {
+  const url = "http://mochi.test:8888/browser/js/xpconnect/tests/browser/browser_consoleStack.html";
+  let newTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, url);
+  let browser = gBrowser.selectedBrowser;
+  let innerWindowId = browser.innerWindowID;
+
+  let stackTraceEmpty = await ContentTask.spawn(browser, {innerWindowId}, async function(args) {
+    let {TestUtils} = ChromeUtils.import("resource://testing-common/TestUtils.jsm", {});
+    let {Assert} = ChromeUtils.import("resource://testing-common/Assert.jsm", {});
+
+    const ConsoleAPIStorage = Cc["@mozilla.org/consoleAPI-storage;1"].getService(Ci.nsIConsoleAPIStorage);
+    let consoleEvents = ConsoleAPIStorage.getEvents(args.innerWindowId);
+    Assert.equal(consoleEvents.length, 1, "Should only be one console event for the window");
+
+    // Intentionally hold a reference to the console event.
+    let leakedConsoleEvent = consoleEvents[0];
+
+    let doc = content.document;
+    let promise = TestUtils.topicObserved("inner-window-nuked", (subject, data) => {
+      let id = subject.QueryInterface(Ci.nsISupportsPRUint64).data;
+      return id == args.innerWindowId;
+    });
+    content.location = "http://example.org/";
+    await promise;
+
+    // This string should be empty. For that to happen, two things
+    // need to be true:
+    //
+    // a) ConsoleCallData::mStack is not null. This means that the
+    // stack trace was not reified before the page was nuked. If it
+    // was, then the correct |filename| value would be stored on the
+    // object. (This is not a problem, except that it stops us from
+    // testing the next condition.)
+    //
+    // b) ConsoleData::mStack.mStack is null. This means that the
+    // JSStackFrame is keeping alive the JS object in the page after
+    // the page was nuked, which leaks the page.
+    return leakedConsoleEvent.stacktrace[0].filename;
+  });
+
+  is(stackTraceEmpty, "",
+     "JSStackFrame shouldn't leak mStack after window nuking");
+
+  BrowserTestUtils.removeTab(newTab);
+});