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]
Created attachment 64636 [details] Testcase
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.
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.
Created attachment 64729 [details] Patch
Created attachment 64730 [details] Fix line break in test
Comment on attachment 64729 [details] Patch What about if the beforeload handler removes the target element's parent?
Created attachment 64732 [details] Add test case which removes element's parent
(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.
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?
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.
(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?
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.
the adblock is presumably using the DOMInserted event. Which we do want to prevent. The parser should not be sending any mutation events.
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. :)
(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.
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?
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.
Is beforeload considered a DOM mutation event?
> 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.
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.
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.
Created attachment 64791 [details] Work in progress
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.
> 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.
> 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.
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.
(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.
(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.
My point above, is that it would be a spec bug if we need to "reparent" and the spec doesn't tell us to.
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.
I think the 99% case does not want the reparenting branch. I think we should assert !parent() and make the AAA reparent explicitly.
Created attachment 64893 [details] Patch
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?
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.
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.
> 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.
Ok.
(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.
Created attachment 64926 [details] Patch
(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.
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.
Yup, fantastic as always, thanks Tony!
Created attachment 64958 [details] Revert stray permission change
Created attachment 64963 [details] Patch for landing
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
Comment on attachment 64963 [details] Patch for landing Stupid leopard CoreVideo bug.
Comment on attachment 64963 [details] Patch for landing Clearing flags on attachment: 64963 Committed r65769: <http://trac.webkit.org/changeset/65769>
All reviewed patches have been landed. Closing bug.
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*.
Darin is right.
(In reply to comment #50) > Darin is right. Good catch. I'll fix that up: https://bugs.webkit.org/show_bug.cgi?id=44547