Last Comment Bug 826526 - Lots of dark matter under nsHtml5StringParser::ParseFragment
: Lots of dark matter under nsHtml5StringParser::ParseFragment
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla20
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on:
Blocks: 829427
  Show dependency treegraph
 
Reported: 2013-01-03 15:56 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2013-03-28 07:40 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
wontfix
wontfix


Attachments
9 records (35.55 KB, text/plain)
2013-01-03 15:57 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
no flags Details
Modify the orphan node reporter so it handles WebIDL objects. (6.81 KB, patch)
2013-01-03 22:23 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
bzbarsky: review+
akeybl: approval‑mozilla‑b2g18+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2013-01-03 15:56:21 PST
I did an 8 hour DMD run and 9 of the top 20 unreported stack trace records
involved nsHtml5StringParser::ParseFragment and related functions.  Here's the
biggest of these records:

> Unreported: ~342 blocks in stack trace record 2 of 4,082
>  ~1,399,806 bytes (~1,399,806 requested / ~0 slop)
>  0.47% of the heap (1.17% cumulative);  2.61% of unreported (6.53% cumulative)
>  Allocated at
>    moz_xmalloc (/home/njn/moz/mi0/memory/mozalloc/mozalloc.cpp:54) 0x7f0e25988058
>    operator new(unsigned long) (/home/njn/moz/mi0/dmdo64/content/base/src/../../../dist/include/mozilla/mozalloc.h:200) 0x7f0e21c309eb
>    nsHtml5TreeOperation::AppendText(unsigned short const*, unsigned int, nsIContent*, nsHtml5TreeOpExecutor*) (/home/njn/moz/mi0/parser/html/nsHtml5TreeOperation.cpp:166) 0x7f0e21fae7be
>    nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) (/home/njn/moz/mi0/parser/html/nsHtml5TreeOperation.cpp:511) 0x7f0e21fb0261
>    nsHtml5TreeOpExecutor::FlushDocumentWrite() (/home/njn/moz/mi0/parser/html/nsHtml5TreeOpExecutor.cpp:654) 0x7f0e21fb264b
>    nsHtml5StringParser::Tokenize(nsAString_internal const&, nsIDocument*, bool) (/home/njn/moz/mi0/parser/html/nsHtml5StringParser.cpp:130) 0x7f0e21fb8f61
>    nsHtml5StringParser::ParseFragment(nsAString_internal const&, nsIContent*, nsIAtom*, int, bool, bool) (/home/njn/moz/mi0/parser/html/nsHtml5StringParser.cpp:65) 0x7f0e21fb8d98
>    nsContentUtils::ParseFragmentHTML(nsAString_internal const&, nsIContent*, nsIAtom*, int, bool, bool) (/home/njn/moz/mi0/content/base/src/nsContentUtils.cpp:4167) 0x7f0e21babc40
>    mozilla::dom::Element::SetInnerHTML(nsAString_internal const&, mozilla::ErrorResult&) (/home/njn/moz/mi0/content/base/src/Element.cpp:3399) 0x7f0e21bfb78a
>    mozilla::dom::ElementBinding::set_innerHTML(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JS::Value*) (/home/njn/moz/mi0/dmdo64/dom/bindings/ElementBinding.cpp:1562) 0x7f0e2263337c
>    mozilla::dom::ElementBinding::genericSetter(JSContext*, unsigned int, JS::Value*) (/home/njn/moz/mi0/dmdo64/dom/bindings/ElementBinding.cpp:1896) 0x7f0e2263297b
>    js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (/home/njn/moz/mi0/js/src/jsinterp.cpp:379) 0x7f0e22ce9174
>    js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) (/home/njn/moz/mi0/js/src/jsinterp.h:112) 0x7f0e22c8cc96

Everything from Tokenize() down was the same in most of these records.
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-01-03 15:57:48 PST
Created attachment 697709 [details]
9 records

Here are the 9 records.
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-01-03 18:38:00 PST
That's really weird.

The CreateHTMLElement, for example, is clearly allocating elements... and I thought we memory-reported those!

Most of the rest of those stacks are also DOM nodes being allocated.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-01-03 20:20:27 PST
> I thought we memory-reported those!

We worked out on IRC that these are probably orphaned WebIDL objects, which the current OrphanReporter doesn't catch, because they don't use the JSObject private field.
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-01-03 20:21:31 PST
Note to self:  XPCConvert::GetISupportsFromJSObject is likely to be involved when fixing this -- it handles objects that use the private slot and those that don't.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-01-03 22:23:41 PST
Created attachment 697802 [details] [diff] [review]
Modify the orphan node reporter so it handles WebIDL objects.

This turned out to be fairly straightforward.  I haven't tested it much;  I'll
do that with a long-running browser session on Monday.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-01-03 23:05:52 PST
Comment on attachment 697802 [details] [diff] [review]
Modify the orphan node reporter so it handles WebIDL objects.

>+    typedef JSBool(*GetISupportsFun)(JSObject *obj, nsISupports **iface);

This should probably document that it does NOT addref the pointer.

r=me with that.
Comment 7 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-01-06 20:27:46 PST
> I haven't tested it much; I'll
> do that with a long-running browser session on Monday.

I just finished.  Gmail has 3.7 MiB of orphan nodes, and they're showing up now.  Good to go.
Comment 8 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-01-06 20:45:36 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/48abe27b73b7
Comment 9 Ed Morley [:emorley] 2013-01-07 08:25:02 PST
https://hg.mozilla.org/mozilla-central/rev/48abe27b73b7
Comment 10 Justin Lebar (not reading bugmail) 2013-03-26 08:29:45 PDT
Comment on attachment 697802 [details] [diff] [review]
Modify the orphan node reporter so it handles WebIDL objects.

[Approval Request Comment]
* Bug caused by (feature/regressing bug #): n/a
* User impact if declined: This may be making it difficult for us to debug memory leaks in Gaia apps, e.g. bug 850803.  It will also make it more difficult for us to debug memory leaks in apps that third parties write.
* Testing completed: on m-c for a while
* Risk to taking this patch (and alternatives if risky): Everyone has to use DMD, which ends up increasing debugging times by many days.
* String or UUID changes made by this patch: n/a
Comment 11 Alex Keybl [:akeybl] 2013-03-27 13:23:03 PDT
Comment on attachment 697802 [details] [diff] [review]
Modify the orphan node reporter so it handles WebIDL objects.

Originally landed in Firefox 20 (about to be released), and this change is cross platform (thus well tested). Approving to help devs.
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-03-28 07:40:53 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ed7a115e5501

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