1333112 - Leaks in wpt service worker tests

Status: RESOLVED FIXED

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

Description

[James Graham [:jgraham]]

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

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[James Graham [:jgraham]]

You would need to pull my commit from try. I could attach it to the bug if that helps?

Comment 3

[James Graham [:jgraham]]

Meant to link the try run, sorry: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53497702c4a5d1ff17229725d5675dd3935318fe

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I can reproduce locally.  I just ran the test and then looked in about:memory.  Not sure yet what is going on.

Comment 5

[Andrew Overholt [:overholt]]

Catalin said he'd take a look here.

Comment 6

[Cătălin Badea (:catalinb)]

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

Comment 7

[Cătălin Badea (:catalinb)]

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 9

[Cătălin Badea (:catalinb)]

(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.

Comment 10

[Cătălin Badea (:catalinb)]

Attachment: Fix windowClient.Navigate leak.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84d3a0578ffe
Fix windowClient.Navigate leak. r=bkelly

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

We should probably uplift this to beta and aurora.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/84d3a0578ffe

Comment 14

[Cătălin Badea (:catalinb)]

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 15

[Julien Cristau [:jcristau]]

Comment on attachment 8830831 [details] [diff] [review]
Fix windowClient.Navigate leak.

Service Worker leak fix, aurora53+, beta52+

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f80dc144cdd0

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f4892327fe52