1. Load a page containing <marquee> 2. Load about:memory Result: ###!!! ASSERTION: aRoot not an ancestor of |this|?: 'cur', file nsINode.h, line 1187 followed by a crash [@ xpc::OrphanReporter::sizeOfIncludingThis]

Nightly: bp-16741166-df44-4161-8dc2-935fd2120713

I'm gonna need help from DOM people with this one. The relevant code in XPCJSRuntime.cpp: static size_t SizeOfTreeIncludingThis(nsINode *tree) { size_t n = tree->SizeOfIncludingThis(OrphanSizeOf); for (nsIContent* child = tree->GetFirstChild(); child; child = child->GetNextNode(tree)) { n += child->SizeOfIncludingThis(OrphanSizeOf); } return n; } and the parameter passed is gotten with SubtreeRoot(): nsCOMPtr<nsINode> orphanTree = node->SubtreeRoot(); Looks unexpectional to me. But we get the assertion failures, and somehow end up in nsINode::GetNextNodeImpl with this==NULL. Is there something special about marquee nodes?

<marquee> is implemented using XBL, which makes it "special".

On Windows: bp-2055f67d-b780-4900-8abe-d7b342120713

Maybe this is a problem with SubtreeRoot not handling XBL properly?

It's not clear that this could happen in normal user scenarios (which loading about:memory is not one of). Do we suspect that this will affect normal use cases?

At one point we were accidentally calling this kind of code once a minute, but I think that's been stopped. So I think it shouldn't be called unless somebody loads about:memory.

We should also check HTML5 video controls, with are also XBLy.

> So I think it shouldn't be called unless somebody loads about:memory. That's correct.

(In reply to Andrew McCreight [:mccr8] from comment #8) > We should also check HTML5 video controls, with are also XBLy. It crashes. Steps to reproduce: - Visit www.youtube.com/html5 to enable WebM - Visit http://www.youtube.com/watch?v=X3F3KmZDtyg - Open about:memory in another tab - Assert with "ASSERTION: aRoot not an ancestor of |this|?: 'cur', file ../../../dist/include/nsINode.h, line 1187", then crash immediately afterwards

So when we hit the assert, aRoot is the <xbl:content>. |this| is an HTMLDivElement. What you're running into is that XBL plays fast and loose with its anonymous content. In particular, if you look at nsXBLBinding::InstallAnonymousContent what it's doing is taking all the kids and rebinding them to a new parent, while still keeping them in the child list of the old parent. The comments say it all. The result is that for the special case when "node" is the <xbl:content> clone, node->GetFirstChild()->GetParent() != node. And then you lose. This doesn't bite people normally because normally there's no way for anyone who might try walking a DOM to actually get their hands on the <xbl:content>. I'm a little surprised, and really worried, that in this case it looks like the <xbl:content> is JS-wrapped... I wonder when that happens.

> - Visit http://www.youtube.com/watch?v=X3F3KmZDtyg http://www.youtube.com/watch?v=X3F3KmZDtyg&webm=1 forces the WebM version directly. bz: your diagnosis is spot on, the attached patch skips any node which violates |node->GetFirstChild()->GetParent() == node| and the crashes go away for both <marquee> and native video controls. It would be nice to fix this in a less crappy way, though.

XBL is a little weird. It might be good to just skip it anyways. IIUC we don't really deal with anonymous content otherwise in the memory reporter.

I would _really_ like to understand why that <xbl:content> has a JS wrapper at all. Possibly as a followup....

(In reply to Andrew McCreight [:mccr8] from comment #13) > XBL is a little weird. It might be good to just skip it anyways. IIUC we > don't really deal with anonymous content otherwise in the memory reporter. I'm ok with that, but it would also be nicer if the condition was |!isXBL(node)| or somesuch, rather than playing that child/parent game. Any suggestions on how to do that? And I sympathize with bz's concerns, too.

(In reply to Boris Zbarsky (:bz) from comment #14) > I would _really_ like to understand why that <xbl:content> has a JS wrapper > at all. Possibly as a followup.... Just to be clear here, the root (<xbl:content>) isn't necessarily the wrapped node: we find wrapped nodes and then get their SubTreeRoots() to get the list of roots. But maybe you noticed that it is in this case, in which case please ignore me... (In reply to Nicholas Nethercote [:njn] from comment #15) > I'm ok with that, but it would also be nicer if the condition was > |!isXBL(node)| or somesuch, rather than playing that child/parent game. Any > suggestions on how to do that? Yes, there are better ways, but I don't know how available they will be in XPConnect land. The UnoptimizableCCNode() method could do in a pinch, and should be a conservative approximation. It may be possible to just dig out some subpart of that, I guess.

Yeah, the wrapped node was in fact the xbl:content if I didn't mess up with gdb. And there should be nothing whose parent chain terminates in that node, so it wouldn't be the subtree root of anything, I think.

This also appears to be the reason areweslimyet tests have been failing since the 10th - The workaround patch seems to revive them

This patch looks for XBL nodes in a nicer fashion, and still prevents the crashes.

Ehsan, what's happening in this bug is that the presence of certain HTML elements (e.g. <marquee>) were causing the memory reporters (that it runs in the <script> element) to crash. This patch adds a <marquee> element to the <body> of the test_memoryReporters.xul test, so that when the memory reporters are run, this path is hit. Is it ok to put content in the <body> of a mochitest like this? I confirmed that it triggers the crash. Thanks!

Once this fix lands, we'll need to back-port it to Aurora 16, or back out the original patch.

Comment on attachment 643211 [details] [diff] [review] (part 2) - Add a <marquee> to test_memoryReporters.xul, to test for the crash in bug 773533. Review of attachment 643211 [details] [diff] [review]: ----------------------------------------------------------------- marquee fun \o/

Comment on attachment 643210 [details] [diff] [review] (part 1) - Skip XBL nodes when looking for orphan DOM nodes. Review of attachment 643210 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +13,5 @@ > #include "XPCJSMemoryReporter.h" > #include "WrapperFactory.h" > #include "dom_quickstubs.h" > > +#include "Element.h" I don't know if Element is the right thing to include here, but maybe it is. @@ +1709,5 @@ > virtual size_t sizeOfIncludingThis(void *aSupports) > { > size_t n = 0; > nsCOMPtr<nsINode> node = do_QueryInterface(static_cast<nsISupports*>(aSupports)); > + nsCOMPtr<nsINode> child; It doesn't look like child is used.

It looks okay to me, but the particulars of this stuff are hazy to me, so Olli should probably look at it. (In reply to Nicholas Nethercote [:njn] from comment #21) > Once this fix lands, we'll need to back-port it to Aurora 16, or back out > the original patch. I can shepherd this patch through to Aurora if that happens while you are on vacation.

> I don't know if Element is the right thing to include here, but maybe it is. Well, it won't compile without it :)

Comment on attachment 643210 [details] [diff] [review] (part 1) - Skip XBL nodes when looking for orphan DOM nodes. We can take this, but please file a followup for the issue mentioned in comment 14.

For the record if your view an XML document then view about:memory it will usually crash.

https://hg.mozilla.org/integration/mozilla-inbound/rev/847116c6177c https://hg.mozilla.org/integration/mozilla-inbound/rev/28e6cfc08efc

(In reply to Hugh Nougher [:Hughman] from comment #27) > For the record if your view an XML document then view about:memory it will > usually crash. AFAICT, the patches I just landed fixed this. I tested with http://www.w3.org/2000/10/swap/test/plist/iTunesMusicLibrary.xml.

Comment on attachment 643210 [details] [diff] [review] (part 1) - Skip XBL nodes when looking for orphan DOM nodes. [Approval Request Comment] Bug caused by (feature/regressing bug #): Addition of orphan DOM node measurement for about:memory (bug 704623). User impact if declined: Visiting about:memory crashes the browser if pages with certain content are loaded, e.g. <marquee>, XML, native video. Testing completed (on m-c, etc.): Green on try server, and I just landed on mozilla-inbound. Risk to taking this patch (and alternatives if risky): Minimal; it's a tiny change to the memory reporter that skips problematic DOM nodes. If this patch isn't approved, then the patch from bug 704623 needs to be backed out. String or UUID changes made by this patch: None.

https://hg.mozilla.org/mozilla-central/rev/847116c6177c https://hg.mozilla.org/mozilla-central/rev/28e6cfc08efc

Comment on attachment 643210 [details] [diff] [review] (part 1) - Skip XBL nodes when looking for orphan DOM nodes. Review of attachment 643210 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/Makefile.in @@ +64,5 @@ > LOCAL_INCLUDES = \ > -I$(srcdir)/../wrappers \ > -I$(srcdir)/../loader \ > -I$(topsrcdir)/caps/include \ > + -I$(topsrcdir)/content/base/public \ This is wrong. ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +13,5 @@ > #include "XPCJSMemoryReporter.h" > #include "WrapperFactory.h" > #include "dom_quickstubs.h" > > +#include "Element.h" You should include "mozilla/dom/Element.h".

> > + -I$(topsrcdir)/content/base/public \ > > This is wrong. What's the right way to do it?

The right way is to include the exported version of the header, which is "mozilla/dom/Element.h".

We don't need to track this regression for release specifically, as we'd likely ship with this in the product if necessary. But we will approve for FF16 given how early we are in the cycle, and the low risk evaluation.

njn is on vacation, so he said I could fix it up and land it on branches.

Comment on attachment 645160 [details] [diff] [review] part 3: fix the imports rs=me; I wouldn't worry about branches.

(In reply to :Ms2ger from comment #37) > rs=me; I wouldn't worry about branches. Thanks! I actually meant for the original patch, so I might as well include this. https://hg.mozilla.org/integration/mozilla-inbound/rev/12c101f0ed10

https://hg.mozilla.org/releases/mozilla-aurora/rev/6811f9bc2f93 https://hg.mozilla.org/releases/mozilla-aurora/rev/aea5bfa1f283 https://hg.mozilla.org/releases/mozilla-aurora/rev/aeb14ddf1fc0

Thanks for fixing the #include and landing the patches on Aurora, mccr8!

Able to see the crash on Nightly 2012-07-12-mozilla-central-debug build. Verified fixed on FF 16 2012-09-11-mozilla-beta-debug build on Win 7 x64, Ubuntu 12.04 and Mac OS X 10.8

Verified fixed on FF 17 2012-10-15-mozilla-beta-debug build on Win 7 x64, Ubuntu 12.04 and Mac OS X 10.7.4.