Closed Bug 1333112 Opened 3 years ago Closed 3 years ago
Leaks in wpt service worker tests
I'm trying to enable leak checking for wpt, and it looks like perhaps some service worker tests are showing leaks (from the list of leaked urls): task 2017-01-19T18:37:31.340874Z] 18:37:31 INFO - PROCESS | 876 | http://www1.web-platform.test:8000/mixed-content/generic/expect.py?redirection=keep-scheme-redirect&action=purge&key=f2a1e836-e66d-4f78-bb24-41a48533aac4&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:37:31.340958Z] 18:37:31 INFO - PROCESS | 876 | https://web-platform.test:8443/service-workers/service-worker/resources/client-navigate-worker.js [task 2017-01-19T18:37:31.341237Z] 18:37:31 INFO - PROCESS | 876 | https://web-platform.test:8443/service-workers/service-worker/resources/client-navigated-frame.html [task 2017-01-19T18:37:31.341900Z] 18:37:31 INFO - PROCESS | 876 | https://web-platform.test:8443/service-workers/service-worker/resources/client-navigated-frame.html [task 2017-01-19T18:37:31.342035Z] 18:37:31 INFO - PROCESS | 876 | https://web-platform.test:8443/service-workers/service-worker/resources/client-navigate-worker.js [task 2017-01-19T18:37:31.342400Z] 18:37:31 INFO - PROCESS | 876 | https://www1.web-platform.test:8443/service-workers/service-worker/resources/client-navigated-frame.html [task 2017-01-19T18:37:31.342830Z] 18:37:31 INFO - PROCESS | 876 | https://www1.web-platform.test:8443/service-workers/service-worker/resources/client-navigated-frame.html https://public-artifacts.taskcluster.net/HszhxBGKTG2qqhPlEESNbQ/0/public/logs/live_backing.log
You need to provide some kind of instructions on how to reproduce this. I don't know how to enable leak checking in WPT locally and AFAICT its not available in try until bug 1333114 lands.
You would need to pull my commit from try. I could attach it to the bug if that helps?
Meant to link the try run, sorry: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53497702c4a5d1ff17229725d5675dd3935318fe
I can reproduce locally. I just ran the test and then looked in about:memory. Not sure yet what is going on.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Catalin said he'd take a look here.
Assignee: bkelly → catalin.badea392
Loading https://w3c-test.org/service-workers/service-worker/client-navigate.https.html in a debug build will leak a bunch of js globals: Leaked URLs: chrome://pluginproblem/content/pluginProblemBinding.css resource://gre-resources/counterstyles.css resource://gre-resources/html.css chrome://global/content/minimal-xul.css resource://gre-resources/quirk.css resource://gre/res/svg.css chrome://global/content/xul.css chrome://global/skin/scrollbars.css resource://gre-resources/number-control.css resource://gre-resources/forms.css resource://gre-resources/noscript.css resource://gre-resources/ua.css chrome://global/content/bindings/scrollbar.xml#scrollbar chrome://global/content/bindings/scrollbar.xml#thumb chrome://global/content/bindings/scrollbar.xml#scrollbar-base chrome://global/content/platformHTMLBindings.xml#inputFields resource://gre-resources/loading-image.png resource://gre-resources/broken-image.png resource://gre-resources/loading-image.png resource://gre-resources/broken-image.png https://w3c-test.org/service-workers/service-worker/resources/client-navigate-worker.js https://w3c-test.org/service-workers/service-worker/resources/client-navigated-frame.html https://w3c-test.org/service-workers/service-worker/resources/client-navigated-frame.html https://w3c-test.org/service-workers/service-worker/resources/client-navigate-worker.js https://www1.w3c-test.org/service-workers/service-worker/resources/client-navigated-frame.html https://www1.w3c-test.org/service-workers/service-worker/resources/client-navigated-frame.html resource://gre-resources/checkmark.svg
I think I found the issue, windowClient.Navigate will create a cycle between WebProgressListener and PromiseWorkerProxy here: http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/dom/workers/ServiceWorkerWindowClient.cpp#218 PromiseWorkerProxy should probably participate in cycle collection and traverse mSupports.
That does sound like the leak, but why should we use CC to break the cycle there? It seems like the cycle is purely in c++ right? We should probably avoid the cycle instead. Do you know why we need to store it in the mPromiseProxy nsISupports array?
(In reply to Ben Kelly [:bkelly] from comment #8) > That does sound like the leak, but why should we use CC to break the cycle > there? It seems like the cycle is purely in c++ right? We should probably > avoid the cycle instead. > > Do you know why we need to store it in the mPromiseProxy nsISupports array? Before we can resolve the navigate promise, we need to wait for the new url to be loaded using WebProgressListener. We need something to keep it alive while this happens, which is why it is currently stored in mPromiseProxy->mSupportsArray. I was wrong in my previous comment, making PromiseWorkerProxy cycle collected may cause everything to be collected too early (before WebProgressListener gets the final OnStateChange call). There are two options: 1) keep using mPromiseProxy->mSupportsArray and break the cycle when we get the final OnStateChange call 2) Use ServiceWorkerPrivate->StoreISupports instead, like we do for Clients.OpenWindow. SWP is cycle collected, but it is tied to a SWInfo so we're not forming an isolated cycle like we currently do with PromiseWorkerProxy. I'm in favor of option 2. Looks like this is the only place where we use PromiseWorkerProxy::StoreISupports, so we could just remove it. Hmm, I wonder what happens when there's a network error.
Attachment #8830831 - Flags: review?(bkelly) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/84d3a0578ffe Fix windowClient.Navigate leak. r=bkelly
We should probably uplift this to beta and aurora.
Comment on attachment 8830831 [details] [diff] [review] Fix windowClient.Navigate leak. Approval Request Comment [Feature/Bug causing the regression]: Service Workers [User impact if declined]: memory leak in SW code [Is this code covered by automated tests?]: We have automated tests for this code, but leak checking is not currently enabled. It will be after bug 1333114 is fixed. [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: This change is specific to SWs. [String changes made/needed]: none
Comment on attachment 8830831 [details] [diff] [review] Fix windowClient.Navigate leak. Service Worker leak fix, aurora53+, beta52+
You need to log in before you can comment on or make changes to this bug.