Closed Bug 1333112 Opened 3 years ago Closed 3 years ago

Leaks in wpt service worker tests

Categories

(Testing :: web-platform-tests, defect)

defect
Not set

Tracking

(firefox-esr45 disabled, firefox51 wontfix, firefox52 fixed, firefox53 fixed, firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- disabled
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: jgraham, Assigned: catalinb)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Flags: needinfo?(bkelly)
Blocks: 1333114
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?
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
Flags: needinfo?(bkelly)
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)
Attachment #8830831 - Flags: review?(bkelly) → review+
We should probably uplift this to beta and aurora.
https://hg.mozilla.org/mozilla-central/rev/84d3a0578ffe
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Attachment #8830831 - Flags: approval-mozilla-beta?
Attachment #8830831 - Flags: approval-mozilla-aurora?
Comment on attachment 8830831 [details] [diff] [review]
Fix windowClient.Navigate leak.

Service Worker leak fix, aurora53+, beta52+
Attachment #8830831 - Flags: approval-mozilla-beta?
Attachment #8830831 - Flags: approval-mozilla-beta+
Attachment #8830831 - Flags: approval-mozilla-aurora?
Attachment #8830831 - Flags: approval-mozilla-aurora+
Blocks: 1341685
Blocks: 1352355
No longer blocks: 1333114
You need to log in before you can comment on or make changes to this bug.