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

[Dimitri Glazkov]

Thanks for bringing this up Ryosuke!

I looked over the problem. It seems, the issue is as follows
(generalized from the specific conditions):

a) When an editing command runs, it modifies DOM in discrete steps.
For instance, the SplitTextNodeCommand first inserts a node, then
removes data from existing node
(http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/editing/SplitTextNodeCommand.cpp&l=98).

b) Some of these operations may force style recalc. For example,
modifying that existing node in SplitTextNodeCommand eventually may
call FrameSelection::textWillBeReplaced
(http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/editing/FrameSelection.cpp&l=379).

c) Depending on the state of the tree, the node (or parent of the node
for text nodes) that's being modified could have its style
recalculated. For example, if a sibling + universal selector are at
play, the whole tree is basically doomed to always have its style
recalculated (see repro in bug).

d) This situation produces two unwanted results:

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 is
usually wall-papered over by the next style recalc, which is triggered
at the end of the editing command.

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.

Did I get the details right?

If so, the feasible three solutions that I can imagine are:

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. For example, this
particular crash could be fixed by changing the order of operations to
update character data first.

B. Change editing commands to force layout only at the end of the
operation, making the commands more atomic.

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".

What do you think?

:DG<


On Sun, Aug 28, 2011 at 7:16 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> Hi all,
> We have a nightmare of editing v.s. shadow DOM again.  The WebKit bug 66241
> is currently the top renderer crasher in Chromium (probably affecting Safari
> 5.1 as much as well).
> The problem is that when the shadow DOM is modified by the editing code, the
> shadow host's style is marked dirty and updateLayout ends up
> calling updateFromElement on the shadow host.
> RenderTextControl*::updateFromElement then recreates the children of
> innerTextElement, and invalidates all positions, ending selections, etc...
> that editing code holds, causing the crash.
> We can certainly add band aids in the editing code to check the nullity of
> positions and node ref-pointers and exit early when they are null.  However,
> this would result in wrong behavior because editing operations will end up
> bailing out early in the middle of doing something.
> It appears to me that there are a couple of approaches we can take but none
> of them are promising:
>
> Avoid marking the shadow host dirty when editing code is modifying its
> shadow tree - This approach is a little fragile in that there's nothing that
> stops authors from writing selectors that matches shadow host with certain
> nodes in its shadow tree, in which case the problem persists.
> Avoid re-creating innerTextElement's contents when the shadow host's style
> changes - This requires a lot of work and may not be feasible in some cases
> because authors can override -webkit-apperance and there are certain
> properties such as whitespace, direction, etc... that need to propagate from
> the shadow host.
> Make editing code aware of this situation and update positions and node
> references as needed - As far as I can tell, this is infeasible.  It'll
> require us creating some XPath-like objects at every other line in editing,
> and computing node references and positions back from those objects.
>
> Are there any clever alternatives here?
> Best,
> Ryosuke Niwa
> Software Engineer
> Google Inc.
>