Bug 44129 – Crash in WebCore::Node::createRendererIfNeeded()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tony Gentilcore]

This was discovered in Chromium 6.0.472.1. Most likely introduced in r64970-65072 (perhaps r65006?).

Reproduces on http://fifax.net/ with AdBlock extension installed. I'm trying to reduce a test case that will demonstrate this without AdBlock.

http://code.google.com/p/chromium/issues/detail?id=52377

Thread 0 *CRASHED* ( EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000020 )
0x565e17e2 	[chrome.dll 	- node.cpp:1399] 	WebCore::Node::createRendererIfNeeded()
0x565f8d21 	[chrome.dll 	- element.cpp:815] 	WebCore::Element::attach()
0x566fe2cf 	[chrome.dll 	- htmlframeelementbase.cpp:212] 	WebCore::HTMLFrameElementBase::attach()
0x567f893c 	[chrome.dll 	- htmlconstructionsite.cpp:129] 	WebCore::HTMLConstructionSite::attachAtSite(WebCore::HTMLConstructionSite::AttachmentSite const &,WTF::PassRefPtr<WebCore::Node>)
0x567f9192 	[chrome.dll 	- htmlconstructionsite.cpp:445] 	WebCore::HTMLConstructionSite::fosterParent(WebCore::Node *)
0x567f91bd 	[chrome.dll 	- htmlconstructionsite.cpp:95] 	WebCore::HTMLConstructionSite::attach<WebCore::Comment>(WebCore::Node *,WTF::PassRefPtr<WebCore::Comment>)
0x567f8b9d 	[chrome.dll 	- htmlconstructionsite.cpp:219] 	WebCore::HTMLConstructionSite::attachToCurrent(WTF::PassRefPtr<WebCore::Element>)
0x567f8c81 	[chrome.dll 	- htmlconstructionsite.cpp:252] 	WebCore::HTMLConstructionSite::insertHTMLElement(WebCore::AtomicHTMLToken &)
0x567c4fe2 	[chrome.dll 	- htmltreebuilder.cpp:2811] 	WebCore::HTMLTreeBuilder::processGenericRawTextStartTag(WebCore::AtomicHTMLToken &)
0x567c24b6 	[chrome.dll 	- htmltreebuilder.cpp:987] 	WebCore::HTMLTreeBuilder::processStartTagForInBody(WebCore::AtomicHTMLToken &)
0x567c2960 	[chrome.dll 	- htmltreebuilder.cpp:1168] 	WebCore::HTMLTreeBuilder::processStartTagForInTable(WebCore::AtomicHTMLToken &)
0x567c3126 	[chrome.dll 	- htmltreebuilder.cpp:1276] 	WebCore::HTMLTreeBuilder::processStartTag(WebCore::AtomicHTMLToken &)
0x567c18ba 	[chrome.dll 	- htmltreebuilder.cpp:529] 	WebCore::HTMLTreeBuilder::processToken(WebCore::AtomicHTMLToken &)
0x56750bb6 	[chrome.dll 	- htmldocumentparser.cpp:172] 	WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
0x56750aca 	[chrome.dll 	- htmldocumentparser.cpp:127] 	WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)
0x56750ed9 	[chrome.dll 	- htmldocumentparser.cpp:351] 	WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution()
0x56750f62 	[chrome.dll 	- htmldocumentparser.cpp:387] 	WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource *)
0x5675898b 	[chrome.dll 	- cachedscript.cpp:99] 	WebCore::CachedScript::checkNotify()
0x56758955 	[chrome.dll 	- cachedscript.cpp:89] 	WebCore::CachedScript::data(WTF::PassRefPtr<WebCore::SharedBuffer>,bool)
0x5665f028 	[chrome.dll 	- loader.cpp:415] 	WebCore::Loader::Host::didFinishLoading(WebCore::SubresourceLoader *)
0x567563c5 	[chrome.dll 	- subresourceloader.cpp:183] 	WebCore::SubresourceLoader::didFinishLoading()
0x56756ed1 	[chrome.dll 	- resourceloader.cpp:443] 	WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle *)
0x568135d7 	[chrome.dll 	- resourcehandle.cpp:191]

[Tony Gentilcore]

Created attachment 64636 [details]
Testcase

[Adam Barth]

DOM mutation events are sad face.  We need to make sure the parser never fires them.  There are a bunch of FIXMEs about where we're firing them today.

[Tony Gentilcore]

I think it is sufficient to check if the child still has a parent and simply not attach it if it has been removed. My patch is a 2-liner, I'll post it shortly.

[Tony Gentilcore]

Created attachment 64729 [details]
Patch

[Tony Gentilcore]

Created attachment 64730 [details]
Fix line break in test

[Darin Adler]

Comment on attachment 64729 [details]
Patch

What about if the beforeload handler removes the target element's parent?

[Tony Gentilcore]

Created attachment 64732 [details]
Add test case which removes element's parent

[Tony Gentilcore]

(In reply to comment #6)
> (From update of attachment 64729 [details])
> What about if the beforeload handler removes the target element's parent?

Good catch. I added another test (explained in the ChangeLog). Please let me know if you had something else in mind.

[Eric Seidel (no email)]

Comment on attachment 64732 [details]
Add test case which removes element's parent

Oh, script tests have to be separate iirc.  I'm not 100% certain of that, but I believe that html5lib doesn't handle tests which run scripts.  Maybe adoption02 is already script-only tests?

This change makes total sense:

+        ASSERT(child->attached() || !child->parent() || !child->parent()->attached());


But this one?

-        if (site.parent->attached() && !child->attached())
+        if (child->parent() && site.parent->attached() && !child->attached())
             child->attach();

we just used site.parent on the previous line.  The only way site.parent could be missing is if it got removed in insiertBefore().  The right fix for that is to add a parserInsertBefore() which doesn't send events.

So I'm confused how this is the right fix?

[Darin Adler]

Comment on attachment 64732 [details]
Add test case which removes element's parent

Sorry, I missed Eric's comments. Setting to review- based on those.

[Tony Gentilcore]

(In reply to comment #9)
> (From update of attachment 64732 [details])
> Oh, script tests have to be separate iirc.  I'm not 100% certain of that, but I believe that html5lib doesn't handle tests which run scripts.  Maybe adoption02 is already script-only tests?

I'm happy to move them if necessary, but they appear to work and there is a <script> block in an existing test in that file. Am I missing something?

> 
> This change makes total sense:
> 
> +        ASSERT(child->attached() || !child->parent() || !child->parent()->attached());
> 
> 
> But this one?
> 
> -        if (site.parent->attached() && !child->attached())
> +        if (child->parent() && site.parent->attached() && !child->attached())
>              child->attach();
> 
> we just used site.parent on the previous line.  The only way site.parent could be missing is if it got removed in insiertBefore().  The right fix for that is to add a parserInsertBefore() which doesn't send events.

I tried that initially by hacking in a quick insertBefore variant with the events commented out. That fixed the crash but then the iframe wasn't actually removed by the beforeload handler. I believe we want to support beforeload removing an element, right? Extensions like AdBlock rely on it.

> 
> So I'm confused how this is the right fix?

[Eric Seidel (no email)]

beforeload should have nothing to do with what method we use to insert the element.  the beforeload will be sent by whatever starts the load, which I presume is the parseMappedAttribute function for the iframe element.

[Eric Seidel (no email)]

the adblock is presumably using the DOMInserted event.  Which we do want to prevent.  The parser should not be sending any mutation events.

[Eric Seidel (no email)]

Yeah, I think this is the wrong fix.  We shouldn't just be guarding against the events.  We should not be sending them at all.  Unless we replace insertBefore with a parserInsertBefore, there will just be other variants of this bug reported tomrorow. :)

[Tony Gentilcore]

(In reply to comment #14)
> Yeah, I think this is the wrong fix.  We shouldn't just be guarding against the events.  We should not be sending them at all.  Unless we replace insertBefore with a parserInsertBefore, there will just be other variants of this bug reported tomrorow. :)

Here's what's happening.

ContainerNode::insertBefore() calls ContainerNode::notifyChildInserted() which calls the Node's insertedIntoDocument() which synchronously runs the beforeload handler (because there is no src on the iframe).

Creating a parserInsertBefore() which doesn't trigger insertedIntoDocument() breaks functionality.

Guarding against it in this case seems perfectly reasonable to me, but you have much more experience with this so I'll defer to your recommendation. Another option might be to look into making beforeload async, but I thought that some folks recently ensured it was synchronous precisely for cases like this (blocking resource load in beforeload).

BTW - AdBlock uses a combination of beforeload and DOMNodeInserted.

[Darin Adler]

Adam, Eric: I know you don't want DOM mutation events to exist at all because you are concerned we can’t implement them as currently designed, and you are pushing to turn them off entirely during initial parsing, but removing does cause compatibility differences, especially for browser extensions.

I’m not sure what the right order to stage this is. I’m worried about just removing DOM mutation events and leaving re-adding them as a task for later rather than moving to a new design. You have made assertions that nobody will be shipping any time soon, but how long will WebKit be in an “unshippable” state?

[Adam Barth]

More detailed response later, but trunk is shippable.  If we're not ready to ship, we should go back behind the setting.  Parser shouldn't fire mutation event, both according to the spec and to the intent of the old parser.  Mutation events are fine for mutations to the document via DOM.

[Tony Gentilcore]

Is beforeload considered a DOM mutation event?

[Adam Barth]

> Is beforeload considered a DOM mutation event?

No.  beforeload is a non-standard WebKit feature added recently to support extensions.  Sadly, it also introduces synchronous JavaScript execution at new times.  I'm sad that the folks who implemented beforeload didn't ask for feedback before introducing a feature that's quite likely to introduce exploitable security vulnerabilities, but that's a discussion for another day.

[Tony Gentilcore]

Sounds like there is a much larger issue at play here.

But this is one of the most popular crashers on the Chrome dev channel (due to the popularity of the AdBlock extension). My question is whether it is reasonable to protect against removal in this case (perhaps along with a FIXME: explaining the desire to prevent arbitrary code execution during parsing), or if the current situation is so bad that I should give up on this patch in favor of a more general resolution to this dilemma.

[Adam Barth]

If you want to fix this crash, here's the flowchart:

1) Determine if it's caused by a DOM mutation event or beforeload.
2) If it's DOM mutation, kill the source of the event.  -> Crash fixed.
3) If it's beforeload, figure out how we're going to recover from arbitrary script at that call site.  -> Crash fixed.

If you want to fix all the bugs, here's the flowchart:

1) Remove all generation of DOM mutation events from the parser.
2) Isolate the generation of all beforeload events to as few program points as possible.
3) Make those program points resilient to arbitrary script execution.

[Tony Gentilcore]

Created attachment 64791 [details]
Work in progress

[Tony Gentilcore]

I uploaded a work in progress that splits out a parserInsertBefore() just like parserAddChild(). That doesn't fix the beforeload case, but at least it suppresses the DOM mutations events which gets us closer to being able to isolate the beforeload points.

However, since these parser*() methods don't handle re-parenting, this approach makes https://bugs.webkit.org/show_bug.cgi?id=44170 more prominent.

I'm thinking about how this should be handled. There is a pretty suspicious FIXME in attach(). Is there any more background behind this?

    // FIXME: It's confusing that HTMLConstructionSite::attach does the magic
    // redirection to the foster parent but HTMLConstructionSite::attachAtSite
    // doesn't.  It feels like we're missing a concept somehow.

[Adam Barth]

> However, since these parser*() methods don't handle re-parenting, this approach makes https://bugs.webkit.org/show_bug.cgi?id=44170 more prominent.

What do you mean by re-parenting?  The parser decides where to insert elements.  Anything that changes that decision in the rest of WebCore is cruft from the old parser that we want to remove.

[Tony Gentilcore]

> What do you mean by re-parenting?

In certain adoption cases, the node being inserted may already have a parent. The test case in 44170 triggers the existing ASSERT() in parserAddChild() that verifies the node being inserted doesn't have a parent. In the WIP patch I posted, the new no-parent ASSERTs in attachAtSite() and parserInsertBefore() are triggered by existing tests in the html5lib suite.

[Adam Barth]

I see.  Yes, the parser* methods need to understand re-parenting in that sense.  In fact, that's the main reason we end up falling back to the regular mutation-event-firing DOM APIs.

[Tony Gentilcore]

(In reply to comment #26)
> I see.  Yes, the parser* methods need to understand re-parenting in that sense.  In fact, that's the main reason we end up falling back to the regular mutation-event-firing DOM APIs.

Thanks. I'll run that down.

[Eric Seidel (no email)]

(In reply to comment #27)
> (In reply to comment #26)
> > I see.  Yes, the parser* methods need to understand re-parenting in that sense.  In fact, that's the main reason we end up falling back to the regular mutation-event-firing DOM APIs.
> 
> Thanks. I'll run that down.

Do they?  The HTML5 spec is explicit about when to remove the parent if it already exists.  We could keep that as a separate step in the AAA.  It's possible there is just missing code in HTMLConstructionSite or the AAA.

[Eric Seidel (no email)]

My point above, is that it would be a spec bug if we need to "reparent" and the spec doesn't tell us to.

[Tony Gentilcore]

You guys are right. From the AAA, step 7:

"Otherwise, append whatever last node ended up being in the previous step to the common ancestor node, first removing it from its previous parent node if any."

So I believe the solution to this and 44170 is to make sure the methods used by attachAtSite for addChild and insertBefore both don't dispatch DOM mutation events and handle re-parenting.

That means making addChild handle re-parenting and making insertBefore not dispatch mutations.

Then we have to deal with onbeforeload separately.

[Eric Seidel (no email)]

I think the 99% case does not want the reparenting branch. I think we should assert !parent() and make the AAA reparent explicitly.

[Tony Gentilcore]

Created attachment 64893 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 64893 [details]
Patch

LayoutTests/ChangeLog:8
 +          * html5lib/resources/adoption02.dat: Add 3 new cases. (1) Misnest a node in a table and remove it during its beforeload handler. The node should not be in the tree. (2) Misnest a node in a table and remove its parent during its beforeload handler. The adoption agency should have already changed its parent to the one before the table and it and its parent should be removed. (3) A crazy DOM from https://bugs.webkig.org/show_bug.cgi?id=44170 which previously crashed. I'm not sure the tree is right, but this case is so crazy, it isn't clear to me what would be right.
Might want to manually wrap this. :)

WebCore/dom/ContainerNode.cpp:137
 +          insertBetween(refChildPreviousSibling.get(), next.get(), child);
This is a great change!

WebCore/dom/ContainerNode.cpp:594
 +      ASSERT(!newChild->parent()); // Does not handle reparenting.
I think the old comment was more useful. :)

WebCore/html/HTMLTreeBuilder.cpp:581
 +                      m_lastScriptElement->removeChildren(); // FIXME: DOM mutation events.
I think removeAllChildren() might not send events, not sure.

WebCore/html/HTMLTreeBuilder.cpp:1749
 +          newParent->appendChild(child, ec); // FIXME: DOM mutation events.
This is easy to fix... :)

WebCore/html/HTMLTreeBuilder.cpp:1821
 +              // Use appendChild instead of parserAddChild to handle possible reparenting.
This comment seems wrong.  We should use explicit parent removal here and then parserAddChild.
Maybe the idea is that appendChild does some sort of fancy move by which the child can stay attached?  Not sure.

WebCore/html/HTMLTreeBuilder.cpp:1840
 +              commonAncestor->appendChild(lastNode->element(), ec, true); // FIXME: DOM mutation events.
Why here?  Should this have just had an uncontidional parserRemoveChild outside of the if/else?

[Eric Seidel (no email)]

In general I think this looks great!  Mostly I need some clarifying remarks from you about which ones you chose to replace or not and why.

[Adam Barth]

Comment on attachment 64893 [details]
Patch

This looks pretty good.  I gave Tony some notes about the tests in IRC.  The <a><svg><tr><input></a> results don't seem right.

[Tony Gentilcore]

> The <a><svg><tr><input></a> results don't seem right.
The FF4 beta produces the same wonky dom. I'll leave it as-is and open a spec bug after checking this in.

[Adam Barth]

Ok.

[Tony Gentilcore]

(In reply to comment #34)
> In general I think this looks great!  Mostly I need some clarifying remarks from you about which ones you chose to replace or not and why.

Well I just updated the case that I needed and marked the others as FIXMEs to keep the patch simpler. If you prefer, I can go ahead and update all of those in this patch.

[Tony Gentilcore]

Created attachment 64926 [details]
Patch

[Tony Gentilcore]

(In reply to comment #33)
> (From update of attachment 64893 [details])
> LayoutTests/ChangeLog:8
>  +          * html5lib/resources/adoption02.dat: Add 3 new cases. (1) Misnest a node in a table and remove it during its beforeload handler. The node should not be in the tree. (2) Misnest a node in a table and remove its parent during its beforeload handler. The adoption agency should have already changed its parent to the one before the table and it and its parent should be removed. (3) A crazy DOM from https://bugs.webkig.org/show_bug.cgi?id=44170 which previously crashed. I'm not sure the tree is right, but this case is so crazy, it isn't clear to me what would be right.
> Might want to manually wrap this. :)

Moot now that the tests are split up. But I'll keep that in mind.

> WebCore/dom/ContainerNode.cpp:594
>  +      ASSERT(!newChild->parent()); // Does not handle reparenting.
> I think the old comment was more useful. :)

OK. I switched back to the old and added the note "... (and want DOM mutation events)".

> WebCore/html/HTMLTreeBuilder.cpp:581
>  +                      m_lastScriptElement->removeChildren(); // FIXME: DOM mutation events.
> I think removeAllChildren() might not send events, not sure.

Oops, you are right. Comment removed.
 
> WebCore/html/HTMLTreeBuilder.cpp:1749
>  +          newParent->appendChild(child, ec); // FIXME: DOM mutation events.
> This is easy to fix... :)

I initially wasn't going to fix everything in this patch. But I went ahead and did everything.

> WebCore/html/HTMLTreeBuilder.cpp:1821
>  +              // Use appendChild instead of parserAddChild to handle possible reparenting.
> This comment seems wrong.  We should use explicit parent removal here and then parserAddChild.
> Maybe the idea is that appendChild does some sort of fancy move by which the child can stay attached?  Not sure.

Done. I don't see any fancy moves in appendChild.

> WebCore/html/HTMLTreeBuilder.cpp:1840
>  +              commonAncestor->appendChild(lastNode->element(), ec, true); // FIXME: DOM mutation events.
> Why here?  Should this have just had an uncontidional parserRemoveChild outside of the if/else?

Yep. Done.

[Adam Barth]

Comment on attachment 64926 [details]
Patch

Very nice.  Thanks for writing this patch.  It pays off a debt we incurred.

WebCore/html/HTMLTreeBuilder.cpp:1825
 +              if (lastNode->element()->parentElement()->attached() && !lastNode->element()->attached())
I would have temporaries for these, but ok.

[Eric Seidel (no email)]

Yup, fantastic as always, thanks Tony!

[Tony Gentilcore]

Created attachment 64958 [details]
Revert stray permission change

[Tony Gentilcore]

Created attachment 64963 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 64963 [details]
Patch for landing

Rejecting patch 64963 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20888 test cases.
media/video-preload.html -> crashed

Exiting early after 1 failures. 17251 tests run.
1000.35s total testing time

17250 test cases (99%) succeeded
1 test case (<1%) crashed
36 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3803043

[Eric Seidel (no email)]

Comment on attachment 64963 [details]
Patch for landing

Stupid leopard CoreVideo bug.

[WebKit Commit Bot]

Comment on attachment 64963 [details]
Patch for landing

Clearing flags on attachment: 64963

Committed r65769: <http://trac.webkit.org/changeset/65769>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

parserAddChild and parserRemoveChild should be non-virtual functions in Element rather than virtual functions in Node. There's no caller that needs to call it on a Node*.

[Eric Seidel (no email)]

Darin is right.

[Tony Gentilcore]

(In reply to comment #50)
> Darin is right.

Good catch. I'll fix that up: https://bugs.webkit.org/show_bug.cgi?id=44547