Back out 4 changesets (bug 1265771, bug 1266857) for leaks in browser_DownloadPDFSaver.js on Windows

CLOSED TREE

Backed out changeset a0c85ccffafd (bug 1266857)
Backed out changeset 1cf8785bdc0c (bug 1265771)
Backed out changeset e411c3ccd7b6 (bug 1265771)
Backed out changeset a298cd2c9417 (bug 1265771)

dom/base/nsDocument.cpp

@@ -1678,6 +1678,11 @@ nsDocument::~nsDocument()
   mImageTracker.Clear();
 
   mPlugins.Clear();
+
+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
+  if (os) {
+    os->RemoveObserver(this, "service-worker-get-client");
+  }
 }
 
 NS_INTERFACE_TABLE_HEAD(nsDocument)
@@ -2079,6 +2084,11 @@ nsDocument::Init()
 
   mozilla::HoldJSObjects(this);
 
+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
+  if (os) {
+    os->AddObserver(this, "service-worker-get-client", /* ownsWeak */ true);
+  }
+
   return NS_OK;
 }
 
@@ -4679,27 +4689,6 @@ nsDocument::SetScriptGlobalObject(nsIScriptGlobalObject *aScriptGlobalObject)
       }
       swm->MaybeStopControlling(this);
     }
-
-    // Remove ourself from the list of clients.  We only register
-    // content principal documents in this list.
-    if (!nsContentUtils::IsSystemPrincipal(GetPrincipal()) &&
-        !GetPrincipal()->GetIsNullPrincipal()) {
-      nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
-      if (os) {
-        os->RemoveObserver(this, "service-worker-get-client");
-      }
-    }
-
-  } else if (!mScriptGlobalObject && aScriptGlobalObject &&
-             !nsContentUtils::IsSystemPrincipal(GetPrincipal()) &&
-             !GetPrincipal()->GetIsNullPrincipal()) {
-    // This document is being activated.  Register it in the list of
-    // clients.  We only do this for content principal documents
-    // since we can never observe system or null principals.
-    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
-    if (os) {
-      os->AddObserver(this, "service-worker-get-client", /* ownsWeak */ false);
-    }
   }
 
   mScriptGlobalObject = aScriptGlobalObject;
@@ -12457,18 +12446,11 @@ nsDocument::Observe(nsISupports *aSubject,
       OnAppThemeChanged();
     }
   } else if (strcmp("service-worker-get-client", aTopic) == 0) {
-    // No need to generate the ID if it doesn't exist here.  The ID being
-    // requested must already be generated in order to passed in as
-    // aSubject.
-    nsString clientId = GetId();
+    nsAutoString clientId;
+    GetOrCreateId(clientId);
     if (!clientId.IsEmpty() && clientId.Equals(aData)) {
       nsCOMPtr<nsISupportsInterfacePointer> ifptr = do_QueryInterface(aSubject);
       if (ifptr) {
-#ifdef DEBUG
-        nsCOMPtr<nsISupports> value;
-        MOZ_ALWAYS_SUCCEEDS(ifptr->GetData(getter_AddRefs(value)));
-        MOZ_ASSERT(!value);
-#endif
         ifptr->SetData(static_cast<nsIDocument*>(this));
         ifptr->SetDataIID(&NS_GET_IID(nsIDocument));
       }

dom/workers/ServiceWorkerManager.cpp

@@ -1946,7 +1946,12 @@ ServiceWorkerManager::MaybeStartControlling(nsIDocument* aDoc,
                                             const nsAString& aDocumentId)
 {
   AssertIsOnMainThread();
-  MOZ_ASSERT(aDoc);
+
+  // We keep a set of documents that service workers may choose to start
+  // controlling using claim().
+  MOZ_ASSERT(!mAllDocuments.Contains(aDoc));
+  mAllDocuments.PutEntry(aDoc);
+
   RefPtr<ServiceWorkerRegistrationInfo> registration =
     GetServiceWorkerRegistrationInfo(aDoc);
   if (registration) {
@@ -1958,7 +1963,6 @@ ServiceWorkerManager::MaybeStartControlling(nsIDocument* aDoc,
 void
 ServiceWorkerManager::MaybeStopControlling(nsIDocument* aDoc)
 {
-  AssertIsOnMainThread();
   MOZ_ASSERT(aDoc);
   RefPtr<ServiceWorkerRegistrationInfo> registration;
   mControlledDocuments.Remove(aDoc, getter_AddRefs(registration));
@@ -1968,6 +1972,8 @@ ServiceWorkerManager::MaybeStopControlling(nsIDocument* aDoc)
   if (registration) {
     StopControllingADocument(registration);
   }
+
+  mAllDocuments.RemoveEntry(aDoc);
 }
 
 void
@@ -2836,28 +2842,10 @@ ServiceWorkerManager::ClaimClients(nsIPrincipal* aPrincipal,
     return NS_ERROR_DOM_INVALID_STATE_ERR;
   }
 
-  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
-  if (NS_WARN_IF(!obs)) {
-    return NS_ERROR_FAILURE;
-  }
-
-  nsCOMPtr<nsISimpleEnumerator> enumerator;
-  nsresult rv = obs->EnumerateObservers("service-worker-get-client",
-                                        getter_AddRefs(enumerator));
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
-
-  bool loop = true;
-  while (NS_SUCCEEDED(enumerator->HasMoreElements(&loop)) && loop) {
-    nsCOMPtr<nsISupports> ptr;
-    rv = enumerator->GetNext(getter_AddRefs(ptr));
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      continue;
-    }
-
-    nsCOMPtr<nsIDocument> doc = do_QueryInterface(ptr);
-    MaybeClaimClient(doc, registration);
+  RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
+  for (auto iter = mAllDocuments.Iter(); !iter.Done(); iter.Next()) {
+    nsCOMPtr<nsIDocument> document = do_QueryInterface(iter.Get()->GetKey());
+    swm->MaybeClaimClient(document, registration);
   }
 
   return NS_OK;

dom/workers/ServiceWorkerManager.h

@@ -106,6 +106,9 @@ public:
 
   nsRefPtrHashtable<nsISupportsHashKey, ServiceWorkerRegistrationInfo> mControlledDocuments;
 
+  // Set of all documents that may be controlled by a service worker.
+  nsTHashtable<nsISupportsHashKey> mAllDocuments;
+
   // Track all documents that have attempted to register a service worker for a
   // given scope.
   typedef nsTArray<nsCOMPtr<nsIWeakReference>> WeakDocumentList;

dom/workers/test/serviceworkers/browser_cached_force_refresh.html

@@ -8,56 +8,9 @@
 </head>
 <body>
 <script type="text/javascript">
-function ok(exp, msg) {
-  if (!exp) {
-    throw(msg);
-  }
-}
-
-function is(actual, expected, msg) {
-  if (actual !== expected) {
-    throw('got "' + actual + '", but expected "' + expected + '" - ' + msg);
-  }
-}
-
-function fail(err) {
-  var custom = new CustomEvent('cached-failure', {
-    bubbles: true,
-    detail: err
-  });
-  document.dispatchEvent(custom);
-}
-
-function getUncontrolledClients(sw) {
-  return new Promise(function(resolve, reject) {
-    navigator.serviceWorker.addEventListener('message', function onMsg(evt) {
-      if (evt.data.type === 'CLIENTS') {
-        navigator.serviceWorker.removeEventListener('message', onMsg);
-        resolve(evt.data.detail);
-      }
-    });
-    sw.postMessage({ type: 'GET_UNCONTROLLED_CLIENTS' })
-  });
-}
-
 addEventListener('load', function(event) {
-  if (!navigator.serviceWorker.controller) {
-    return fail(window.location.href + ' is not controlled!');
-  }
-
-  getUncontrolledClients(navigator.serviceWorker.controller)
-    .then(function(clientList) {
-      is(clientList.length, 1, 'should only have one client');
-      is(clientList[0].url, window.location.href,
-         'client url should match current window');
-      is(clientList[0].frameType, 'top-level',
-         'client should be a top-level window');
-      var custom = new Event('cached-load', { bubbles: true });
-      document.dispatchEvent(custom);
-    })
-    .catch(function(err) {
-      fail(err);
-    });
+  var custom = new Event('cached-load', { bubbles: true });
+  document.dispatchEvent(custom);
 });
 </script>
 </body>

dom/workers/test/serviceworkers/browser_force_refresh.js

@@ -14,7 +14,7 @@ function forceRefresh() {
 
 function frameScript() {
   function eventHandler(event) {
-    sendAsyncMessage("test:event", {type: event.type, detail: event.detail});
+    sendAsyncMessage("test:event", {type: event.type});
   }
 
   // These are tab-local, so no need to unregister them.
@@ -22,7 +22,6 @@ function frameScript() {
   addEventListener('base-register', eventHandler, true, true);
   addEventListener('base-sw-ready', eventHandler, true, true);
   addEventListener('cached-load', eventHandler, true, true);
-  addEventListener('cached-failure', eventHandler, true, true);
 }
 
 function test() {
@@ -48,14 +47,13 @@ function test() {
       executeSoon(finish);
     }
 
-    var maxCacheLoadCount = 3;
-    var cachedLoadCount = 0;
+    var cachedLoad = false;
     var baseLoadCount = 0;
 
     function eventHandler(msg) {
       if (msg.data.type === 'base-load') {
         baseLoadCount += 1;
-        if (cachedLoadCount === maxCacheLoadCount) {
+        if (cachedLoad) {
           is(baseLoadCount, 2, 'cached load should occur before second base load');
           return done();
         }
@@ -64,23 +62,17 @@ function test() {
           return done();
         }
       } else if (msg.data.type === 'base-register') {
-        ok(!cachedLoadCount, 'cached load should not occur before base register');
+        ok(!cachedLoad, 'cached load should not occur before base register');
         is(baseLoadCount, 1, 'register should occur after first base load');
       } else if (msg.data.type === 'base-sw-ready') {
-        ok(!cachedLoadCount, 'cached load should not occur before base ready');
+        ok(!cachedLoad, 'cached load should not occur before base ready');
         is(baseLoadCount, 1, 'ready should occur after first base load');
         refresh();
       } else if (msg.data.type === 'cached-load') {
-        ok(cachedLoadCount < maxCacheLoadCount, 'cached load should not occur too many times');
+        ok(!cachedLoad, 'cached load should not occur twice');
         is(baseLoadCount, 1, 'cache load occur after first base load');
-        cachedLoadCount += 1;
-        if (cachedLoadCount < maxCacheLoadCount) {
-          return refresh();
-        }
+        cachedLoad = true;
         forceRefresh();
-      } else if (msg.data.type === 'cached-failure') {
-        ok(false, 'failure: ' + msg.data.detail);
-        done();
       }
 
       return;

dom/workers/test/serviceworkers/force_refresh_browser_worker.js

@@ -20,15 +20,3 @@ self.addEventListener('fetch', function (event) {
     })
   );
 });
-
-self.addEventListener('message', function (event) {
-  if (event.data.type === 'GET_UNCONTROLLED_CLIENTS') {
-    event.waitUntil(clients.matchAll({ includeUncontrolled: true })
-      .then(function(clientList) {
-        var resultList = clientList.map(function(c) {
-          return { url: c.url, frameType: c.frameType };
-        });
-        event.source.postMessage({ type: 'CLIENTS', detail: resultList });
-      }));
-  }
-});

testing/web-platform/tests/service-workers/service-worker/navigate-window.https.html

@@ -29,11 +29,6 @@ function navigate_window(win, url) {
   return wait_for_message('LOADED').then(_ => win);
 }
 
-function reload_window(win) {
-  win.location.reload();
-  return wait_for_message('LOADED').then(_ => win);
-}
-
 function go_back(win) {
   win.history.back();
   return wait_for_message('PAGESHOW').then(_ => win);
@@ -66,39 +61,24 @@ function validate_window(win, url, opts) {
         // opened window in this case.
         assert_equals(win.navigator.serviceWorker.controller, reg.active,
                       'window should be controlled by service worker');
-        return get_clients(win, reg.active, opts);
+        return get_clients(win, reg.active);
       })
     .then(resultList => {
-        // We should always see our controlled window.
-        var expected = [
-          { url: url, frameType: 'auxiliary' }
-        ];
-        // If we are including uncontrolled windows, then we might see the
-        // test window itself and the test harness.
-        if (opts.includeUncontrolled) {
-          expected.push({ url: BASE_URL + 'navigate-window.https.html',
-                          frameType: 'auxiliary' });
-          expected.push({ url: host_info['HTTPS_ORIGIN'] + '/testharness_runner.html',
-                          frameType: 'top-level' });
-        }
-        assert_equals(resultList.length, expected.length,
-                      'expected number of clients');
-        for (var i = 0; i < resultList.length; ++i) {
-          assert_equals(resultList[i].url, expected[i].url,
-                        'client should have expected url');
-          assert_equals(resultList[i].frameType, expected[i].frameType,
-                        ' client should have expected frame type');
-        }
+        assert_equals(resultList.length, 1, 'there should only be one client');
+        assert_equals(resultList[0].url, url,
+                      'client should be our opened window');
+        assert_equals(resultList[0].frameType, 'auxiliary',
+                      'window.open() should create a client with an auxiliary frame type');
         return win;
       })
 }
 
-promise_test(function(t) {
+async_test(function(t) {
     var worker = BASE_URL + 'resources/navigate-window-worker.js';
-    var scope = BASE_URL + 'resources/loaded.html?navigate-window-controlled';
+    var scope = BASE_URL + 'resources/loaded.html?navigate-window';
     var url1 = scope + '&q=1';
     var url2 = scope + '&q=2';
-    return service_worker_unregister_and_register(t, worker, scope)
+    service_worker_unregister_and_register(t, worker, scope)
       .then(reg => wait_for_state(t, reg.installing, 'activated') )
       .then(___ => with_window(url1))
       .then(win => validate_window(win, url1, { includeUncontrolled: false }))
@@ -108,34 +88,10 @@ promise_test(function(t) {
       .then(win => validate_window(win, url1, { includeUncontrolled: false }))
       .then(win => go_forward(win))
       .then(win => validate_window(win, url2, { includeUncontrolled: false }))
-      .then(win => reload_window(win))
-      .then(win => validate_window(win, url2, { includeUncontrolled: false }))
       .then(win => win.close())
       .catch(unreached_rejection(t))
-      .then(___ => service_worker_unregister(t, scope))
+      .then(___ => service_worker_unregister_and_done(t, scope))
   }, 'Clients.matchAll() should not show an old window as controlled after ' +
      'it navigates.');
-
-promise_test(function(t) {
-    var worker = BASE_URL + 'resources/navigate-window-worker.js';
-    var scope = BASE_URL + 'resources/loaded.html?navigate-window-uncontrolled';
-    var url1 = scope + '&q=1';
-    var url2 = scope + '&q=2';
-    return service_worker_unregister_and_register(t, worker, scope)
-      .then(reg => wait_for_state(t, reg.installing, 'activated') )
-      .then(___ => with_window(url1))
-      .then(win => validate_window(win, url1, { includeUncontrolled: true }))
-      .then(win => navigate_window(win, url2))
-      .then(win => validate_window(win, url2, { includeUncontrolled: true }))
-      .then(win => go_back(win))
-      .then(win => validate_window(win, url1, { includeUncontrolled: true }))
-      .then(win => go_forward(win))
-      .then(win => validate_window(win, url2, { includeUncontrolled: true }))
-      .then(win => reload_window(win))
-      .then(win => validate_window(win, url2, { includeUncontrolled: true }))
-      .then(win => win.close())
-      .catch(unreached_rejection(t))
-      .then(___ => service_worker_unregister(t, scope))
-  }, 'Clients.matchAll() should not show an old window after it navigates.');
 </script>
 </body>