Bug 1316659 - Use OpenWindowRunnable as an observer when waiting for fennec to start before executing an open window operation. r=bkelly
authorCatalin Badea <catalin.badea392@gmail.com>
Tue, 31 Jan 2017 14:21:52 +0000
changeset 378477 60b3df69c5a9e204c9da031e6b515c9b9b8cc391
parent 378476 069ef3c1416b4bb80b1aaaee819f3736cb682ca8
child 378478 faa107ec149b0517fe2d54d5e1724dbaa25732e8
push id7198
push userjlorenzo@mozilla.com
push dateTue, 18 Apr 2017 12:07:49 +0000
treeherdermozilla-beta@d57aa49c3948 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbkelly
bugs1316659
milestone54.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 1316659 - Use OpenWindowRunnable as an observer when waiting for fennec to start before executing an open window operation. r=bkelly This patch also changes the order in which pending openWindow operations are executed.
dom/workers/ServiceWorkerClients.cpp
dom/workers/ServiceWorkerPrivate.cpp
dom/workers/ServiceWorkerPrivate.h
--- a/dom/workers/ServiceWorkerClients.cpp
+++ b/dom/workers/ServiceWorkerClients.cpp
@@ -480,35 +480,69 @@ NS_IMPL_CYCLE_COLLECTION(WebProgressList
                          mServiceWorkerPrivate, mWindow)
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(WebProgressListener)
   NS_INTERFACE_MAP_ENTRY(nsIWebProgressListener)
   NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
 NS_INTERFACE_MAP_END
 
 class OpenWindowRunnable final : public Runnable
+                               , public nsIObserver
+                               , public nsSupportsWeakReference
 {
   RefPtr<PromiseWorkerProxy> mPromiseProxy;
   nsString mUrl;
   nsString mScope;
 
 public:
+  NS_DECL_ISUPPORTS_INHERITED
+  // Note: |OpenWindowRunnable| cannot be cycle collected because it inherits
+  // thread safe reference counting from |mozilla::Runnable|. On Fennec, we
+  // might use |ServiceWorkerPrivate::StoreISupports| to keep this object alive
+  // while waiting for an event from the observer service. As such, to avoid
+  // creating a cycle that will leak, |OpenWindowRunnable| must not hold a strong
+  // reference to |ServiceWorkerPrivate|.
+
   OpenWindowRunnable(PromiseWorkerProxy* aPromiseProxy,
                      const nsAString& aUrl,
                      const nsAString& aScope)
     : mPromiseProxy(aPromiseProxy)
     , mUrl(aUrl)
     , mScope(aScope)
   {
     MOZ_ASSERT(aPromiseProxy);
     MOZ_ASSERT(aPromiseProxy->GetWorkerPrivate());
     aPromiseProxy->GetWorkerPrivate()->AssertIsOnWorkerThread();
   }
 
   NS_IMETHOD
+  Observe(nsISupports* aSubject, const char* aTopic, const char16_t* /* aData */) override
+  {
+    AssertIsOnMainThread();
+
+    nsCString topic(aTopic);
+    if (!topic.Equals(NS_LITERAL_CSTRING("BrowserChrome:Ready"))) {
+      MOZ_ASSERT(false, "Unexpected topic.");
+      return NS_ERROR_FAILURE;
+    }
+
+    nsCOMPtr<nsIObserverService> os = services::GetObserverService();
+    NS_ENSURE_STATE(os);
+    os->RemoveObserver(this, "BrowserChrome:Ready");
+
+    RefPtr<ServiceWorkerPrivate> swp = GetServiceWorkerPrivate();
+    NS_ENSURE_STATE(swp);
+
+    MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(this));
+    swp->RemoveISupports(static_cast<nsIObserver*>(this));
+
+    return NS_OK;
+  }
+
+  NS_IMETHOD
   Run() override
   {
     AssertIsOnMainThread();
 
     MutexAutoLock lock(mPromiseProxy->Lock());
     if (mPromiseProxy->CleanedUp()) {
       return NS_OK;
     }
@@ -541,92 +575,91 @@ public:
 
       nsCOMPtr<nsIDocShell> docShell = window->GetDocShell();
       nsCOMPtr<nsIWebProgress> webProgress = do_GetInterface(docShell);
 
       if (!webProgress) {
         return NS_ERROR_FAILURE;
       }
 
-      RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
-      if (!swm) {
-        // browser shutdown
-        return NS_ERROR_FAILURE;
-      }
-
-      nsCOMPtr<nsIPrincipal> principal = workerPrivate->GetPrincipal();
-      MOZ_ASSERT(principal);
-      RefPtr<ServiceWorkerRegistrationInfo> registration =
-        swm->GetRegistration(principal, NS_ConvertUTF16toUTF8(mScope));
-      if (NS_WARN_IF(!registration)) {
-        return NS_ERROR_FAILURE;
-      }
-      RefPtr<ServiceWorkerInfo> serviceWorkerInfo =
-        registration->GetServiceWorkerInfoById(workerPrivate->ServiceWorkerID());
-      if (NS_WARN_IF(!serviceWorkerInfo)) {
-        return NS_ERROR_FAILURE;
-      }
+      RefPtr<ServiceWorkerPrivate> swp = GetServiceWorkerPrivate();
+      NS_ENSURE_STATE(swp);
 
       nsCOMPtr<nsIWebProgressListener> listener =
-        new WebProgressListener(mPromiseProxy, serviceWorkerInfo->WorkerPrivate(),
-                                window, baseURI);
+        new WebProgressListener(mPromiseProxy, swp, window, baseURI);
 
       rv = webProgress->AddProgressListener(listener,
                                             nsIWebProgress::NOTIFY_STATE_DOCUMENT);
       MOZ_ASSERT(NS_SUCCEEDED(rv));
       return NS_OK;
     }
 #ifdef MOZ_WIDGET_ANDROID
     else if (rv == NS_ERROR_NOT_AVAILABLE) {
       // We couldn't get a browser window, so Fennec must not be running.
       // Send an Intent to launch Fennec and wait for "BrowserChrome:Ready"
       // to try opening a window again.
+      RefPtr<ServiceWorkerPrivate> swp = GetServiceWorkerPrivate();
+      NS_ENSURE_STATE(swp);
+
       nsCOMPtr<nsIObserverService> os = services::GetObserverService();
       NS_ENSURE_STATE(os);
 
-      WorkerPrivate* workerPrivate = mPromiseProxy->GetWorkerPrivate();
-      MOZ_ASSERT(workerPrivate);
-
-      RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
-      if (!swm) {
-        // browser shutdown
-        return NS_ERROR_FAILURE;
-      }
-
-      nsCOMPtr<nsIPrincipal> principal = workerPrivate->GetPrincipal();
-      MOZ_ASSERT(principal);
+      rv = os->AddObserver(this, "BrowserChrome:Ready", /* weakRef */ true);
+      NS_ENSURE_SUCCESS(rv, rv);
 
-      RefPtr<ServiceWorkerRegistrationInfo> registration =
-        swm->GetRegistration(principal, NS_ConvertUTF16toUTF8(mScope));
-      if (NS_WARN_IF(!registration)) {
-        return NS_ERROR_FAILURE;
-      }
+      swp->StoreISupports(static_cast<nsIObserver*>(this));
 
-      RefPtr<ServiceWorkerInfo> serviceWorkerInfo =
-        registration->GetServiceWorkerInfoById(workerPrivate->ServiceWorkerID());
-      if (NS_WARN_IF(!serviceWorkerInfo)) {
-        return NS_ERROR_FAILURE;
-      }
-
-      os->AddObserver(static_cast<nsIObserver*>(serviceWorkerInfo->WorkerPrivate()),
-                      "BrowserChrome:Ready", true);
-      serviceWorkerInfo->WorkerPrivate()->AddPendingWindow(this);
       return NS_OK;
     }
 #endif
 
     RefPtr<ResolveOpenWindowRunnable> resolveRunnable =
       new ResolveOpenWindowRunnable(mPromiseProxy, nullptr, rv);
 
     Unused << NS_WARN_IF(!resolveRunnable->Dispatch());
 
     return NS_OK;
   }
 
 private:
+  ~OpenWindowRunnable()
+  { }
+
+  ServiceWorkerPrivate*
+  GetServiceWorkerPrivate() const
+  {
+    AssertIsOnMainThread();
+
+    RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
+    if (!swm) {
+      // browser shutdown
+      return nullptr;
+    }
+
+    WorkerPrivate* workerPrivate = mPromiseProxy->GetWorkerPrivate();
+    MOZ_ASSERT(workerPrivate);
+
+    nsCOMPtr<nsIPrincipal> principal = workerPrivate->GetPrincipal();
+    MOZ_DIAGNOSTIC_ASSERT(principal);
+
+    RefPtr<ServiceWorkerRegistrationInfo> registration =
+      swm->GetRegistration(principal, NS_ConvertUTF16toUTF8(mScope));
+    if (NS_WARN_IF(!registration)) {
+      return nullptr;
+    }
+
+    RefPtr<ServiceWorkerInfo> serviceWorkerInfo =
+      registration->GetServiceWorkerInfoById(workerPrivate->ServiceWorkerID());
+    if (NS_WARN_IF(!serviceWorkerInfo)) {
+      return nullptr;
+    }
+
+    return serviceWorkerInfo->WorkerPrivate();
+  }
+
   nsresult
   OpenWindow(nsPIDOMWindowOuter** aWindow)
   {
     MOZ_DIAGNOSTIC_ASSERT(aWindow);
     WorkerPrivate* workerPrivate = mPromiseProxy->GetWorkerPrivate();
 
     // [[1. Let url be the result of parsing url with entry settings object's API
     //   base URL.]]
@@ -725,16 +758,26 @@ private:
     nsCOMPtr<nsPIDOMWindowOuter> pWin = nsPIDOMWindowOuter::From(win);
     pWin.forget(aWindow);
     MOZ_DIAGNOSTIC_ASSERT(*aWindow);
 
     return NS_OK;
   }
 };
 
+NS_IMPL_ADDREF_INHERITED(OpenWindowRunnable, Runnable)                                    \
+NS_IMPL_RELEASE_INHERITED(OpenWindowRunnable, Runnable)
+
+NS_INTERFACE_MAP_BEGIN(OpenWindowRunnable)
+NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
+NS_INTERFACE_MAP_ENTRY(nsIObserver)
+NS_INTERFACE_MAP_ENTRY(nsIRunnable)
+NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIObserver)
+NS_INTERFACE_MAP_END
+
 } // namespace
 
 already_AddRefed<Promise>
 ServiceWorkerClients::Get(const nsAString& aClientId, ErrorResult& aRv)
 {
   WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
   MOZ_ASSERT(workerPrivate);
   workerPrivate->AssertIsOnWorkerThread();
--- a/dom/workers/ServiceWorkerPrivate.cpp
+++ b/dom/workers/ServiceWorkerPrivate.cpp
@@ -32,24 +32,22 @@
 #include "mozilla/dom/RequestBinding.h"
 #include "mozilla/Unused.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
 BEGIN_WORKERS_NAMESPACE
 
-NS_IMPL_CYCLE_COLLECTING_ADDREF(ServiceWorkerPrivate)
-NS_IMPL_CYCLE_COLLECTING_RELEASE(ServiceWorkerPrivate)
+NS_IMPL_CYCLE_COLLECTING_NATIVE_ADDREF(ServiceWorkerPrivate)
+NS_IMPL_CYCLE_COLLECTING_NATIVE_RELEASE(ServiceWorkerPrivate)
 NS_IMPL_CYCLE_COLLECTION(ServiceWorkerPrivate, mSupportsArray)
 
-NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(ServiceWorkerPrivate)
-  NS_INTERFACE_MAP_ENTRY(nsISupports)
-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIObserver)
-NS_INTERFACE_MAP_END
+NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(ServiceWorkerPrivate, AddRef)
+NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(ServiceWorkerPrivate, Release)
 
 // Tracks the "dom.disable_open_click_delay" preference.  Modified on main
 // thread, read on worker threads.
 // It is updated every time a "notificationclick" event is dispatched. While
 // this is done without synchronization, at the worst, the thread will just get
 // an older value within which a popup is allowed to be displayed, which will
 // still be a valid value since it was set prior to dispatching the runnable.
 Atomic<uint32_t> gDOMDisableOpenClickDelay(0);
@@ -1861,17 +1859,17 @@ ServiceWorkerPrivate::TerminateWorker()
   AssertIsOnMainThread();
 
   mIdleWorkerTimer->Cancel();
   mIdleKeepAliveToken = nullptr;
   if (mWorkerPrivate) {
     if (Preferences::GetBool("dom.serviceWorkers.testing.enabled")) {
       nsCOMPtr<nsIObserverService> os = services::GetObserverService();
       if (os) {
-        os->NotifyObservers(this, "service-worker-shutdown", nullptr);
+        os->NotifyObservers(nullptr, "service-worker-shutdown", nullptr);
       }
     }
 
     Unused << NS_WARN_IF(!mWorkerPrivate->Terminate());
     mWorkerPrivate = nullptr;
     mSupportsArray.Clear();
 
     // Any pending events are never going to fire on this worker.  Cancel
@@ -2092,48 +2090,16 @@ ServiceWorkerPrivate::CreateEventKeepAli
   AssertIsOnMainThread();
   MOZ_ASSERT(mWorkerPrivate);
   MOZ_ASSERT(mIdleKeepAliveToken);
   RefPtr<KeepAliveToken> ref = new KeepAliveToken(this);
   return ref.forget();
 }
 
 void
-ServiceWorkerPrivate::AddPendingWindow(Runnable* aPendingWindow)
-{
-  AssertIsOnMainThread();
-  pendingWindows.AppendElement(aPendingWindow);
-}
-
-nsresult
-ServiceWorkerPrivate::Observe(nsISupports* aSubject, const char* aTopic, const char16_t* aData)
-{
-  AssertIsOnMainThread();
-
-  nsCString topic(aTopic);
-  if (!topic.Equals(NS_LITERAL_CSTRING("BrowserChrome:Ready"))) {
-    MOZ_ASSERT(false, "Unexpected topic.");
-    return NS_ERROR_FAILURE;
-  }
-
-  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
-  NS_ENSURE_STATE(os);
-  os->RemoveObserver(static_cast<nsIObserver*>(this), "BrowserChrome:Ready");
-
-  size_t len = pendingWindows.Length();
-  for (int i = len-1; i >= 0; i--) {
-    RefPtr<Runnable> runnable = pendingWindows[i];
-    MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(runnable));
-    pendingWindows.RemoveElementAt(i);
-  }
-
-  return NS_OK;
-}
-
-void
 ServiceWorkerPrivate::SetHandlesFetch(bool aValue)
 {
   AssertIsOnMainThread();
 
   if (NS_WARN_IF(!mInfo)) {
     return;
   }
 
--- a/dom/workers/ServiceWorkerPrivate.h
+++ b/dom/workers/ServiceWorkerPrivate.h
@@ -57,25 +57,32 @@ public:
 // 2. If the worker stopped controlling documents and it is not handling push
 // events.
 // 3. The content process is shutting down.
 //
 // Adding an API function for a new event requires calling |SpawnWorkerIfNeeded|
 // with an appropriate reason before any runnable is dispatched to the worker.
 // If the event is extendable then the runnable should inherit
 // ExtendableEventWorkerRunnable.
-class ServiceWorkerPrivate final : public nsIObserver
+class ServiceWorkerPrivate final
 {
   friend class KeepAliveToken;
 
 public:
-  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
-  NS_DECL_CYCLE_COLLECTION_CLASS(ServiceWorkerPrivate)
-  NS_DECL_NSIOBSERVER
+  NS_IMETHOD_(MozExternalRefCountType) AddRef();
+  NS_IMETHOD_(MozExternalRefCountType) Release();
+  NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(ServiceWorkerPrivate)
+
+  typedef mozilla::FalseType HasThreadSafeRefCnt;
 
+protected:
+  nsCycleCollectingAutoRefCnt mRefCnt;
+  NS_DECL_OWNINGTHREAD
+
+public:
   explicit ServiceWorkerPrivate(ServiceWorkerInfo* aInfo);
 
   nsresult
   SendMessageEvent(JSContext* aCx, JS::Handle<JS::Value> aMessage,
                    const Optional<Sequence<JS::Value>>& aTransferable,
                    UniquePtr<ServiceWorkerClientInfo>&& aClientInfo);
 
   // This is used to validate the worker script and continue the installation
@@ -146,19 +153,16 @@ public:
 
   nsresult
   DetachDebugger();
 
   bool
   IsIdle() const;
 
   void
-  AddPendingWindow(Runnable* aPendingWindow);
-
-  void
   SetHandlesFetch(bool aValue);
 
 private:
   enum WakeUpReason {
     FetchEvent = 0,
     PushEvent,
     PushSubscriptionChangeEvent,
     MessageEvent,
@@ -223,17 +227,15 @@ private:
   // on the main thread. Access to this array is provided through
   // |StoreISupports| and |RemoveISupports|. Note that the array is also
   // cleared whenever the worker is terminated.
   nsTArray<nsCOMPtr<nsISupports>> mSupportsArray;
 
   // Array of function event worker runnables that are pending due to
   // the worker activating.  Main thread only.
   nsTArray<RefPtr<WorkerRunnable>> mPendingFunctionalEvents;
-
-  nsTArray<Runnable*> pendingWindows;
 };
 
 } // namespace workers
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_workers_serviceworkerprivate_h