Last Comment Bug 827541 - Leak calling AudioContext on removed iframe
: Leak calling AudioContext on removed iframe
Status: RESOLVED FIXED
[MemShrink]
: mlk, regression, testcase
Product: Core
Classification: Components
Component: Web Audio (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla21
Assigned To: Andrew McCreight [:mccr8]
:
: Maire Reavy [:mreavy] Please needinfo me
Mentors:
Depends on:
Blocks: 326633 594645 webaudio 814789
  Show dependency treegraph
 
Reported: 2013-01-07 15:06 PST by Jesse Ruderman
Modified: 2013-06-05 07:11 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed


Attachments
testcase (374 bytes, text/html)
2013-01-07 15:06 PST, Jesse Ruderman
no flags Details
traverse mAudioContexts (1.57 KB, patch)
2013-01-14 14:16 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
traverse+unlink mAudioContexts (4.50 KB, patch)
2013-01-14 14:42 PST, Andrew McCreight [:mccr8]
ehsan: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2013-01-07 15:06:53 PST
Created attachment 698892 [details]
testcase

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.
Comment 1 User image Andrew McCreight [:mccr8] 2013-01-07 15:38:32 PST
I can do some initial analysis of what is happening here, if you'd like, Ehsan. Just assign it to me in that case.
Comment 2 User image Andrew McCreight [:mccr8] 2013-01-07 15:38:48 PST
(By that, I mean run the CC logging stuff...)
Comment 3 User image :Ehsan Akhgari 2013-01-07 16:16:54 PST
Never turn down an offer to help, said a wise man once.  :-)
Comment 4 User image :Ehsan Akhgari 2013-01-07 16:18:00 PST
(FWIW, this _might_ have something to do with bug 814789.)
Comment 5 User image Andrew McCreight [:mccr8] 2013-01-07 16:23:03 PST
Yeah, just looking at the patch I'd guess mAudioContexts needs to be added to a Traverse and Unlink function. ;)
Comment 6 User image :Ehsan Akhgari 2013-01-07 17:10:33 PST
(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.  :(
Comment 7 User image Andrew McCreight [:mccr8] 2013-01-14 14:16:50 PST
Created attachment 701969 [details] [diff] [review]
traverse mAudioContexts

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.
Comment 8 User image Andrew McCreight [:mccr8] 2013-01-14 14:20:21 PST
Oh, right, I should unlink it, too.
Comment 9 User image :Ehsan Akhgari 2013-01-14 14:30:47 PST
You want a mochitest for now so that you can enable the webaudio pref for now.
Comment 10 User image Andrew McCreight [:mccr8] 2013-01-14 14:42:21 PST
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. :)

I reordered the fields a bit, but the only change should be that mAudioContexts gets added.
Comment 11 User image :Ehsan Akhgari 2013-01-14 14:53:17 PST
(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 User image :Ehsan Akhgari 2013-01-14 14:53:50 PST
Comment on attachment 701993 [details] [diff] [review]
traverse+unlink mAudioContexts

Review of attachment 701993 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again for taking this!
Comment 13 User image Andrew McCreight [:mccr8] 2013-01-14 15:21:14 PST
> 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.
Comment 14 User image Andrew McCreight [:mccr8] 2013-01-14 15:22:50 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff42bab2f66
Comment 15 User image :Ehsan Akhgari 2013-01-14 19:34:50 PST
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 User image Benoit Jacob [:bjacob] (mostly away) 2013-01-14 20:15:45 PST
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.)
Comment 17 User image Andrew McCreight [:mccr8] 2013-01-14 20:23:24 PST
> 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 User image :Ehsan Akhgari 2013-01-14 22:15:53 PST
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
Comment 19 User image Andrew McCreight [:mccr8] 2013-01-14 22:20:46 PST
Well, I just cut and pasted Jesse's test case. Hopefully the new one also leaks without the patch. ;)
Comment 20 User image Benoit Jacob [:bjacob] (mostly away) 2013-01-15 04:10:06 PST
Comment 16 says it does at least create a non-cycle-collected cycle ;-)
Comment 22 User image Andrew McCreight [:mccr8] 2013-01-16 05:50:28 PST
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
Comment 23 User image Lukas Blakk [:lsblakk] use ?needinfo 2013-01-16 12:35:24 PST
Comment on attachment 701993 [details] [diff] [review]
traverse+unlink mAudioContexts

Low risk, comes with test, approving since it's so early in Aurora.
Comment 24 User image Ryan VanderMeulen [:RyanVM] 2013-01-19 21:16:49 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b5cbc7b817c
Comment 25 User image Ryan VanderMeulen [:RyanVM] 2013-01-19 21:17:18 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e248e6bd617
Comment 26 User image :Ehsan Akhgari 2013-06-05 07:11:38 PDT
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.

Note You need to log in before you can comment on or make changes to this bug.