826526 - Lots of dark matter under nsHtml5StringParser::ParseFragment

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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 Long Service Leave)]

Attachment: 9 records

Here are the 9 records.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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 Long Service Leave)]

> 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 Long Service Leave)]

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 Long Service Leave)]

Attachment: 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 [:bzbarsky]]

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 Long Service Leave)]

> 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 Long Service Leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48abe27b73b7

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/48abe27b73b7

Comment 10

[Justin Lebar (not reading bugmail)]

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]]

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]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ed7a115e5501