Lots of dark matter under nsHtml5StringParser::ParseFragment

RESOLVED FIXED in Firefox 20

Status

()

Core
HTML: Parser
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(firefox20 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 697709 [details]
9 records

Here are the 9 records.
Whiteboard: [MemShrink]
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.
(Assignee)

Comment 3

5 years ago
> 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.
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
Attachment #697802 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Assignee: nobody → n.nethercote
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.
Attachment #697802 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

5 years ago
> 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.
(Assignee)

Comment 8

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

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/48abe27b73b7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Updated

5 years ago
Blocks: 829427
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
Attachment #697802 - Flags: approval-mozilla-b2g18?
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.
Attachment #697802 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ed7a115e5501
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox20: --- → fixed
You need to log in before you can comment on or make changes to this bug.