Bug 1409115 - Fix leaks by adding promise window proxy. r=bkelly draft
authorAndrew McCreight <continuation@gmail.com>
Wed, 06 Dec 2017 16:48:18 -0800
changeset 710197 95b7e63632f211dcea78f5a85c1232da7f327e85
parent 710166 fc68e1a602bf81601fe51f94450e7a0a3668499f
child 743537 d64ff1a5e374213c8beb4eb2b74802225751fb57
push id92774
push userbmo:continuation@gmail.com
push dateFri, 08 Dec 2017 21:08:18 +0000
reviewersbkelly
bugs1409115
milestone59.0a1
Bug 1409115 - Fix leaks by adding promise window proxy. r=bkelly The service worker job queue can get stuck sometimes, so we don't want the per-process service worker data structure to hold strong references to content. Instead, I add a proxy that is only a weak reference. The wrinkle is that we need to keep the promise alive as long as the job and the window are otherwise alive. I solve this by putting a cycle collected reference to the promise on the window itself. MozReview-Commit-ID: GbmCY4ZIQWk
dom/base/nsGlobalWindowInner.cpp
dom/base/nsGlobalWindowInner.h
dom/promise/PromiseWindowProxy.cpp
dom/promise/PromiseWindowProxy.h
dom/promise/moz.build
dom/workers/ServiceWorkerManager.cpp
dom/workers/ServiceWorkerRegistration.cpp
--- a/dom/base/nsGlobalWindowInner.cpp
+++ b/dom/base/nsGlobalWindowInner.cpp
@@ -1510,16 +1510,18 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mExternal)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMozSelfSupport)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIntlUtils)
 
   tmp->TraverseHostObjectURIs(cb);
 
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mChromeFields.mMessageManager)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mChromeFields.mGroupMessageManagers)
+
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPendingPromises)
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsGlobalWindowInner)
   tmp->CleanupCachedXBLHandlers();
 
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mNavigator)
 
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mPerformance)
@@ -1604,16 +1606,18 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ns
       static_cast<nsFrameMessageManager*>(
         tmp->mChromeFields.mMessageManager.get())->Disconnect();
       NS_IMPL_CYCLE_COLLECTION_UNLINK(mChromeFields.mMessageManager)
     }
     tmp->DisconnectAndClearGroupMessageManagers();
     NS_IMPL_CYCLE_COLLECTION_UNLINK(mChromeFields.mGroupMessageManagers)
   }
 
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mPendingPromises)
+
   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 #ifdef DEBUG
 void
 nsGlobalWindowInner::RiskyUnlink()
 {
   NS_CYCLE_COLLECTION_INNERNAME.Unlink(this);
@@ -4942,16 +4946,29 @@ nsGlobalWindowInner::GetIndexedDB(ErrorR
     // This may keep mIndexedDB null without setting an error.
     aError = IDBFactory::CreateForWindow(this,
                                          getter_AddRefs(mIndexedDB));
   }
 
   return mIndexedDB;
 }
 
+void
+nsGlobalWindowInner::AddPendingPromise(mozilla::dom::Promise* aPromise)
+{
+  mPendingPromises.AppendElement(aPromise);
+}
+
+void
+nsGlobalWindowInner::RemovePendingPromise(mozilla::dom::Promise* aPromise)
+{
+  DebugOnly<bool> foundIt = mPendingPromises.RemoveElement(aPromise);
+  MOZ_ASSERT(foundIt, "tried to remove a non-existent element from mPendingPromises");
+}
+
 //*****************************************************************************
 // nsGlobalWindowInner::nsIInterfaceRequestor
 //*****************************************************************************
 
 NS_IMETHODIMP
 nsGlobalWindowInner::GetInterface(const nsIID & aIID, void **aSink)
 {
   nsGlobalWindowOuter* outer = GetOuterWindowInternal();
@@ -6731,16 +6748,19 @@ nsGlobalWindowInner::AddSizeOfIncludingT
   }
 
   if (mPerformance) {
     aWindowSizes.mDOMPerformanceUserEntries =
       mPerformance->SizeOfUserEntries(aWindowSizes.mState.mMallocSizeOf);
     aWindowSizes.mDOMPerformanceResourceEntries =
       mPerformance->SizeOfResourceEntries(aWindowSizes.mState.mMallocSizeOf);
   }
+
+  aWindowSizes.mDOMOtherSize +=
+    mPendingPromises.ShallowSizeOfExcludingThis(aWindowSizes.mState.mMallocSizeOf);
 }
 
 void
 nsGlobalWindowInner::AddGamepad(uint32_t aIndex, Gamepad* aGamepad)
 {
   // Create the index we will present to content based on which indices are
   // already taken, as required by the spec.
   // https://w3c.github.io/gamepad/gamepad.html#widl-Gamepad-index
--- a/dom/base/nsGlobalWindowInner.h
+++ b/dom/base/nsGlobalWindowInner.h
@@ -1159,16 +1159,20 @@ public:
 
   virtual uint32_t GetFocusMethod() override;
 
   virtual bool ShouldShowFocusRing() override;
 
   // Inner windows only.
   void UpdateCanvasFocus(bool aFocusChanged, nsIContent* aNewContent);
 
+  // See PromiseWindowProxy.h for an explanation.
+  void AddPendingPromise(mozilla::dom::Promise* aPromise);
+  void RemovePendingPromise(mozilla::dom::Promise* aPromise);
+
 public:
   virtual already_AddRefed<nsPIWindowRoot> GetTopWindowRoot() override;
 
 protected:
   static void NotifyDOMWindowDestroyed(nsGlobalWindowInner* aWindow);
   void NotifyWindowIDDestroyed(const char* aTopic);
 
   static void NotifyDOMWindowFrozen(nsGlobalWindowInner* aWindow);
@@ -1403,16 +1407,18 @@ protected:
   RefPtr<mozilla::dom::VREventObserver> mVREventObserver;
 
   int64_t mBeforeUnloadListenerCount;
 
   RefPtr<mozilla::dom::IntlUtils> mIntlUtils;
 
   mozilla::UniquePtr<mozilla::dom::ClientSource> mClientSource;
 
+  nsTArray<RefPtr<mozilla::dom::Promise>> mPendingPromises;
+
   static InnerWindowByIdTable* sInnerWindowsById;
 
   // Members in the mChromeFields member should only be used in chrome windows.
   // All accesses to this field should be guarded by a check of mIsChrome.
   struct ChromeFields {
     ChromeFields()
       : mGroupMessageManagers(1)
     {}
new file mode 100644
--- /dev/null
+++ b/dom/promise/PromiseWindowProxy.cpp
@@ -0,0 +1,54 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* 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 "mozilla/dom/PromiseWindowProxy.h"
+
+#include "nsGlobalWindowInner.h"
+#include "nsPIDOMWindow.h"
+#include "nsIWeakReference.h"
+#include "mozilla/dom/Promise.h"
+
+using namespace mozilla;
+using namespace dom;
+
+
+PromiseWindowProxy::PromiseWindowProxy(nsPIDOMWindowInner* aWindow, Promise* aPromise)
+  : mPromise(aPromise)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  MOZ_DIAGNOSTIC_ASSERT(aWindow && aPromise);
+  auto* window = nsGlobalWindowInner::Cast(aWindow);
+  window->GetWeakReference(getter_AddRefs(mWindow));
+  window->AddPendingPromise(aPromise);
+}
+
+PromiseWindowProxy::~PromiseWindowProxy()
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  nsCOMPtr<nsPIDOMWindowInner> window = GetWindow();
+  if (window && mPromise) {
+    nsGlobalWindowInner::Cast(window)->RemovePendingPromise(mPromise);
+  }
+}
+
+RefPtr<Promise>
+PromiseWindowProxy::Get() const
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!mPromise) {
+    return nullptr;
+  }
+  RefPtr<Promise> promise(mPromise);
+  return promise;
+}
+
+nsCOMPtr<nsPIDOMWindowInner>
+PromiseWindowProxy::GetWindow() const
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryReferent(mWindow);
+  return window;
+}
new file mode 100644
--- /dev/null
+++ b/dom/promise/PromiseWindowProxy.h
@@ -0,0 +1,57 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* 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/. */
+
+#ifndef mozilla_dom_PromiseWindowProxy_h
+#define mozilla_dom_PromiseWindowProxy_h
+
+#include "nsCOMPtr.h"
+#include "mozilla/WeakPtr.h"
+
+class nsIWeakReference;
+class nsPIDOMWindowInner;
+
+namespace mozilla {
+namespace dom {
+
+class Promise;
+
+// When a data structure is waiting to resolve a promise from a
+// window, but that data structure is not owned by that window, there
+// exists a dilemma:
+//
+// 1) If the promise is never resolved, and the non-window data
+// structure is keeping alive the promise, the promise's window will
+// be leaked.
+//
+// 2) Something has to keep the promise alive from native code in case
+// script has dropped all direct references to it, but the promise is
+// later resolved.
+//
+// PromiseWindowProxy solves this dilemma for the non-window data
+// structure. It solves (1) by only keeping a weak reference to the
+// promise. It solves (2) by adding a strong reference to the promise
+// on the window itself. This ensures that as long as both the
+// PromiseWindowProxy and the window are still alive that the promise
+// will remain alive. This strong reference is cycle collected, so it
+// won't cause the window to leak.
+class PromiseWindowProxy final
+{
+public:
+  PromiseWindowProxy(nsPIDOMWindowInner* aWindow, mozilla::dom::Promise* aPromise);
+  ~PromiseWindowProxy();
+
+  RefPtr<Promise> Get() const;
+  nsCOMPtr<nsPIDOMWindowInner> GetWindow() const;
+
+private:
+  nsCOMPtr<nsIWeakReference> mWindow;
+  WeakPtr<Promise> mPromise;
+};
+
+} // namespace dom
+} // namespace mozilla
+
+#endif // mozilla_dom_PromiseWindowProxy_h
--- a/dom/promise/moz.build
+++ b/dom/promise/moz.build
@@ -6,22 +6,24 @@
 
 with Files("**"):
     BUG_COMPONENT = ("Core", "DOM")
 
 EXPORTS.mozilla.dom += [
     'Promise.h',
     'PromiseDebugging.h',
     'PromiseNativeHandler.h',
+    'PromiseWindowProxy.h',
     'PromiseWorkerProxy.h',
 ]
 
 UNIFIED_SOURCES += [
     'Promise.cpp',
     'PromiseDebugging.cpp',
+    'PromiseWindowProxy.cpp',
 ]
 
 LOCAL_INCLUDES += [
     '../base',
     '../ipc',
     '../workers',
 ]
 
--- a/dom/workers/ServiceWorkerManager.cpp
+++ b/dom/workers/ServiceWorkerManager.cpp
@@ -41,16 +41,17 @@
 #include "mozilla/dom/ClientManager.h"
 #include "mozilla/dom/ContentParent.h"
 #include "mozilla/dom/ErrorEvent.h"
 #include "mozilla/dom/Headers.h"
 #include "mozilla/dom/InternalHeaders.h"
 #include "mozilla/dom/Navigator.h"
 #include "mozilla/dom/NotificationEvent.h"
 #include "mozilla/dom/PromiseNativeHandler.h"
+#include "mozilla/dom/PromiseWindowProxy.h"
 #include "mozilla/dom/Request.h"
 #include "mozilla/dom/RootedDictionary.h"
 #include "mozilla/dom/TypedArray.h"
 #include "mozilla/ipc/BackgroundChild.h"
 #include "mozilla/ipc/PBackgroundChild.h"
 #include "mozilla/ipc/PBackgroundSharedTypes.h"
 #include "mozilla/dom/ScriptLoader.h"
 #include "mozilla/Unused.h"
@@ -358,50 +359,57 @@ ServiceWorkerManager::MaybeStartShutdown
   RefPtr<TeardownRunnable> runnable = new TeardownRunnable(mActor);
   nsresult rv = NS_DispatchToMainThread(runnable);
   Unused << NS_WARN_IF(NS_FAILED(rv));
   mActor = nullptr;
 }
 
 class ServiceWorkerResolveWindowPromiseOnRegisterCallback final : public ServiceWorkerJob::Callback
 {
-  RefPtr<nsPIDOMWindowInner> mWindow;
   // The promise "returned" by the call to Update up to
   // navigator.serviceWorker.register().
-  RefPtr<Promise> mPromise;
+  PromiseWindowProxy mPromise;
 
   ~ServiceWorkerResolveWindowPromiseOnRegisterCallback()
   {}
 
   virtual void
   JobFinished(ServiceWorkerJob* aJob, ErrorResult& aStatus) override
   {
     AssertIsOnMainThread();
     MOZ_ASSERT(aJob);
+    RefPtr<Promise> promise = mPromise.Get();
+    if (!promise) {
+      return;
+    }
 
     if (aStatus.Failed()) {
-      mPromise->MaybeReject(aStatus);
+      promise->MaybeReject(aStatus);
+      return;
+    }
+
+    nsCOMPtr<nsPIDOMWindowInner> window = mPromise.GetWindow();
+    if (!window) {
       return;
     }
 
     MOZ_ASSERT(aJob->GetType() == ServiceWorkerJob::Type::Register);
     RefPtr<ServiceWorkerRegisterJob> registerJob =
       static_cast<ServiceWorkerRegisterJob*>(aJob);
     RefPtr<ServiceWorkerRegistrationInfo> reg = registerJob->GetRegistration();
 
     RefPtr<ServiceWorkerRegistration> swr =
-      mWindow->GetServiceWorkerRegistration(NS_ConvertUTF8toUTF16(reg->mScope));
-    mPromise->MaybeResolve(swr);
+      window->GetServiceWorkerRegistration(NS_ConvertUTF8toUTF16(reg->mScope));
+    promise->MaybeResolve(swr);
   }
 
 public:
   ServiceWorkerResolveWindowPromiseOnRegisterCallback(nsPIDOMWindowInner* aWindow,
                                                       Promise* aPromise)
-    : mWindow(aWindow)
-    , mPromise(aPromise)
+    : mPromise(aWindow, aPromise)
   {}
 
   NS_INLINE_DECL_REFCOUNTING(ServiceWorkerResolveWindowPromiseOnRegisterCallback, override)
 };
 
 namespace {
 
 class PropagateSoftUpdateRunnable final : public Runnable
--- a/dom/workers/ServiceWorkerRegistration.cpp
+++ b/dom/workers/ServiceWorkerRegistration.cpp
@@ -4,16 +4,17 @@
  * 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 "ServiceWorkerRegistration.h"
 
 #include "ipc/ErrorIPCUtils.h"
 #include "mozilla/dom/Notification.h"
 #include "mozilla/dom/Promise.h"
+#include "mozilla/dom/PromiseWindowProxy.h"
 #include "mozilla/dom/PromiseWorkerProxy.h"
 #include "mozilla/dom/PushManagerBinding.h"
 #include "mozilla/dom/PushManager.h"
 #include "mozilla/dom/ServiceWorkerRegistrationBinding.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/Services.h"
 #include "mozilla/Unused.h"
 #include "nsCycleCollectionParticipant.h"
@@ -408,38 +409,43 @@ UpdateInternal(nsIPrincipal* aPrincipal,
     return;
   }
 
   swm->Update(aPrincipal, NS_ConvertUTF16toUTF8(aScope), aCallback);
 }
 
 class MainThreadUpdateCallback final : public ServiceWorkerUpdateFinishCallback
 {
-  RefPtr<Promise> mPromise;
+  PromiseWindowProxy mPromise;
 
   ~MainThreadUpdateCallback()
   { }
 
 public:
-  explicit MainThreadUpdateCallback(Promise* aPromise)
-    : mPromise(aPromise)
+  explicit MainThreadUpdateCallback(nsPIDOMWindowInner* aWindow,
+                                    Promise* aPromise)
+    : mPromise(aWindow, aPromise)
   {
     AssertIsOnMainThread();
   }
 
   void
   UpdateSucceeded(ServiceWorkerRegistrationInfo* aRegistration) override
   {
-    mPromise->MaybeResolveWithUndefined();
+    if (RefPtr<Promise> promise = mPromise.Get()) {
+      promise->MaybeResolveWithUndefined();
+    }
   }
 
   void
   UpdateFailed(ErrorResult& aStatus) override
   {
-    mPromise->MaybeReject(aStatus);
+    if (RefPtr<Promise> promise = mPromise.Get()) {
+      promise->MaybeReject(aStatus);
+    }
   }
 };
 
 class UpdateResultRunnable final : public WorkerRunnable
 {
   RefPtr<PromiseWorkerProxy> mPromiseProxy;
   IPC::Message mSerializedErrorResult;
 
@@ -565,41 +571,46 @@ private:
   {}
 
   RefPtr<PromiseWorkerProxy> mPromiseProxy;
   const nsString mScope;
 };
 
 class UnregisterCallback final : public nsIServiceWorkerUnregisterCallback
 {
-  RefPtr<Promise> mPromise;
+  PromiseWindowProxy mPromise;
 
 public:
   NS_DECL_ISUPPORTS
 
-  explicit UnregisterCallback(Promise* aPromise)
-    : mPromise(aPromise)
+  explicit UnregisterCallback(nsPIDOMWindowInner* aWindow,
+                              Promise* aPromise)
+    : mPromise(aWindow, aPromise)
   {
-    MOZ_ASSERT(mPromise);
+    MOZ_ASSERT(aPromise);
   }
 
   NS_IMETHOD
   UnregisterSucceeded(bool aState) override
   {
     AssertIsOnMainThread();
-    mPromise->MaybeResolve(aState);
+    if (RefPtr<Promise> promise = mPromise.Get()) {
+      promise->MaybeResolve(aState);
+    }
     return NS_OK;
   }
 
   NS_IMETHOD
   UnregisterFailed() override
   {
     AssertIsOnMainThread();
 
-    mPromise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
+    if (RefPtr<Promise> promise = mPromise.Get()) {
+      promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
+    }
     return NS_OK;
   }
 
 private:
   ~UnregisterCallback()
   { }
 };
 
@@ -759,17 +770,17 @@ ServiceWorkerRegistrationMainThread::Upd
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
 
   nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
   MOZ_ASSERT(doc);
 
   RefPtr<MainThreadUpdateCallback> cb =
-    new MainThreadUpdateCallback(promise);
+    new MainThreadUpdateCallback(GetOwner(), promise);
   UpdateInternal(doc->NodePrincipal(), mScope, cb);
 
   return promise.forget();
 }
 
 already_AddRefed<Promise>
 ServiceWorkerRegistrationMainThread::Unregister(ErrorResult& aRv)
 {
@@ -817,17 +828,17 @@ ServiceWorkerRegistrationMainThread::Unr
   nsCOMPtr<nsIServiceWorkerManager> swm =
     mozilla::services::GetServiceWorkerManager();
 
   RefPtr<Promise> promise = Promise::Create(go, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
 
-  RefPtr<UnregisterCallback> cb = new UnregisterCallback(promise);
+  RefPtr<UnregisterCallback> cb = new UnregisterCallback(GetOwner(), promise);
 
   NS_ConvertUTF8toUTF16 scope(uriSpec);
   aRv = swm->Unregister(documentPrincipal, cb, scope);
   if (aRv.Failed()) {
     return nullptr;
   }
 
   return promise.forget();