Bug 1438211 P8 Hold the ServiceWorkerRegistration alive until the global is detached or the backing ServiceWorkerRegistrationInfo is removed. r=asuth
authorBen Kelly <ben@wanderview.com>
Thu, 08 Mar 2018 13:43:33 -0500
changeset 407230 bcea03ca0cda
parent 407229 076cd67616db
child 407231 42f6cbb08529
push id33596
push userncsoregi@mozilla.com
push dateFri, 09 Mar 2018 00:18:11 +0000
treeherdermozilla-central@31a33fc61956 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs1438211
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1438211 P8 Hold the ServiceWorkerRegistration alive until the global is detached or the backing ServiceWorkerRegistrationInfo is removed. r=asuth
dom/serviceworkers/ServiceWorkerRegistration.h
dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp
dom/serviceworkers/ServiceWorkerRegistrationImpl.h
--- a/dom/serviceworkers/ServiceWorkerRegistration.h
+++ b/dom/serviceworkers/ServiceWorkerRegistration.h
@@ -126,17 +126,22 @@ public:
 private:
   ServiceWorkerRegistration(nsIGlobalObject* aGlobal,
                             const ServiceWorkerRegistrationDescriptor& aDescriptor,
                             Inner* aInner);
 
   ~ServiceWorkerRegistration();
 
   ServiceWorkerRegistrationDescriptor mDescriptor;
+
+  // This forms a ref-cycle with the inner implementation object.  Its broken
+  // when either the global is torn down or the registration is removed from
+  // the ServiceWorkerManager.
   RefPtr<Inner> mInner;
+
   RefPtr<ServiceWorker> mInstallingWorker;
   RefPtr<ServiceWorker> mWaitingWorker;
   RefPtr<ServiceWorker> mActiveWorker;
   RefPtr<PushManager> mPushManager;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(ServiceWorkerRegistration, NS_DOM_SERVICEWORKERREGISTRATION_IID)
 
--- a/dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp
+++ b/dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp
@@ -79,36 +79,56 @@ ServiceWorkerRegistrationMainThread::Sto
   RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
   if (swm) {
     swm->RemoveRegistrationEventListener(mScope, this);
   }
   mListeningForEvents = false;
 }
 
 void
+ServiceWorkerRegistrationMainThread::RegistrationRemovedInternal()
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  // If the registration is being removed completely, remove it from the
+  // window registration hash table so that a new registration would get a new
+  // wrapper JS object.
+  if (mOuter && mOuter->GetOwner()) {
+    mOuter->GetOwner()->InvalidateServiceWorkerRegistration(mScope);
+  }
+  StopListeningForEvents();
+
+  // Since the registration is effectively dead in the SWM we can break
+  // the ref-cycle and let the binding object clean up.
+  mOuter = nullptr;
+}
+
+void
 ServiceWorkerRegistrationMainThread::UpdateFound()
 {
   mOuter->DispatchTrustedEvent(NS_LITERAL_STRING("updatefound"));
 }
 
 void
 ServiceWorkerRegistrationMainThread::UpdateState(const ServiceWorkerRegistrationDescriptor& aDescriptor)
 {
   mOuter->UpdateState(aDescriptor);
 }
 
 void
 ServiceWorkerRegistrationMainThread::RegistrationRemoved()
 {
-  // If the registration is being removed completely, remove it from the
-  // window registration hash table so that a new registration would get a new
-  // wrapper JS object.
-  if (nsCOMPtr<nsPIDOMWindowInner> window = mOuter->GetOwner()) {
-    window->InvalidateServiceWorkerRegistration(mScope);
-  }
+  // Queue a runnable to clean up the registration.  This is necessary
+  // because there may be runnables in the event queue already to
+  // update the registration state.  We want to let those run
+  // if possible before clearing our mOuter reference.
+  nsCOMPtr<nsIRunnable> r = NewRunnableMethod(
+    "ServiceWorkerRegistrationMainThread::RegistrationRemoved",
+    this,
+    &ServiceWorkerRegistrationMainThread::RegistrationRemovedInternal);
+  MOZ_ALWAYS_SUCCEEDS(SystemGroup::Dispatch(TaskCategory::Other, r.forget()));
 }
 
 bool
 ServiceWorkerRegistrationMainThread::MatchesDescriptor(const ServiceWorkerRegistrationDescriptor& aDescriptor)
 {
   return mOuter->MatchesDescriptor(aDescriptor);
 }
 
@@ -119,18 +139,17 @@ ServiceWorkerRegistrationMainThread::Set
   MOZ_DIAGNOSTIC_ASSERT(!mOuter);
   mOuter = aReg;
   StartListeningForEvents();
 }
 
 void
 ServiceWorkerRegistrationMainThread::ClearServiceWorkerRegistration(ServiceWorkerRegistration* aReg)
 {
-  MOZ_DIAGNOSTIC_ASSERT(mOuter);
-  MOZ_DIAGNOSTIC_ASSERT(mOuter == aReg);
+  MOZ_ASSERT_IF(mOuter, mOuter == aReg);
   StopListeningForEvents();
   mOuter = nullptr;
 }
 
 namespace {
 
 void
 UpdateInternal(nsIPrincipal* aPrincipal,
@@ -598,16 +617,21 @@ public:
   }
 };
 } // namespace
 
 already_AddRefed<Promise>
 ServiceWorkerRegistrationMainThread::Update(ErrorResult& aRv)
 {
   MOZ_ASSERT(NS_IsMainThread());
+  if (!mOuter) {
+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
+    return nullptr;
+  }
+
   nsCOMPtr<nsIGlobalObject> go = mOuter->GetParentObject();
   if (!go) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
   RefPtr<Promise> promise = Promise::Create(go, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
@@ -623,16 +647,21 @@ ServiceWorkerRegistrationMainThread::Upd
 
   return promise.forget();
 }
 
 already_AddRefed<Promise>
 ServiceWorkerRegistrationMainThread::Unregister(ErrorResult& aRv)
 {
   MOZ_ASSERT(NS_IsMainThread());
+  if (!mOuter) {
+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
+    return nullptr;
+  }
+
   nsCOMPtr<nsIGlobalObject> go = mOuter->GetParentObject();
   if (!go) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
   // Although the spec says that the same-origin checks should also be done
   // asynchronously, we do them in sync because the Promise created by the
@@ -690,16 +719,21 @@ ServiceWorkerRegistrationMainThread::Unr
 // Notification API extension.
 already_AddRefed<Promise>
 ServiceWorkerRegistrationMainThread::ShowNotification(JSContext* aCx,
                                                       const nsAString& aTitle,
                                                       const NotificationOptions& aOptions,
                                                       ErrorResult& aRv)
 {
   MOZ_ASSERT(NS_IsMainThread());
+  if (!mOuter) {
+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
+    return nullptr;
+  }
+
   nsCOMPtr<nsPIDOMWindowInner> window = mOuter->GetOwner();
   if (NS_WARN_IF(!window)) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
   nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
   if (NS_WARN_IF(!doc)) {
@@ -722,30 +756,39 @@ ServiceWorkerRegistrationMainThread::Sho
 
   return p.forget();
 }
 
 already_AddRefed<Promise>
 ServiceWorkerRegistrationMainThread::GetNotifications(const GetNotificationOptions& aOptions, ErrorResult& aRv)
 {
   MOZ_ASSERT(NS_IsMainThread());
+  if (!mOuter) {
+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
+    return nullptr;
+  }
   nsCOMPtr<nsPIDOMWindowInner> window = mOuter->GetOwner();
   if (NS_WARN_IF(!window)) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return nullptr;
   }
   return Notification::Get(window, aOptions, mScope, aRv);
 }
 
 already_AddRefed<PushManager>
 ServiceWorkerRegistrationMainThread::GetPushManager(JSContext* aCx,
                                                     ErrorResult& aRv)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
+  if (!mOuter) {
+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
+    return nullptr;
+  }
+
   nsCOMPtr<nsIGlobalObject> globalObject = mOuter->GetParentObject();
 
   if (!globalObject) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
   GlobalObject global(aCx, globalObject->GetGlobalJSObject());
@@ -800,17 +843,19 @@ public:
     }
   }
 
   void
   StopListeningForEvents()
   {
     MOZ_ASSERT(NS_IsMainThread());
 
-    MOZ_ASSERT(mListeningForEvents);
+    if (!mListeningForEvents) {
+      return;
+    }
 
     RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
 
     // We aren't going to need this anymore and we shouldn't hold on since the
     // worker will go away soon.
     mWorkerPrivate = nullptr;
 
     if (swm) {
@@ -827,20 +872,17 @@ public:
   void
   UpdateState(const ServiceWorkerRegistrationDescriptor& aDescriptor) override
   {
     MOZ_ASSERT(NS_IsMainThread());
     // TODO: Not implemented
   }
 
   void
-  RegistrationRemoved() override
-  {
-    MOZ_ASSERT(NS_IsMainThread());
-  }
+  RegistrationRemoved() override;
 
   void
   GetScope(nsAString& aScope) const override
   {
     aScope = mScope;
   }
 
   bool
@@ -886,29 +928,34 @@ ServiceWorkerRegistrationWorkerThread::S
 
 ServiceWorkerRegistrationWorkerThread::~ServiceWorkerRegistrationWorkerThread()
 {
   MOZ_DIAGNOSTIC_ASSERT(!mListener);
   MOZ_DIAGNOSTIC_ASSERT(!mOuter);
 }
 
 void
+ServiceWorkerRegistrationWorkerThread::RegistrationRemoved()
+{
+  mOuter = nullptr;
+}
+
+void
 ServiceWorkerRegistrationWorkerThread::SetServiceWorkerRegistration(ServiceWorkerRegistration* aReg)
 {
   MOZ_DIAGNOSTIC_ASSERT(aReg);
   MOZ_DIAGNOSTIC_ASSERT(!mOuter);
   mOuter = aReg;
   InitListener();
 }
 
 void
 ServiceWorkerRegistrationWorkerThread::ClearServiceWorkerRegistration(ServiceWorkerRegistration* aReg)
 {
-  MOZ_DIAGNOSTIC_ASSERT(mOuter);
-  MOZ_DIAGNOSTIC_ASSERT(mOuter == aReg);
+  MOZ_ASSERT_IF(mOuter, mOuter == aReg);
   ReleaseListener();
   mOuter = nullptr;
 }
 
 already_AddRefed<Promise>
 ServiceWorkerRegistrationWorkerThread::Update(ErrorResult& aRv)
 {
   WorkerPrivate* worker = GetCurrentThreadWorkerPrivate();
@@ -1020,16 +1067,24 @@ ServiceWorkerRegistrationWorkerThread::R
   mListener = nullptr;
   mWorkerPrivate = nullptr;
 }
 
 bool
 ServiceWorkerRegistrationWorkerThread::Notify(WorkerStatus aStatus)
 {
   ReleaseListener();
+
+  // Break the ref-cycle immediately when the worker thread starts to
+  // teardown.  We must make sure its GC'd before the worker RuntimeService
+  // is destroyed.  The WorkerListener may not be able to post a runnable
+  // clearing this value after shutdown begins and thus delaying cleanup
+  // too late.
+  mOuter = nullptr;
+
   return true;
 }
 
 class FireUpdateFoundRunnable final : public WorkerRunnable
 {
   RefPtr<WorkerListener> mListener;
 public:
   FireUpdateFoundRunnable(WorkerPrivate* aWorkerPrivate,
@@ -1063,16 +1118,60 @@ WorkerListener::UpdateFound()
   MOZ_ASSERT(NS_IsMainThread());
   if (mWorkerPrivate) {
     RefPtr<FireUpdateFoundRunnable> r =
       new FireUpdateFoundRunnable(mWorkerPrivate, this);
     Unused << NS_WARN_IF(!r->Dispatch());
   }
 }
 
+class RegistrationRemovedWorkerRunnable final : public WorkerRunnable
+{
+  RefPtr<WorkerListener> mListener;
+public:
+  RegistrationRemovedWorkerRunnable(WorkerPrivate* aWorkerPrivate,
+                                    WorkerListener* aListener)
+    : WorkerRunnable(aWorkerPrivate)
+    , mListener(aListener)
+  {
+    // Need this assertion for now since runnables which modify busy count can
+    // only be dispatched from parent thread to worker thread and we don't deal
+    // with nested workers. SW threads can't be nested.
+    MOZ_ASSERT(aWorkerPrivate->IsServiceWorker());
+  }
+
+  bool
+  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
+  {
+    MOZ_ASSERT(aWorkerPrivate);
+    aWorkerPrivate->AssertIsOnWorkerThread();
+
+    ServiceWorkerRegistrationWorkerThread* reg = mListener->GetRegistration();
+    if (reg) {
+      reg->RegistrationRemoved();
+    }
+    return true;
+  }
+};
+
+void
+WorkerListener::RegistrationRemoved()
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!mWorkerPrivate) {
+    return;
+  }
+
+  RefPtr<WorkerRunnable> r =
+    new RegistrationRemovedWorkerRunnable(mWorkerPrivate, this);
+  Unused << r->Dispatch();
+
+  StopListeningForEvents();
+}
+
 // Notification API extension.
 already_AddRefed<Promise>
 ServiceWorkerRegistrationWorkerThread::ShowNotification(JSContext* aCx,
                                                         const nsAString& aTitle,
                                                         const NotificationOptions& aOptions,
                                                         ErrorResult& aRv)
 {
   if (!mWorkerPrivate) {
--- a/dom/serviceworkers/ServiceWorkerRegistrationImpl.h
+++ b/dom/serviceworkers/ServiceWorkerRegistrationImpl.h
@@ -80,17 +80,20 @@ private:
   ~ServiceWorkerRegistrationMainThread();
 
   void
   StartListeningForEvents();
 
   void
   StopListeningForEvents();
 
-  ServiceWorkerRegistration* mOuter;
+  void
+  RegistrationRemovedInternal();
+
+  RefPtr<ServiceWorkerRegistration> mOuter;
   const nsString mScope;
   bool mListeningForEvents;
 };
 
 ////////////////////////////////////////////////////
 // Worker Thread implementation
 
 class WorkerListener;
@@ -99,16 +102,19 @@ class ServiceWorkerRegistrationWorkerThr
                                                   , public WorkerHolder
 {
 public:
   NS_INLINE_DECL_REFCOUNTING(ServiceWorkerRegistrationWorkerThread, override)
 
   ServiceWorkerRegistrationWorkerThread(WorkerPrivate* aWorkerPrivate,
                                         const ServiceWorkerRegistrationDescriptor& aDescriptor);
 
+  void
+  RegistrationRemoved();
+
   // ServiceWorkerRegistration::Inner
   void
   SetServiceWorkerRegistration(ServiceWorkerRegistration* aReg) override;
 
   void
   ClearServiceWorkerRegistration(ServiceWorkerRegistration* aReg) override;
 
   already_AddRefed<Promise>
@@ -141,16 +147,21 @@ private:
   ~ServiceWorkerRegistrationWorkerThread();
 
   void
   InitListener();
 
   void
   ReleaseListener();
 
-  ServiceWorkerRegistration* mOuter;
+  // Store a strong reference to the outer binding object.  This will create
+  // a ref-cycle.  We must hold it alive in case any events need to be fired
+  // on it.  The cycle is broken when the global becomes detached or the
+  // registration is removed in the ServiceWorkerManager.
+  RefPtr<ServiceWorkerRegistration> mOuter;
+
   WorkerPrivate* mWorkerPrivate;
   const nsString mScope;
   RefPtr<WorkerListener> mListener;
 };
 
 } // dom namespace
 } // mozilla namespace