From 296f3a95c32319875c79d89d94576fc1f842c2dc Mon Sep 17 00:00:00 2001 From: Phil Ringnalda Date: Fri, 22 Apr 2016 21:27:11 -0700 Subject: [PATCH] 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 | 42 ++++-------- dom/workers/ServiceWorkerManager.cpp | 36 ++++------- dom/workers/ServiceWorkerManager.h | 3 + .../browser_cached_force_refresh.html | 51 +-------------- .../serviceworkers/browser_force_refresh.js | 22 ++----- .../force_refresh_browser_worker.js | 12 ---- .../service-worker/navigate-window.https.html | 64 +++---------------- 7 files changed, 46 insertions(+), 184 deletions(-) diff --git a/dom/base/nsDocument.cpp b/dom/base/nsDocument.cpp index ee503c46db2d..1955321cf3cd 100644 --- a/dom/base/nsDocument.cpp +++ b/dom/base/nsDocument.cpp @@ -1678,6 +1678,11 @@ nsDocument::~nsDocument() mImageTracker.Clear(); mPlugins.Clear(); + + nsCOMPtr 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 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 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 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 ifptr = do_QueryInterface(aSubject); if (ifptr) { -#ifdef DEBUG - nsCOMPtr value; - MOZ_ALWAYS_SUCCEEDS(ifptr->GetData(getter_AddRefs(value))); - MOZ_ASSERT(!value); -#endif ifptr->SetData(static_cast(this)); ifptr->SetDataIID(&NS_GET_IID(nsIDocument)); } diff --git a/dom/workers/ServiceWorkerManager.cpp b/dom/workers/ServiceWorkerManager.cpp index 0c6d339c7f4c..ac20c90fe25f 100644 --- a/dom/workers/ServiceWorkerManager.cpp +++ b/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 registration = GetServiceWorkerRegistrationInfo(aDoc); if (registration) { @@ -1958,7 +1963,6 @@ ServiceWorkerManager::MaybeStartControlling(nsIDocument* aDoc, void ServiceWorkerManager::MaybeStopControlling(nsIDocument* aDoc) { - AssertIsOnMainThread(); MOZ_ASSERT(aDoc); RefPtr 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 obs = mozilla::services::GetObserverService(); - if (NS_WARN_IF(!obs)) { - return NS_ERROR_FAILURE; - } - - nsCOMPtr 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 ptr; - rv = enumerator->GetNext(getter_AddRefs(ptr)); - if (NS_WARN_IF(NS_FAILED(rv))) { - continue; - } - - nsCOMPtr doc = do_QueryInterface(ptr); - MaybeClaimClient(doc, registration); + RefPtr swm = ServiceWorkerManager::GetInstance(); + for (auto iter = mAllDocuments.Iter(); !iter.Done(); iter.Next()) { + nsCOMPtr document = do_QueryInterface(iter.Get()->GetKey()); + swm->MaybeClaimClient(document, registration); } return NS_OK; diff --git a/dom/workers/ServiceWorkerManager.h b/dom/workers/ServiceWorkerManager.h index 75c71e4e7412..601fea9a768d 100644 --- a/dom/workers/ServiceWorkerManager.h +++ b/dom/workers/ServiceWorkerManager.h @@ -106,6 +106,9 @@ public: nsRefPtrHashtable mControlledDocuments; + // Set of all documents that may be controlled by a service worker. + nsTHashtable mAllDocuments; + // Track all documents that have attempted to register a service worker for a // given scope. typedef nsTArray> WeakDocumentList; diff --git a/dom/workers/test/serviceworkers/browser_cached_force_refresh.html b/dom/workers/test/serviceworkers/browser_cached_force_refresh.html index 33bd8cdaa66b..4b550a3d23a3 100644 --- a/dom/workers/test/serviceworkers/browser_cached_force_refresh.html +++ b/dom/workers/test/serviceworkers/browser_cached_force_refresh.html @@ -8,56 +8,9 @@ diff --git a/dom/workers/test/serviceworkers/browser_force_refresh.js b/dom/workers/test/serviceworkers/browser_force_refresh.js index a2c9c871c37d..9f5b3bcb8f2b 100644 --- a/dom/workers/test/serviceworkers/browser_force_refresh.js +++ b/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; diff --git a/dom/workers/test/serviceworkers/force_refresh_browser_worker.js b/dom/workers/test/serviceworkers/force_refresh_browser_worker.js index 96d9d0f176d0..0fe91ed95a1b 100644 --- a/dom/workers/test/serviceworkers/force_refresh_browser_worker.js +++ b/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 }); - })); - } -}); diff --git 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 index e3aaf4c5cd42..4a0e51d3d84b 100644 --- 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 @@ -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.');