[webkit-dev] Top Crasher: Shadow DOM and Editing Collide Again

[Dimitri Glazkov]

On Mon, Aug 29, 2011 at 1:03 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> Thanks for the analysis, Dimitri.
> On Mon, Aug 29, 2011 at 12:01 PM, Dimitri Glazkov <dglazkov at chromium.org>
> wrote:
>>
>> 1 -- the style is calculated in the middle of the editing command run,
>> producing definitely incorrect results (for example, in the repro in
>> bug, you will have style calculated for a string "aa" in the text
>> area, since we first insert data, and then remove data.
>
> This isn't really unwanted.  It happens everywhere in editing and we need
> those style recalculation in place because editing code inherently relies on
> renderStyle.  i.e. updating styles at intermediate states are needed by
> editing code.

Makes sense.

>>
>> 2 -- the style recalc my trigger forms machinery to assume that this
>> change is "for realz" and start updating its form values, and, as it
>> happens in the repro on bug, stomp over the CharacterData we just
>> started to update, leading to BOOM.
>
> This is the problem.  It's same thing as if mutation event listeners started
> modifying DOM in the middle of an editing command.

Right.

>>
>> A. Ensure that any layout that's forced by an editing operation is
>> performed in a specific order, ensuring that the forced style recalc
>> does not find the tree in a mid-transition state.
>
> I don't think this approach is feasible given that it requires us modifying
> hundreds of lines of editing code.
>>
>> B. Change editing commands to force layout only at the end of the
>> operation, making the commands more atomic.
>
> I can safely say this is impossible because editability itself depends on
> style; i.e. whether a node is editable or not is determined by -the value
> webkit-user-modify.  Nodes without renderers are treated differently than
> nodes with renderers in editing code.
>>
>> C. Add case-specific hacks to areas of code where we know there are
>> troubles. For example, we could add a flag to HTMLTextAreaElement to
>> say "please ignore, this is not a real style recalc, the real one is
>> coming up".
>
> As far as I'm concerned, this is the only practical solution among the
> three.
> I'm still quite skeptical as to why we'd have to re-create descendent nodes
> of innerTextElement upon style recalc / layout.  It seems odd that the life
> time of the shadow DOM is tied to its host's style change.

I just realized what's going on here. The lifetime of the shadow DOM
_used_ to be tied to the HTMLTextAreaElement's RenderObjects. Kent-san
changed that and now the shadow DOM's lifecycle matches that of the
rest of the DOM. However, the update cycle still assumes that the
shadow DOM lives on the RenderObjects, and does this really freaky
thing with shuttling updated value via an extra style recalc (see
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/HTMLTextAreaElement.cpp&l=349).
We should just fix that and move updating code out of
updateFromElement.

:DG<


> - Ryosuke
>