Closed
Bug 1271493
Opened 8 years ago
Closed 8 years ago
service workers that fail to install due to 404 during Cache.addAll() are still leaking in pgo builds
Categories
(Core :: DOM: Service Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
(Whiteboard: [MemShrink] btpp-active)
Attachments
(1 file, 1 obsolete file)
1.10 KB,
patch
|
bkelly
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ 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.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Summary: service workers that fail to install due to 404 during Cache.addAll() are still leaking in opt/pgo builds → service workers that fail to install due to 404 during Cache.addAll() are still leaking in pgo builds
Assignee | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8750957 -
Flags: review?(continuation)
Assignee | ||
Updated•8 years ago
|
status-firefox46:
--- → wontfix
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox-esr45:
--- → wontfix
Updated•8 years ago
|
Attachment #8750957 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89476d7fd188
Attachment #8750957 -
Attachment is obsolete: true
Attachment #8751044 -
Flags: review+
(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.
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7) > Uh, yeah, that's crazy. We should fix it. I wrote bug 1272010 to discuss any changes there.
Assignee | ||
Comment 10•8 years ago
|
||
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.
Attachment #8751044 -
Flags: approval-mozilla-beta?
Attachment #8751044 -
Flags: approval-mozilla-aurora?
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/585e0723bae5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 12•8 years ago
|
||
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.
Attachment #8751044 -
Flags: approval-mozilla-beta?
Attachment #8751044 -
Flags: approval-mozilla-beta+
Attachment #8751044 -
Flags: approval-mozilla-aurora?
Attachment #8751044 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 13•8 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/20fb337ac107 and remote: https://hg.mozilla.org/releases/mozilla-beta/rev/55d9570b4520
You need to log in
before you can comment on or make changes to this bug.
Description
•