Style update done due to mutation event dispatching in textarea can be used to corrupt the render tree

• Component: New Bugs
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Mutation events and textarea can be used to corrupt the render tree.

[Dimitri Glazkov (Google)]

This is not yet ready for review, but I wanted you guys to take a peek. The problem is in RenderTextControlMultiLine::updateFromElement(), because by using setInnerTextValue(), it may call ContainerNode::appendChild(), which in turn may dispatch child insertion events (DOMNodeInserted and/or DOMNodeInsertedIntoDocument).

Armed with this knowledge, we can create a sequence of event where dispatching this event triggers recalcStyle on the whole document, which in turn proceeds to detach/attach nodes in the tree, thus creating a re-entrantcy.

Adding a few ULs/LIs is just enough to get the renderer to attempt locating its common ancestor renderer and crash, realizing that it's an orphaned RenderObject (see test attached)...

The badness is obvious, but the fix isn't. My take is to explicitly silence child insertion events when attaching an HTMLFormControlElement, since it shouldn't fire mutation events here anyway.

What do you think?

[Darin Adler]

I think Hyatt said the fix for this sort of thing is to keep those events inside the shadow tree.

[Dimitri Glazkov (Google)]

Created attachment 52108 [details]
Patch

[Darin Adler]

Comment on attachment 52108 [details]
Patch

Maybe instead we should prevent these events from bubbling up outside the shadow tree. After all, it's not good to expose these shadow tree nodes to JavaScript code from the surrounding web page.

[Ojan Vafai]

I think I'm missing something. Don't we already avoid bubbling these events outside the shadow tree? I wrote a quick testcase http://bit.ly/9bJrHI. It creates a contentEditable div and below it a textarea and logs mutation events on the body to the console. I don't see mutationEvents fire from textareas.

[Darin Adler]

(In reply to comment #1)
> Adding a few ULs/LIs is just enough to get the renderer to attempt locating its
> common ancestor renderer and crash, realizing that it's an orphaned
> RenderObject (see test attached).

It's not attached!

[Dimitri Glazkov (Google)]

(In reply to comment #5)
> I think I'm missing something. Don't we already avoid bubbling these events
> outside the shadow tree? I wrote a quick testcase http://bit.ly/9bJrHI. It
> creates a contentEditable div and below it a textarea and logs mutation events
> on the body to the console. I don't see mutationEvents fire from textareas.

This is not about mutation events firing from textareas. It's about a side effect, which is dispatching mutation events while a textarea is being attached. Further badness is triggered by Document::updateStyleForAllDocuments(), which is invoked at the end of the dispatchGenericEvent. In the test above, the mutation event is actually empty.

[James Robinson]

Looks like a regression, doesn't crash on Google Chrome stable channel.

[James Robinson]

This is the significant callstack:

WebCore::ContainerNode::queuePostAttachCallback()
WebCore::HTMLFormControlElement::recalcStyle()
WebCore::Element::recalcStyle()
WebCore::Element::recalcStyle()
WebCore::Element::recalcStyle()
WebCore::Element::recalcStyle()
WebCore::Document::recalcStyle()
WebCore::Document::updateStyleIfNeeded()
WebCore::Document::updateStyleForAllDocuments()
WebCore::Node::dispatchGenericEvent()
WebCore::Node::dispatchEvent()
WebCore::dispatchChildInsertionEvents()
WebCore::ContainerNode::appendChild()
WebCore::replaceChildrenWithText()
WebCore::HTMLElement::setInnerText()
WebCore::RenderTextControl::setInnerTextValue()
WebCore::RenderTextControlMultiLine::updateFromElement()
WebCore::HTMLFormControlElement::attach()
WebCore::ContainerNode::attach()
WebCore::Element::attach()
WebCore::V8Node::appendChildCallback()

The updateFromElement() call on the RenderTextControlMultiLine is causing a mutation event to fire.  This forces a call to Document::updateStyleForAllDocuments(), which enqueues the updateFromElementCallback.  This runs at the end of the style recalc and calls back in to updateFromElement() before running control to Element::attach().  Somehow this horks up the render tree, but I'm not sure exactly how.  It made sense when Dimitri explained it :)

Crashing callstack:

WebCore::RenderBlock::addChild()
WebCore::Node::createRendererIfNeeded()
WebCore::Element::attach()
WebCore::HTMLLIElement::attach()
WebCore::ContainerNode::attach()
WebCore::Element::attach()
WebCore::ContainerNode::attach()
WebCore::Element::attach()
WebCore::V8Node::appendChildCallback()

[Dimitri Glazkov (Google)]

Sorry for being so tongue-tied yesterday -- was running out of the door :)

Yes, jamesr explained it better. I am surprised it's not crashing on Chrome stable. I'll look into that.

Overall, my question is: does the approach of silencing mutation events make sense?

Or should we instead come up with a side-effect-free way of appending a text node to the text area?

Or perhaps detect attachment re-entrancy?

[Darin Adler]

(In reply to comment #10)
> Overall, my question is: does the approach of silencing mutation events make
> sense?

No, I think it's an oblique approach that makes it easy to reintroduce the problem later.

> Or should we instead come up with a side-effect-free way of appending a text
> node to the text area?
>
> Or perhaps detect attachment re-entrancy?

I think the place here to make changes is to look into refining the updateStyleForAllDocuments machinery. It does us no good in cases where the event is not something triggered from outside WebKit. There's no reason to update style at this point.

[James Robinson]

Here's a better explanation of why we crash on the line "bar.appendChild(foo);":

When appendChild() is called, the div 'foo' has three unattached DOM nodes and some Text nodes.  ContainerNode::attach() iterates through its children and attaches each one.  When the textarea is attached, there's a call made to renderer()->updateFromElement() (http://trac.webkit.org/browser/trunk/WebCore/html/HTMLFormControlElement.cpp#L131).  RenderTextControlMultiLine::updateFromElement() calls setInnerTextValue() to populate its text content.  This causes Text nodes to be appended as children of the element, which fires the DOM mutation events.

When the mutation event fires, there's a call made to Document::updateStyleForAllDocuments().  Since the className on bar (foo's parent) has changed since the last style resolution pass, styles are recalculated on it.  Since there are descendant selectors present in the page, all of bar's children are also marked for a style recalculation.  Text::recalcStyle() attaches if it is not currently attached, so all of the children of 'foo' end up getting attached inside the updateStyle...() call.  When this eventually returns, ContainerNode::attach() keeps iterating through children and calling attach() on each child.  Unfortunately the children past the first one were already attached, so we end up attaching some nodes twice and orphaning RenderObjects.  From that point on it's just a matter of time before some codepath touches the wrong RenderObject and dies.

I really think we need to make recalcStyle() have no side effects other than updating the calculated styles on existing renderers.  This is a bit tricky to do since some nodes (forms and anything else with shadow DOMs, at least) rely on recalcStyle() and post attach callbacks to update the render tree in response to DOM mutations.  We currently have some really strange bugs due to the fact that Document::updateStyleForAllDocuments() has a bunch of other weird side effects.

[Darin Adler]

(In reply to comment #12)
> When the mutation event fires, there's a call made to
> Document::updateStyleForAllDocuments().

I think this is what is unneeded. The high level reason we do this after events fire is we think we may be "leaving WebKit" at that point. There's no real reason that event dispatch is the bottleneck for this. Instead the bottleneck could be the level where the call comes into WebKit.

> I really think we need to make recalcStyle() have no side effects other than
> updating the calculated styles on existing renderers.

Sounds like a major project to me, and harder than what I'm suggesting.

[James Robinson]

(In reply to comment #13)
> (In reply to comment #12)
> > When the mutation event fires, there's a call made to
> > Document::updateStyleForAllDocuments().
> 
> I think this is what is unneeded. The high level reason we do this after events
> fire is we think we may be "leaving WebKit" at that point. There's no real
> reason that event dispatch is the bottleneck for this. Instead the bottleneck
> could be the level where the call comes into WebKit.

I'm not sure why the "leaving WebKit" distinction matters.  Nothing that Document::updateStyleForAllDocuments() does should be observable outside of WebKit, it should be fine to return control to the embedder with the styles in any state.  I think the real problem is that updateStyles...() has side effects that are visible inside WebKit and to script so if we omit calls we sometimes get bugs.

For example, a number of layout tests currently run in the body's onload event handler.  If you change the tests to instead run in the DOMContentLoaded event (which fires before onload) then some layout tests (like fast/forms/select-change-popup-to-listbox.html) fail.  Why?  Because currently we fire DOMContentLoaded, which causes a call to Document::updateStyleForAllDocuments(), then we fire the onload event which calls the test code.

> 
> > I really think we need to make recalcStyle() have no side effects other than
> > updating the calculated styles on existing renderers.
> 
> Sounds like a major project to me, and harder than what I'm suggesting.

It is.  There is probably a simpler workaround for this crash, as well.  Since this is one of Chrome's top crashers (this structure shows up on Facebook), we should probably do something sooner rather than later even if it's not perfect.  I still think we have to address the root issue.

[Darin Adler]

(In reply to comment #14)
> I'm not sure why the "leaving WebKit" distinction matters.  Nothing that
> Document::updateStyleForAllDocuments() does should be observable outside of
> WebKit, it should be fine to return control to the embedder with the styles in
> any state.

What matters is that if you return to the run loop without updating style, then visible changes won't happen until something “pokes” WebKit later.

[James Robinson]

(In reply to comment #15)
> (In reply to comment #14)
> > I'm not sure why the "leaving WebKit" distinction matters.  Nothing that
> > Document::updateStyleForAllDocuments() does should be observable outside of
> > WebKit, it should be fine to return control to the embedder with the styles in
> > any state.
> 
> What matters is that if you return to the run loop without updating style, then
> visible changes won't happen until something “pokes” WebKit later.

If styles are out of date the document's m_styleRecalcTimer will be set (in theory anyway, if this is not true it's probably a bug).  When it fires, styles are resolved any any visible changes will cause a repaint to get scheduled.

[Darin Adler]

(In reply to comment #14)
> I think the real problem is that updateStyles...() has side effects
> that are visible inside WebKit and to script so if we omit calls we sometimes
> get bugs.

OK. I think we should start working on these, then. Maybe we can file individual bugs about each one we know about and mark them as blocking a bug that eliminates the updateStyleForAllDocuments call entirely.

[James Robinson]

It looks like the situation is better now than I remember (ggaren in particular has fixed some of these bugs).  Currently fast/forms/select-change-listbox-to-popup.html is the only layout test that fails and it only fails in pixel tests (the selection state is incorrect).  I think we should just nuke Document::updateStyleForAllDocuments() and file a bug to fix up listboxes.

[Darin Adler]

(In reply to comment #18)
> I
> think we should just nuke Document::updateStyleForAllDocuments() and file a bug
> to fix up listboxes.

I would prefer that we fix the list box problem before nuking updateStyleForAllDocuments.

Also, do you have any ideas about flushing out any other problems that may be untested/unreported?

[Dimitri Glazkov (Google)]

I am going to argue that we're not breaking the test -- we're fixing it. We aren't supposed to select items in the multi-line box by default. The selected state is an artifact of it being a listbox, and should probably not carry over if we changed it to menulist.

If anything, I need to ensure the same (default selection reset when switching over from listbox to menulist) happens outside of the onload event.

[Geoffrey Garen]

(In reply to comment #20)
> I am going to argue that we're not breaking the test -- we're fixing it.

How do other browsers behave in this case?

Is there a relevant standard that comments on this case?

[Dimitri Glazkov (Google)]

(In reply to comment #21)
> (In reply to comment #20)
> > I am going to argue that we're not breaking the test -- we're fixing it.
> 
> How do other browsers behave in this case?

I would've thought of that! :) Well, they all preserve the selection, so never mind my awesome argument. Stupid other browsers.

> Is there a relevant standard that comments on this case?

No, though http://dev.w3.org/html5/spec/forms.html#the-select-element is pretty through. We should ask Hixie to add the default-selection clause.

[Ian 'Hixie' Hickson]

Unless I'm misunderstanding, the spec:

   http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-select-element

...covers these cases in detail. Am I missing something?

[Dimitri Glazkov (Google)]

(In reply to comment #23)
> Unless I'm misunderstanding, the spec:
> 
>   
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-select-element
> 
> ...covers these cases in detail. Am I missing something?

... only #whatwg conversation I just had w/annevk who explained this to me :)

[Dimitri Glazkov (Google)]

Created attachment 52467 [details]
Patch

[Darin Adler]

Comment on attachment 52467 [details]
Patch

Looks like a good approach, but I don't think it's quite right.

> +        recalcListItemsIfNeeded();

Why do this unconditionally? Seems to me we only need to do it if the call to m_data.setSize is going to actually change the size.

Why do this before calling m_data.setSize? It seems too early? Why do it just before code that's going to call setRecalcListItems?

Could you explain more about exactly what good it does to recalculate the list at this point?

[Dimitri Glazkov (Google)]

Created attachment 52471 [details]
Patch

[James Robinson]

(In reply to comment #23)
> Unless I'm misunderstanding, the spec:
> 
>   
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-select-element
> 
> ...covers these cases in detail. Am I missing something?

The spec seems to be wrong about this case.  The relevant case for these tests is what the default selection behavior should be for select elements with different values for size without the multiple attribute set.  For example, with the following HTML:

<select size="4">
  <option>a
  <option>b
</select>

which element (if any) should be selected?  The spec says: "If the multiple attribute is absent, whenever there are no option elements in the select element's list of options that have their selectedness set to true, the user agent must set the selectedness of the first option element in the list of options in tree order that is not disabled, if any, to true." which would mean that the option "a" would be set to selected as soon as the select is created.  This is clearly a spec error and no browser actually behaves this way, instead the initial state of the select is that none of the available options are selected.

If the option's size attribute is 1 at creation then the first eligible element should be initially selected.  All browsers seem to properly do this.  The tricky case is when the size is changed to 1.  In this case, most browsers (IE/Opera) will set the first eligible option to selected (except for Firefox which goes into an apparently invalid state). fast/forms/select-change-popup-to-listbox-roundtrip.html covers changing the size from an initial value of 5 to 1 then back to 5.  Initially no elements are selected, then when the size changes to 1 the first element becomes selected and stays selected when the size changes back to 5.

The rationale for calling recalcListItemsIfNeeded() when the size is about to change (but before actually changing it) is that we have to be sure to calculate the selectedness of the element at its current size at least once.  I.e. if we create a select of size 1 and then change it to 5, we have to be sure to call recalcListItems() with a size of 1 to ensure that the first element is selected.  Currently we get this wrong: http://livedom.validator.nu/?%3C!DOCTYPE%20html%3E%0A%3Cbody%3E%0A%3Cselect%20size%3D%221%22%20id%3D%22foo%22%3E%0A%3Coption%3Ea%0A%3Coption%3Eb%0A%3C/select%3E%0A%3Cscript%3E%0Adocument.getElementById(%22foo%22).size%3D5;%0A%3C/script%3E%0A%3C/body%3E

It's probably safe to guard the recalcListItemsIfNeeded() call with a size != oldSize check.

[Darin Adler]

(In reply to comment #28)
> The rationale for calling recalcListItemsIfNeeded() when the size is about to
> change (but before actually changing it) is that we have to be sure to
> calculate the selectedness of the element at its current size at least once. 

The code needs to say this. I think a comment is a good way. Or in theory we could write code that said this so clearly no comment would be necessary.

[Dimitri Glazkov (Google)]

Created attachment 52538 [details]
Patch

[Dimitri Glazkov (Google)]

(In reply to comment #29)
> (In reply to comment #28)
> > The rationale for calling recalcListItemsIfNeeded() when the size is about to
> > change (but before actually changing it) is that we have to be sure to
> > calculate the selectedness of the element at its current size at least once. 
> 
> The code needs to say this. I think a comment is a good way. Or in theory we
> could write code that said this so clearly no comment would be necessary.

Added a comment for now. Will continue refining this code to eliminate necessity of the comment.

[Dimitri Glazkov (Google)]

Comment on attachment 52538 [details]
Patch

Clearing flags on attachment: 52538

Committed r57081: <http://trac.webkit.org/changeset/57081>

[Dimitri Glazkov (Google)]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57081 might have broken Tiger Intel Release

[Adam Barth]

This patch was rolled out in <http://trac.webkit.org/changeset/57100>.  Jamesr is investigating why this patched caused two tests to fail on Tiger and Qt.  Some discussion can be found in Bug 37121.

[James Robinson]

This exposed a race condition due to the way we do object loads.  The test page has an <embed> and an <object> that both reference external resources.  The parser first sees the <embed> and starts its resource load.  The parser then sees the <object>.  A few things happen here:

1.) An HTMLObjectEmbed is created.  Since it is created by the parser, the m_needWidgetUpdate flag is initially false.
2.) HTMLObjectEmbed::parseMappedAttribute() is called on the 'data' attribute.  Since the object is not yet attached, it has no renderer() and thus m_needWidgetUpdate remains false
3.) HTMLObjectEmbed::attach() is called from Element::attach().  This calls queuePostAttachCallback(&HTMLPlugInElement::updateWidgetCallback, this).
4.) Element::attach() runs all post attach callbacks, including the one the HTMLObjectEmbed posted, which calls HTMLEmbedElement::updateWidget()
5.) Since m_needWidgetUpdate is still false, RenderEmbeddedObject::updateWidget is not yet called (see http://trac.webkit.org/browser/trunk/WebCore/html/HTMLEmbedElement.cpp#L181)
6.) The parser then calls HTMLObjectEmbed::finishParsingChildren() which sets m_needWidgetUpdate to true and calls setNeedsStyleRecalc() which starts the Document's m_styleRecalcTimer

At this point we have the race - the <object>'s data will not start loading until its renderer's RenderEmbeddedObject::updateWidget() is called, but that will not happen until styles are resolved.  The Document::m_styleRecalcTimer may or may not run before the <embed>'s resource load finishes.  If the <embed> load finishes first then it ends up calling through FrameLoader::checkCallImplicitClose() which notices that there are no pending resource loads and calls Document::implicitClose() which fires the load event.

Prior to r57081, the call to dispatchEvent() at http://trac.webkit.org/browser/trunk/WebCore/dom/Document.cpp#L4188 caused a call to Document::updateStyleForAllDocuments() which made sure the object's resource load was started before calling into FrameLoader::checkCallImplicitClose().  Even after r57081 it seems that the style recalc time almost always ran before the <embed>'s resource load except for on the Qt port and the Tiger Release bot.

I'm about to upload a patch that adds an explicit updateStyleIfNeeded() call to Document::finishedParsing() to make sure that all <object> resource loads get a chance to complete.

[James Robinson]

Created attachment 52595 [details]
Patch

[James Robinson]

Note: The easiest way to reproduce the issue at r57081 is to up the time on Document::m_styleRecalcTimer from 0 seconds to something like 5 seconds.  This produces pretty reliable failures on Leopard and linux.

[Darin Adler]

(In reply to comment #38)
> Note: The easiest way to reproduce the issue at r57081 is to up the time on
> Document::m_styleRecalcTimer from 0 seconds to something like 5 seconds.  This
> produces pretty reliable failures on Leopard and linux.

Maybe we should have a way to run tests in that mode under DumpRenderTree?

[Dimitri Glazkov (Google)]

(In reply to comment #39)
> (In reply to comment #38)
> > Note: The easiest way to reproduce the issue at r57081 is to up the time on
> > Document::m_styleRecalcTimer from 0 seconds to something like 5 seconds.  This
> > produces pretty reliable failures on Leopard and linux.
> 
> Maybe we should have a way to run tests in that mode under DumpRenderTree?

I am working on a few ideas on how to flush these races and incomplete recalcs out.

[Dimitri Glazkov (Google)]

Comment on attachment 52595 [details]
Patch

r=me the new parts. Assuming darin still blesses the old parts.

[James Robinson]

Committed r57116: <http://trac.webkit.org/changeset/57116>

[Adam Barth]

Comment on attachment 52595 [details]
Patch

Thanks for sorting this out jamesr.

[Adam Barth]

Comment on attachment 52595 [details]
Patch

Looks like this was already landed.

[James Robinson]

Yeah, it's odd that webkit-patch didn't clear out the flags on the patch.

[Alexey Proskuryakov]

> Yeah, it's odd that webkit-patch didn't clear out the flags on the patch.

I can't wait until it actually stops clearing r+.

[Adam Barth]

> I can't wait until it actually stops clearing r+.

It does that so the patches don't show up in pending-commit if the bugs are reopened.

[Balazs Kelemen]

Seems like we have a failure on Qt because of the approach of calling updateStyleIfNeeded in finishedParsing: https://bugs.webkit.org/show_bug.cgi?id=71906. In this case we are loading the plugin twice, first from updateStyleIfNeeded than from finishedParsing. Do you have an idea how to fix it properly?

[Balazs Kelemen]

(In reply to comment #48)
> Seems like we have a failure on Qt because of the approach of calling updateStyleIfNeeded in finishedParsing: https://bugs.webkit.org/show_bug.cgi?id=71906. In this case we are loading the plugin twice, first from updateStyleIfNeeded than from finishedParsing. Do you have an idea how to fix it properly?

I think it's shown on Qt because the test plugin is broken currently, otherwise the first load would be forbidden by this in HTMLObject::updateWidget:
if (pluginCreationOption == CreateOnlyNonNetscapePlugins && wouldLoadAsNetscapePlugin(url, serviceType)) {
    // Ensure updateWidget() is called again during layout to create the Netscape plug-in.
    setNeedsWidgetUpdate(true);
    return;
}

But it's still a bug that we try to load that thing twice. However, I guess we won't do it if it were not blocked, so it's not a very dangerous bug, but still not the perfect behavior.

[Eric Seidel (no email)]

Comment on attachment 52595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=52595&action=review

> WebCore/dom/Document.cpp:4201
> +        // FrameLoader::finishedParsing() might end up calling Document::implicitClose() if all
> +        // resource loads are complete. HTMLObjectElements can start loading their resources from
> +        // post attach callbacks triggered by recalcStyle().  This means if we parse out an <object>
> +        // tag and then reach the end of the document without updating styles, we might not have yet
> +        // started the resource load and might fire the window load event too early.  To avoid this
> +        // we force the styles to be up to date before calling FrameLoader::finishedParsing().
> +        // See https://bugs.webkit.org/show_bug.cgi?id=36864 starting around comment 35.
> +        updateStyleIfNeeded();

Ugh.  We really want to get rid of this once we fix frames/plugins to not load from the rendering tree.