Leak with expando on Plugin's MimeType object

VERIFIED FIXED in Firefox 25

Status

()

Core
Plug-ins
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: jst)

Tracking

(Blocks: 1 bug, {memory-leak, regression, testcase})

Trunk
mozilla25
x86_64
Mac OS X
memory-leak, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 unaffected, firefox25 verified)

Details

(Whiteboard: [MemShrink])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 782309 [details]
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
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.
Whiteboard: [MemShrink]

Comment 2

5 years ago
Plugin MIME type objects, how much don't I love thee :-(
(Reporter)

Comment 3

5 years ago
Now it asserts too: bug 899270.
Assignee: nobody → continuation
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.
Assignee: continuation → jst
status-firefox24: --- → unaffected
status-firefox25: --- → affected
(Assignee)

Comment 5

5 years ago
Created attachment 782973 [details] [diff] [review]
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.
Attachment #782973 - Flags: review?(continuation)
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".
Attachment #782973 - Flags: review?(continuation) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 783464 [details] [diff] [review]
Fix, with test and nits addressed.
Attachment #782973 - Attachment is obsolete: true
Attachment #783464 - Flags: review+

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/018ef921c945
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

5 years ago
status-firefox25: affected → fixed
Keywords: verifyme

Comment 10

5 years ago
Created attachment 813075 [details]
about:bloat on 09/30 Firefox 25 biuld

Comment 11

5 years ago
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.
QA Contact: ioana.budnar
(Reporter)

Comment 12

5 years ago
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.
Flags: needinfo?(ioana.budnar)

Comment 13

5 years ago
(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).
Flags: needinfo?(ioana.budnar) → needinfo?(jruderman)
(Reporter)

Comment 14

5 years ago
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).
Flags: needinfo?(jruderman) → needinfo?(ioana.budnar)

Comment 15

5 years ago
(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!
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
Flags: needinfo?(ioana.budnar)
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.