Closed
Bug 827541
Opened 12 years ago
Closed 12 years ago
Leak calling AudioContext on removed iframe
Categories
(Core :: Web Audio, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
| Tracking | Status | |
|---|---|---|
| firefox19 | --- | unaffected |
| firefox20 | --- | fixed |
| firefox21 | --- | fixed |
People
(Reporter: jruderman, Assigned: mccr8)
References
Details
(Keywords: memory-leak, regression, testcase, Whiteboard: [MemShrink])
Attachments
(2 files, 1 obsolete file)
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
ehsan.akhgari
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1. Set:
user_pref("media.webaudio.enabled", true);
2. Run a debug build of Firefox with XPCOM_MEM_LEAK_LOG=2
3. Load the testcase
4. Quit Firefox
Result: trace-refcnt reports that a bunch of stuff leaked.
|
Assignee | |
Comment 1•12 years ago
|
||
I can do some initial analysis of what is happening here, if you'd like, Ehsan. Just assign it to me in that case.
Whiteboard: [MemShrink]
|
|
Assignee | |
Comment 2•12 years ago
|
||
(By that, I mean run the CC logging stuff...)
|
||
Comment 3•12 years ago
|
||
Never turn down an offer to help, said a wise man once. :-)
Assignee: nobody → continuation
|
|
||
Comment 4•12 years ago
|
||
(FWIW, this _might_ have something to do with bug 814789.)
|
|
Assignee | |
Comment 5•12 years ago
|
||
Yeah, just looking at the patch I'd guess mAudioContexts needs to be added to a Traverse and Unlink function. ;)
|
|
||
Comment 6•12 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #5)
> Yeah, just looking at the patch I'd guess mAudioContexts needs to be added
> to a Traverse and Unlink function. ;)
Indeed. If I had a penny for every time I missed something this simple with the CC. :(
|
|
Assignee | |
Updated•12 years ago
|
Blocks: 814789
status-firefox19:
--- → unaffected
status-firefox20:
--- → affected
status-firefox21:
--- → affected
|
|
Assignee | |
Comment 7•12 years ago
|
||
This fixes the leak for me. I'm not sure what the right way to turn the test case into a Mochitest or crash test thing is.
|
|
Assignee | |
Comment 8•12 years ago
|
||
Oh, right, I should unlink it, too.
|
|
||
Comment 9•12 years ago
|
||
You want a mochitest for now so that you can enable the webaudio pref for now.
|
|
Assignee | |
Comment 10•12 years ago
|
||
The general rule of thumb is that if you add a ref counted field to a cycle collected class, you want to think very carefully about if you should add something to the Traverse and Unlink functions. :)
I reordered the fields a bit, but the only change should be that mAudioContexts gets added.
Attachment #701969 -
Attachment is obsolete: true
Attachment #701993 -
Flags: review?(ehsan)
|
|
||
Comment 11•12 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #10)
> Created attachment 701993 [details] [diff] [review]
> traverse+unlink mAudioContexts
>
> The general rule of thumb is that if you add a ref counted field to a cycle
> collected class, you want to think very carefully about if you should add
> something to the Traverse and Unlink functions. :)
Yeah, and people keep forgetting to do it. :( Which is why I filed bug 570416 which was duped against bug 423032 back in the day. Did that work ever get anywhere? Dehydra is obsolete as I understand things...
|
|
||
Comment 12•12 years ago
|
||
Comment on attachment 701993 [details] [diff] [review]
traverse+unlink mAudioContexts
Review of attachment 701993 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks again for taking this!
Attachment #701993 -
Flags: review?(ehsan) → review+
|
|
Assignee | |
Comment 13•12 years ago
|
||
> Did that work ever get anywhere? Dehydra is obsolete as I understand things...
I worked on it for awhile, but there were too many weird special cases to make it useful. It did find a few minor problems.
|
|
Assignee | |
Comment 14•12 years ago
|
||
|
|
||
Comment 15•12 years ago
|
||
Comment on attachment 698892 [details]
testcase
><!DOCTYPE html>
><html>
><head>
><script>
>
>function boom()
>{
> var iframe = document.createElementNS("http://www.w3.org/1999/xhtml", "iframe");
> document.body.appendChild(iframe);
> var frameWin = iframe.contentWindow;
> frameWin.mozAudioContext;
So I don't understand this part of the test case. Will this create a new mozAudioContext object? This just looks like a property access to me...
|
||
Comment 16•12 years ago
|
||
The testcase as-is doesn't seem to cause the AudioContext constructor to be called here. With () appended to this line, it works as expected.
I'm also super happy to report that this testcase is the first time my tool (bug 704240) was able to correctly report a leak (had to fix a couple bugs that this uncovered). Here's what I'm seeing right now in my about:refgraph page (of course, this is without the present patch applied):
type name: mozilla::dom::AudioContext
address: 0x7f99f6276560
size: 88 bytes
traversed edge
type name: mozilla::dom::AudioDestinationNode
address: 0x7f99f62776e0
size: 88 bytes
traversed edge
type name: mozilla::dom::AudioContext
address: 0x7f99f6276560
size: 88 bytes
traversed edge
type name: mozilla::dom::AudioDestinationNode
address: 0x7f99f62776e0
size: 88 bytes
traversed edge
type name: nsGlobalWindow
address: 0x7f99fabe3c40
size: 1224 bytes
traversed edge
type name: nsGlobalWindow
address: 0x7f99fabe3c40
size: 1224 bytes
NON-traversed edge
type name:
address: 0x7f99f3bc8940
size: 260 bytes
traversed edge
type name: nsPrincipal
address: 0x7f99f3cfeb00
size: 80 bytes
NON-traversed edge
type name: nsStandardURL
address: 0x7f99f9722d40
size: 264 bytes
NON-traversed edge
type name:
address: 0x7f99f7524720
size: 24 bytes
NON-traversed edge
type name: mozilla::dom::AudioContext
address: 0x7f99f6276560
size: 88 bytes
*** CYCLE NOT TRAVERSED BY THE CC! ***
(snipped.)
|
|
Assignee | |
Comment 17•12 years ago
|
||
> I'm also super happy to report that this testcase is the first time my tool (bug 704240) was able to correctly report a leak
Nice!
|
|
||
Comment 18•12 years ago
|
||
I pushed a follow-up to fix the test case, as I'm pretty sure that it's wrong.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b698ca72776
|
|
Assignee | |
Comment 19•12 years ago
|
||
Well, I just cut and pasted Jesse's test case. Hopefully the new one also leaks without the patch. ;)
|
|
||
Comment 20•12 years ago
|
||
Comment 16 says it does at least create a non-cycle-collected cycle ;-)
|
||
Comment 21•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0ff42bab2f66
https://hg.mozilla.org/mozilla-central/rev/7b698ca72776
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
|
||
Updated•12 years ago
|
|
|
Assignee | |
Comment 22•12 years ago
|
||
Comment on attachment 701993 [details] [diff] [review]
traverse+unlink mAudioContexts
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 814789
User impact if declined: remote chance of leaks, but probably nothing without the webaudio pref turned on.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none
Attachment #701993 -
Flags: approval-mozilla-aurora?
|
||
Comment 23•12 years ago
|
||
Comment on attachment 701993 [details] [diff] [review]
traverse+unlink mAudioContexts
Low risk, comes with test, approving since it's so early in Aurora.
Attachment #701993 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
|
|
||
Updated•12 years ago
|
Keywords: checkin-needed
Whiteboard: [MemShrink] → [MemShrink][land-on-aurora]
|
|
||
Updated•12 years ago
|
Keywords: checkin-needed
|
||
Comment 24•12 years ago
|
||
Whiteboard: [MemShrink][land-on-aurora] → [MemShrink]
|
|
||
Comment 25•12 years ago
|
||
Flags: in-testsuite+
Keywords: regression
|
|
||
Comment 26•11 years ago
|
||
Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.
Component: Video/Audio → Web Audio
You need to log in
before you can comment on or make changes to this bug.
Description
•