1271493 - service workers that fail to install due to 404 during Cache.addAll() are still leaking in pgo builds

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

+++ This bug was initially created as a clone of Bug #1270161 +++

Bug 1270161 fixed the leak in my local opt-DEBUG build, but the leak is still happening in opt/pgo builds.  There must still be a race leading to a feature leaking the worker thread.

My current thought is that its probably the Cache feature.  I will investigate tomorrow.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I can only reproduce this in pgo builds.  If I add any debugging it goes away.  Seems there is a very fine race somewhere after queueing the control runnable to cleanup.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Actually, it has nothing to do with PGO.

I am using NS_WARN_IF_FALSE() to warn if the control runnable cannot be dispatched.  This breaks in opt because NS_WARN_IF_FALSE() does NOT evaluate its expression in on-DEBUG builds.

This is my fault, but talk about a footgun when NS_WARN_IF() is in the tree and DOES evaluate its expression in non-DEBUG.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Dispatch fetch control runnable even in opt builds. r=mccr8

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/212d1af61570

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Backed out:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32c3d6465f96

Apparently that NS_IMETHOD change really is only acceptable on beta.  Looks like khuey did not make nsICancelableRunnable::Cancel() NS_IMETHOD when he refactored it.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Dispatch fetch control runnable even in opt builds. r=mccr8

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89476d7fd188

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Ben Kelly [:bkelly] from comment #2)
> This is my fault, but talk about a footgun when NS_WARN_IF() is in the tree
> and DOES evaluate its expression in non-DEBUG.

Uh, yeah, that's crazy.  We should fix it.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/585e0723bae5

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #7)
> Uh, yeah, that's crazy.  We should fix it.

I wrote bug 1272010 to discuss any changes there.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8751044 [details] [diff] [review]
Dispatch fetch control runnable even in opt builds. r=mccr8

Approval Request Comment
[Feature/regressing bug #]: Bug 1270161 and its original memory leak.
[User impact if declined]: The memory leak in bug 1270161 continues to be a problem in opt builds due to a bug in my previous fix.  I accidentally used a macro that compiles to nothing in opt builds which prevented that fix from doing anything.
[Describe test coverage new/current, TreeHerder]: Same tests as bug 1270161. We have tests to protect against regressions, but no test to verify the leak is fixed.  I wrote bug 1270594 about infrastructure we would need to build to write tests for this kind of leak.  Unfortunately its not available now.
[Risks and why]: Minimal.  This change is equivalent to the old code in debug builds and now works in opt builds as well.
[String/UUID change made/needed]: None.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/585e0723bae5

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8751044 [details] [diff] [review]
Dispatch fetch control runnable even in opt builds. r=mccr8

Fix for memory leak, please uplift. This could make it into beta 5 but more likely beta 6.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/20fb337ac107

and

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/55d9570b4520