Closed Bug 898906 Opened 6 years ago Closed 6 years ago

Leak with expando on Plugin's MimeType object

Categories

(Core :: Plug-ins, defect)

x86_64
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox24 --- unaffected
firefox25 --- verified

People

(Reporter: jruderman, Assigned: jst)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, regression, testcase, Whiteboard: [MemShrink])

Attachments

(3 files, 1 obsolete file)

Attached file 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]
Plugin MIME type objects, how much don't I love thee :-(
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
Attached patch diff (obsolete) — Splinter Review
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+
Attachment #782973 - Attachment is obsolete: true
Attachment #783464 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/018ef921c945
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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
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)
(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)
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)
(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
Flags: needinfo?(ioana.budnar)
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.