Closed Bug 826526 Opened 8 years ago Closed 8 years ago
Lots of dark matter under ns
Html5String Parser::Parse Fragment
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.
Here are the 9 records.
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.
> 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.
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.
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)
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+
> 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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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+
You need to log in before you can comment on or make changes to this bug.