XBL elements (e.g. <marquee>, HTML5 video controls) make about:memory crash, possibly due to nsINode::SubtreeRoot() being defective?

VERIFIED FIXED in Firefox 16

Status

()

Core
DOM
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

(Blocks: 1 bug, 4 keywords)

16 Branch
mozilla17
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16- verified, firefox17 verified)

Details

(crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 641712 [details]
A page containing <marquee>

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]
(Reporter)

Comment 1

5 years ago
Created attachment 641715 [details]
stack trace

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?
(Reporter)

Comment 3

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

Comment 4

5 years ago
On Windows: bp-2055f67d-b780-4900-8abe-d7b342120713
Crash Signature: [@ xpc::OrphanReporter::sizeOfIncludingThis ] → [@ xpc::OrphanReporter::sizeOfIncludingThis ] [@ nsINode::GetNextNodeImpl(nsINode const*, bool)]
OS: Mac OS X → All
Hardware: x86_64 → All
Version: Trunk → 16 Branch
Maybe this is a problem with SubtreeRoot not handling XBL properly?
(Reporter)

Updated

5 years ago
tracking-firefox16: --- → ?

Comment 6

5 years ago
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
Summary: <marquee> makes about:memory crash → XBL elements (e.g. <marquee>, HTML5 video controls) make about:memory crash, possibly due to nsINode::SubtreeRoot() being defective?
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.
Blocks: 687724
Created attachment 642813 [details] [diff] [review]
work-around

> - 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
Created attachment 643210 [details] [diff] [review]
(part 1) - Skip XBL nodes when looking for orphan DOM nodes.

This patch looks for XBL nodes in a nicer fashion, and still prevents the crashes.
Attachment #643210 - Flags: review?(continuation)
Created attachment 643211 [details] [diff] [review]
(part 2) - Add a <marquee> to test_memoryReporters.xul, to test for the crash in bug 773533.

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!
Attachment #643211 - Flags: review?(ehsan)
Attachment #642813 - Attachment is obsolete: true
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/
Attachment #643211 - Flags: review?(ehsan) → review+
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.
Attachment #643210 - Flags: review?(continuation)
Attachment #643210 - Flags: review?(bugs)
Attachment #643210 - Flags: feedback+
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.
Attachment #643210 - Flags: review?(bugs) → review+
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.
Attachment #643210 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/847116c6177c
https://hg.mozilla.org/mozilla-central/rev/28e6cfc08efc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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.
tracking-firefox16: ? → -

Updated

5 years ago
Attachment #643210 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox16: --- → affected
status-firefox17: --- → fixed
Created attachment 645160 [details] [diff] [review]
part 3: fix the imports

njn is on vacation, so he said I could fix it up and land it on branches.
Attachment #645160 - Flags: review?(Ms2ger)
Comment on attachment 645160 [details] [diff] [review]
part 3: fix the imports

rs=me; I wouldn't worry about branches.
Attachment #645160 - Flags: review?(Ms2ger) → review+
(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
status-firefox16: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/12c101f0ed10
Thanks for fixing the #include and landing the patches on Aurora, mccr8!
No longer blocks: 687724
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
status-firefox16: fixed → verified
QA Contact: paul.silaghi
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
status-firefox17: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.