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 468832 60b3df69c5a9e204c9da031e6b515c9b9b8cc391
parent 468831 069ef3c1416b4bb80b1aaaee819f3736cb682ca8
child 468833 faa107ec149b0517fe2d54d5e1724dbaa25732e8
push id43551
push userbmo:kgilbert@mozilla.com
push dateTue, 31 Jan 2017 23:27:06 +0000
reviewersbkelly
bugs1316659
milestone54.0a1
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