Back out 4 changesets (bug 1265771, bug 1266857) for leaks in browser_DownloadPDFSaver.js on Windows
authorPhil Ringnalda <philringnalda@gmail.com>
Fri, 22 Apr 2016 21:27:11 -0700
changeset 318397 94f517f324033ef28d216e69841a1fedf3bfa934
parent 318396 8f0b8d0c6bc3cd2127d92590ddb6652e1dc8d19a
child 318398 de2a25e8925636c5491901f78256c779372e2719
push id9480
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 17:12:58 +0000
treeherdermozilla-aurora@0d6a91c76a9e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1265771, 1266857
milestone48.0a1
backs outa0c85ccffafd5429453c823f5f551121d515463c
1cf8785bdc0ca68fadd0913ca29148136960b489
e411c3ccd7b6b30ca1fd3c98bfefb15fd7a6ea02
a298cd2c9417fd235b7080fe9e5fabd870117506
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
dom/workers/ServiceWorkerManager.cpp
dom/workers/ServiceWorkerManager.h
dom/workers/test/serviceworkers/browser_cached_force_refresh.html
dom/workers/test/serviceworkers/browser_force_refresh.js
dom/workers/test/serviceworkers/force_refresh_browser_worker.js
testing/web-platform/tests/service-workers/service-worker/navigate-window.https.html
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -1673,16 +1673,21 @@ nsDocument::~nsDocument()
   mPendingTitleChangeEvent.Revoke();
 
   // We don't want to leave residual locks on images. Make sure we're in an
   // unlocked state, and then clear the table.
   SetImageLockingState(false);
   mImageTracker.Clear();
 
   mPlugins.Clear();
+
+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
+  if (os) {
+    os->RemoveObserver(this, "service-worker-get-client");
+  }
 }
 
 NS_INTERFACE_TABLE_HEAD(nsDocument)
   NS_WRAPPERCACHE_INTERFACE_TABLE_ENTRY
   NS_INTERFACE_TABLE_BEGIN
     NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(nsDocument, nsISupports, nsINode)
     NS_INTERFACE_TABLE_ENTRY(nsDocument, nsINode)
     NS_INTERFACE_TABLE_ENTRY(nsDocument, nsIDocument)
@@ -2074,16 +2079,21 @@ nsDocument::Init()
   NS_ENSURE_TRUE(global, NS_ERROR_FAILURE);
   mScopeObject = do_GetWeakReference(global);
   MOZ_ASSERT(mScopeObject);
 
   mScriptLoader = new nsScriptLoader(this);
 
   mozilla::HoldJSObjects(this);
 
+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
+  if (os) {
+    os->AddObserver(this, "service-worker-get-client", /* ownsWeak */ true);
+  }
+
   return NS_OK;
 }
 
 void
 nsIDocument::DeleteAllProperties()
 {
   for (uint32_t i = 0; i < GetPropertyTableCount(); ++i) {
     PropertyTable(i)->DeleteAllProperties();
@@ -4674,37 +4684,16 @@ nsDocument::SetScriptGlobalObject(nsIScr
         // of bfcache.  Clear our state to allow that to happen.  Only
         // clear this flag if we are actually controlled, though, so pages
         // that were force reloaded don't become controlled when they
         // come out of bfcache.
         mMaybeServiceWorkerControlled = false;
       }
       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;
 
   if (aScriptGlobalObject) {
     mHasHadScriptHandlingObject = true;
     mHasHadDefaultView = true;
     // Go back to using the docshell for the layout history state
@@ -12452,28 +12441,21 @@ nsDocument::Observe(nsISupports *aSubjec
 {
   if (strcmp("app-theme-changed", aTopic) == 0) {
     if (!nsContentUtils::IsSystemPrincipal(NodePrincipal()) &&
         !IsUnstyledDocument()) {
       // We don't want to style the chrome window, only app ones.
       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));
       }
     }
   }
   return NS_OK;
 }
 
--- a/dom/workers/ServiceWorkerManager.cpp
+++ b/dom/workers/ServiceWorkerManager.cpp
@@ -1941,38 +1941,44 @@ ServiceWorkerManager::MaybeRemoveRegistr
   }
 }
 
 void
 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) {
     MOZ_ASSERT(!mControlledDocuments.Contains(aDoc));
     StartControllingADocument(registration, aDoc, aDocumentId);
   }
 }
 
 void
 ServiceWorkerManager::MaybeStopControlling(nsIDocument* aDoc)
 {
-  AssertIsOnMainThread();
   MOZ_ASSERT(aDoc);
   RefPtr<ServiceWorkerRegistrationInfo> registration;
   mControlledDocuments.Remove(aDoc, getter_AddRefs(registration));
   // A document which was uncontrolled does not maintain that state itself, so
   // it will always call MaybeStopControlling() even if there isn't an
   // associated registration. So this check is required.
   if (registration) {
     StopControllingADocument(registration);
   }
+
+  mAllDocuments.RemoveEntry(aDoc);
 }
 
 void
 ServiceWorkerManager::MaybeCheckNavigationUpdate(nsIDocument* aDoc)
 {
   AssertIsOnMainThread();
   MOZ_ASSERT(aDoc);
   // We perform these success path navigation update steps when the
@@ -2831,38 +2837,20 @@ ServiceWorkerManager::ClaimClients(nsIPr
     GetRegistration(aPrincipal, aScope);
 
   if (!registration || !registration->GetActive() ||
       !(registration->GetActive()->ID() == aId)) {
     // The worker is not active.
     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;
 }
 
 nsresult
 ServiceWorkerManager::SetSkipWaitingFlag(nsIPrincipal* aPrincipal,
                                          const nsCString& aScope,
--- a/dom/workers/ServiceWorkerManager.h
+++ b/dom/workers/ServiceWorkerManager.h
@@ -101,16 +101,19 @@ public:
 
   struct RegistrationDataPerPrincipal;
   nsClassHashtable<nsCStringHashKey, RegistrationDataPerPrincipal> mRegistrationInfos;
 
   nsTObserverArray<ServiceWorkerRegistrationListener*> mServiceWorkerRegistrationListeners;
 
   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;
   nsClassHashtable<nsCStringHashKey, WeakDocumentList> mRegisteringDocuments;
 
   // Track all intercepted navigation channels for a given scope.  Channels are
   // placed in the appropriate list before dispatch the FetchEvent to the worker
   // thread and removed once FetchEvent processing dispatches back to the main
--- a/dom/workers/test/serviceworkers/browser_cached_force_refresh.html
+++ b/dom/workers/test/serviceworkers/browser_cached_force_refresh.html
@@ -3,62 +3,15 @@
   http://creativecommons.org/publicdomain/zero/1.0/
 -->
 <!DOCTYPE HTML>
 <html>
 <head>
 </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
-  });
+addEventListener('load', function(event) {
+  var custom = new Event('cached-load', { bubbles: true });
   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);
-    });
 });
 </script>
 </body>
 </html>
--- a/dom/workers/test/serviceworkers/browser_force_refresh.js
+++ b/dom/workers/test/serviceworkers/browser_force_refresh.js
@@ -9,25 +9,24 @@ function refresh() {
 }
 
 function forceRefresh() {
   EventUtils.synthesizeKey('R', { accelKey: true, shiftKey: true });
 }
 
 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.
   addEventListener('base-load', eventHandler, true, true);
   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() {
   waitForExplicitFinish();
   SpecialPowers.pushPrefEnv({'set': [['dom.serviceWorkers.enabled', true],
                                      ['dom.serviceWorkers.exemptFromPerDomainMax', true],
                                      ['dom.serviceWorkers.testing.enabled', true],
                                      ['dom.caches.enabled', true],
@@ -43,49 +42,42 @@ function test() {
 
     function done() {
       tab.linkedBrowser.messageManager.removeMessageListener("test:event", eventHandler);
 
       gBrowser.removeTab(tab);
       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();
         }
         if (baseLoadCount !== 1) {
           ok(false, 'base load without cached load should only occur once');
           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;
     }
 
     tab.linkedBrowser.messageManager.addMessageListener("test:event", eventHandler);
   });
 }
--- a/dom/workers/test/serviceworkers/force_refresh_browser_worker.js
+++ b/dom/workers/test/serviceworkers/force_refresh_browser_worker.js
@@ -15,20 +15,8 @@ self.addEventListener('fetch', function 
   event.respondWith(
     caches.open(name).then(function(cache) {
       return cache.match(event.request);
     }).then(function(response) {
       return response || fetch(event.request);
     })
   );
 });
-
-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 });
-      }));
-  }
-});
--- a/testing/web-platform/tests/service-workers/service-worker/navigate-window.https.html
+++ b/testing/web-platform/tests/service-workers/service-worker/navigate-window.https.html
@@ -24,21 +24,16 @@ function with_window(url) {
   return wait_for_message('LOADED').then(_ => win);
 }
 
 function navigate_window(win, url) {
   win.location = 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);
 }
 
 function go_forward(win) {
   win.history.forward();
   return wait_for_message('PAGESHOW').then(_ => win);
@@ -61,81 +56,42 @@ function get_clients(win, sw, opts) {
 function validate_window(win, url, opts) {
   return win.navigator.serviceWorker.getRegistration(url)
     .then(reg => {
         // In order to compare service worker instances we need to
         // make sure the DOM object is owned by the same global; the
         // 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 }))
       .then(win => navigate_window(win, url2))
       .then(win => validate_window(win, url2, { includeUncontrolled: false }))
       .then(win => go_back(win))
       .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>