Bug 1450274 P1 Make ServiceWorker use DOMEventTargetHelper::KeepAliveIfHasListenersFor(). r=asuth
authorBen Kelly <ben@wanderview.com>
Tue, 10 Apr 2018 11:00:56 -0700
changeset 412650 c1838e2fbb4f7c6efb049a5b7b2ee45b8b361805
parent 412649 417fa1dd6bb9019f8b6da83096761be573bdc52b
child 412651 6337703ef9d8338e21704eb0a2e0c2a54dc83bb4
push id33814
push userccoroiu@mozilla.com
push dateTue, 10 Apr 2018 21:56:42 +0000
treeherdermozilla-central@0528a414c2a8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs1450274
milestone61.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 1450274 P1 Make ServiceWorker use DOMEventTargetHelper::KeepAliveIfHasListenersFor(). r=asuth
dom/serviceworkers/ServiceWorker.cpp
dom/serviceworkers/ServiceWorker.h
dom/serviceworkers/ServiceWorkerInfo.cpp
dom/serviceworkers/ServiceWorkerInfo.h
--- a/dom/serviceworkers/ServiceWorker.cpp
+++ b/dom/serviceworkers/ServiceWorker.cpp
@@ -71,26 +71,29 @@ ServiceWorker::ServiceWorker(nsIGlobalOb
   : DOMEventTargetHelper(aGlobal)
   , mDescriptor(aDescriptor)
   , mInner(aInner)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_DIAGNOSTIC_ASSERT(aGlobal);
   MOZ_DIAGNOSTIC_ASSERT(mInner);
 
+  KeepAliveIfHasListenersFor(NS_LITERAL_STRING("statechange"));
+
+  // The error event handler is required by the spec currently, but is not used
+  // anywhere.  Don't keep the object alive in that case.
+
   // This will update our state too.
   mInner->AddServiceWorker(this);
 }
 
 ServiceWorker::~ServiceWorker()
 {
   MOZ_ASSERT(NS_IsMainThread());
-  if (mInner) {
-    mInner->RemoveServiceWorker(this);
-  }
+  mInner->RemoveServiceWorker(this);
 }
 
 NS_IMPL_ADDREF_INHERITED(ServiceWorker, DOMEventTargetHelper)
 NS_IMPL_RELEASE_INHERITED(ServiceWorker, DOMEventTargetHelper)
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(ServiceWorker)
   NS_INTERFACE_MAP_ENTRY(ServiceWorker)
 NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
@@ -109,33 +112,42 @@ ServiceWorker::State() const
   return mDescriptor.State();
 }
 
 void
 ServiceWorker::SetState(ServiceWorkerState aState)
 {
   ServiceWorkerState oldState = mDescriptor.State();
   mDescriptor.SetState(aState);
-  if (oldState != aState) {
-    DOMEventTargetHelper::DispatchTrustedEvent(NS_LITERAL_STRING("statechange"));
+  if (oldState == aState) {
+    return;
+  }
+
+  DOMEventTargetHelper::DispatchTrustedEvent(NS_LITERAL_STRING("statechange"));
+
+  // Once we have transitioned to the redundant state then no
+  // more statechange events will occur.  We can allow the DOM
+  // object to GC if script is not holding it alive.
+  if (mDescriptor.State() == ServiceWorkerState::Redundant) {
+    IgnoreKeepAliveIfHasListenersFor(NS_LITERAL_STRING("statechange"));
   }
 }
 
 void
 ServiceWorker::GetScriptURL(nsString& aURL) const
 {
   CopyUTF8toUTF16(mDescriptor.ScriptURL(), aURL);
 }
 
 void
 ServiceWorker::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
                            const Sequence<JSObject*>& aTransferable,
                            ErrorResult& aRv)
 {
-  if (State() == ServiceWorkerState::Redundant || !mInner) {
+  if (State() == ServiceWorkerState::Redundant) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return;
   }
 
   mInner->PostMessage(GetParentObject(), aCx, aMessage, aTransferable, aRv);
 }
 
 
@@ -143,17 +155,13 @@ const ServiceWorkerDescriptor&
 ServiceWorker::Descriptor() const
 {
   return mDescriptor;
 }
 
 void
 ServiceWorker::DisconnectFromOwner()
 {
-  if (mInner) {
-    mInner->RemoveServiceWorker(this);
-    mInner = nullptr;
-  }
   DOMEventTargetHelper::DisconnectFromOwner();
 }
 
 } // namespace dom
 } // namespace mozilla
--- a/dom/serviceworkers/ServiceWorker.h
+++ b/dom/serviceworkers/ServiceWorker.h
@@ -98,20 +98,16 @@ private:
                 const ServiceWorkerDescriptor& aDescriptor,
                 Inner* aInner);
 
   // This class is reference-counted and will be destroyed from Release().
   ~ServiceWorker();
 
   ServiceWorkerDescriptor mDescriptor;
 
-  // Hold a strong reference to the inner service worker object.  This will
-  // create a ref-cycle.  The cycle is broken when either DisconnectFromOwner()
-  // is called due to the global tearing down or when the underlying service
-  // worker transitions to the redundant state.
   RefPtr<Inner> mInner;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(ServiceWorker, NS_DOM_SERVICEWORKER_IID)
 
 } // namespace dom
 } // namespace mozilla
 
--- a/dom/serviceworkers/ServiceWorkerInfo.cpp
+++ b/dom/serviceworkers/ServiceWorkerInfo.cpp
@@ -113,17 +113,17 @@ ServiceWorkerInfo::DetachDebugger()
   return mServiceWorkerPrivate->DetachDebugger();
 }
 
 namespace {
 
 class ChangeStateUpdater final : public Runnable
 {
 public:
-  ChangeStateUpdater(const nsTArray<RefPtr<ServiceWorker>>& aInstances,
+  ChangeStateUpdater(const nsTArray<ServiceWorker*>& aInstances,
                      ServiceWorkerState aState)
     : Runnable("dom::ChangeStateUpdater")
     , mState(aState)
   {
     for (size_t i = 0; i < aInstances.Length(); ++i) {
       mInstances.AppendElement(aInstances[i]);
     }
   }
@@ -171,21 +171,16 @@ ServiceWorkerInfo::UpdateState(ServiceWo
   if (State() != aState) {
     mServiceWorkerPrivate->UpdateState(aState);
   }
   mDescriptor.SetState(aState);
   nsCOMPtr<nsIRunnable> r = new ChangeStateUpdater(mInstances, State());
   MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(r.forget()));
   if (State() == ServiceWorkerState::Redundant) {
     serviceWorkerScriptCache::PurgeCache(mPrincipal, mCacheName);
-
-    // Break the ref-cycle with the binding objects.  We won't need to
-    // fire any more events and they should be able to GC once content
-    // script no longer references them.
-    mInstances.Clear();
   }
 }
 
 ServiceWorkerInfo::ServiceWorkerInfo(nsIPrincipal* aPrincipal,
                                      const nsACString& aScope,
                                      const nsACString& aScriptSpec,
                                      const nsAString& aCacheName,
                                      nsLoadFlags aImportsLoadFlags)
@@ -251,21 +246,18 @@ ServiceWorkerInfo::RemoveServiceWorker(S
   MOZ_DIAGNOSTIC_ASSERT(aWorker);
 #ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
   nsAutoString workerURL;
   aWorker->GetScriptURL(workerURL);
   MOZ_DIAGNOSTIC_ASSERT(
     workerURL.Equals(NS_ConvertUTF8toUTF16(mDescriptor.ScriptURL())));
 #endif
 
-  // If the binding layer initiates this call by disconnecting the global,
-  // then we will find an entry in mInstances here.  If the worker transitions
-  // to redundant and we clear mInstances, then we will not find an entry
-  // here.
-  mInstances.RemoveElement(aWorker);
+  DebugOnly<bool> removed = mInstances.RemoveElement(aWorker);
+  MOZ_ASSERT(removed);
 }
 
 void
 ServiceWorkerInfo::PostMessage(nsIGlobalObject* aGlobal,
                                JSContext* aCx, JS::Handle<JS::Value> aMessage,
                                const Sequence<JSObject*>& aTransferable,
                                ErrorResult& aRv)
 {
--- a/dom/serviceworkers/ServiceWorkerInfo.h
+++ b/dom/serviceworkers/ServiceWorkerInfo.h
@@ -49,26 +49,22 @@ private:
   TimeStamp mCreationTimeStamp;
 
   // The time of states are 0, if SW has not reached that state yet. Besides, we
   // update each of them after UpdateState() is called in SWRegistrationInfo.
   PRTime mInstalledTime;
   PRTime mActivatedTime;
   PRTime mRedundantTime;
 
-  // Track the list of known binding objects so we can fire events on them
-  // when appropriate.  These are held using strong references so that they
-  // are not GC'd while an event handler is registered and could observe an
-  // event.  This reference will create a cycle with the binding object.  The
-  // cycle is broken when either the global is detached or the service worker
-  // transitions to the redundant state.
+  // We hold rawptrs since the ServiceWorker constructor and destructor ensure
+  // addition and removal.
   //
   // There is a high chance of there being at least one ServiceWorker
   // associated with this all the time.
-  AutoTArray<RefPtr<ServiceWorker>, 1> mInstances;
+  AutoTArray<ServiceWorker*, 1> mInstances;
 
   RefPtr<ServiceWorkerPrivate> mServiceWorkerPrivate;
   bool mSkipWaitingFlag;
 
   enum {
     Unknown,
     Enabled,
     Disabled