898906 - Leak with expando on Plugin's MimeType object

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Running with XPCOM_MEM_LEAK_LOG=2 shows leaked nsDocument and nsGlobalWindow.

Maybe a regression from bug 896242?
http://hg.mozilla.org/mozilla-central/rev/f9e89a88f017

Comment 1

[Andrew McCreight [:mccr8]]

Seems unlikely, but stranger things have happened.  This code has changed a bit in the last month or so, so I could believe that something is still wrong here.

Comment 2

[Benjamin Smedberg]

Plugin MIME type objects, how much don't I love thee :-(

Comment 3

[Jesse Ruderman]

Now it asserts too: bug 899270.

Comment 4

[Andrew McCreight [:mccr8]]

Oops, bad reviewer!  I missed that jst unrolled the definition of the macro incorrectly so we failed to traverse the array.  jst has a patch that will fix that and the invalidate behavior.

Comment 5

[Johnny Stenback (:jst)]

Attachment: diff

This fixes the leak and also makes the enabledPlugin property of mimetype objects be deterministic and independent of GC/CC timing. This adds an explicit reference cycle from nsPluginElement to its nsMimeType objects back to the nsPluginElement, and relies on the cycle collector to clean that up. This should also be an alternative (and better, less buggy if nothing else) fix for bug 896242, which introduced this leak.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 782973 [details] [diff] [review]
diff

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

Looks good.  Please add Jesse's test case as a crash test or whatever.

::: dom/base/nsMimeTypeArray.h
@@ +87,5 @@
>  protected:
>    nsWeakPtr mWindow;
>  
> +  // Strong reference to the active plugin, if any. Note that this
> +  // creates an explicit reference cycle through the plugin elements

nit: element's

@@ +88,5 @@
>    nsWeakPtr mWindow;
>  
> +  // Strong reference to the active plugin, if any. Note that this
> +  // creates an explicit reference cycle through the plugin elements
> +  // mimetype array, we rely on the cycle collector to break this

nit: "array.  We" not "array, we".

Comment 7

[Johnny Stenback (:jst)]

Attachment: Fix, with test and nits addressed.

Comment 8

[Johnny Stenback (:jst)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/018ef921c945

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/018ef921c945

Comment 10

[Ioana (away)]

Attachment: about:bloat on 09/30 Firefox 25 biuld

Comment 11

[Ioana (away)]

When testing with the latest Firefox 25 debug (ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-10-01-mozilla-beta-debug) and XPCOM_MEM_LEAK_LOG=2, I still get those leaks with the attached testcase - see my attachment.

Please let me know if there's anything else you need tested here.

Comment 12

[Jesse Ruderman]

Ioana, can you check with the shutdown leak log (printed on stderr) instead of about:bloat?  It's hard to tell whether there's a leak from about:bloat.

Comment 13

[Ioana (away)]

(In reply to Jesse Ruderman from comment #12)
> Ioana, can you check with the shutdown leak log (printed on stderr) instead
> of about:bloat?  It's hard to tell whether there's a leak from about:bloat.

I tried to check with the shutdown log, but I don't seem to understand if there are leaks there or not. By the way, I couldn't get data in this log without also using the trace-malloc option. Is that expected?

Here's the shutdown leak log: https://docs.google.com/file/d/0B4LS2jzcUzsHSlNRRERWMjZRQTg/edit?usp=drive_web
nsGlobalWindow doesn't appear here at all, but the nsDocument Controller does appear several times. Jesse can you take a look? Or, better yet, explain how I can verify this fix with this log?

All I did was open Firefox, load the attached testcase, then quit Firefox (Firefox 25 beta from comment 11 on Mac OSX 10.7.5).

Comment 14

[Jesse Ruderman]

I don't know how to read trace-malloc logs, but you shouldn't need trace-malloc to reproduce this bug.

You might find it easier to experiment with trace-refcnt using bug 914015, and then you can come back to this bug.  If you still need help getting trace-refcnt working, catch me on IRC (Jesse in #memshrink).

Comment 15

[Ioana (away)]

(In reply to Jesse Ruderman from comment #14)
> I don't know how to read trace-malloc logs, but you shouldn't need
> trace-malloc to reproduce this bug.
> 
> You might find it easier to experiment with trace-refcnt using bug 914015,
> and then you can come back to this bug.  If you still need help getting
> trace-refcnt working, catch me on IRC (Jesse in #memshrink).

With these logs it's very easily noticeable that I reproduce the bug on the 07/27 debug nightly build and I get no leaks with the 10/08 debug beta build.

Thanks for all the help Jesse!