[Shadow][Editing] Deleting character in distributed element caused a crash

• Component: HTML Editing
• Status: RESOLVED FIXED

[Shinya Kawanaka]

<div id="host" contenteditable><div>foo</div></div>

var root = new WebKitShadowRoot(host);
root.innerHTML = "<content></content>";


When removing 'o' of foo, 'foo' will be deleted.

[Hayato Ito]

I had a different behavior in r119949.
We can remove character by character as expected. But when I tried to remove the last one character, it crashed.

../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp(344) : void WebCore::CompositeEditCommand::insertNodeAt(WTF::PassRefPtr<WebCore::Node>, const WebCore::Position&)  

Let me investigate further.

[Shinya Kawanaka]

(In reply to comment #1)
> I had a different behavior in r119949.
> We can remove character by character as expected. But when I tried to remove the last one character, it crashed.
> 
> ../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp(344) : void WebCore::CompositeEditCommand::insertNodeAt(WTF::PassRefPtr<WebCore::Node>, const WebCore::Position&)  
> 
> Let me investigate further.

Oh...

[Hayato Ito]

Created attachment 146835 [details]
fix

[Hayato Ito]

This WIP patch is just a quick fix for crashes. Let me add a test later.

I am afraid that attaching node while in editing is a bad idea, isn't it?
Although I'll think about a better fix which does not require attaching nodes, early feedback from editing guys is welcome.

(In reply to comment #3)
> Created an attachment (id=146835) [details]
> fix

[Hayato Ito]

Things are not so simple. Let me summarize what the issue is in general:

- In applying editing command, If we mutate nodes which are children of a shadow host, it causes ElementShadow:: invalidateDistribution(), which is called from Element::childrenChanged(). As a result, shadow host is detached (and lazyAttached()) and its renderer is gone.
- There are many places where we call isEditablePosition(Position) in edition code. The problem is Position's anchorNode might be a shadow host which is currently detached. isEditablePosition(Position) uses anchorNode's renderer to judge whether it is editable or not.

So we have to recalcStyle somehow in order to make the shadow host get renderer.
Ideally, we should do recalcStyle in isEditablePosition(), but there are other callers of isEditablePosition() other than editing. eg. paintCaret(). Which would fire assertion ASSERT(!isPainting) in recalcStyle.

Node::rendererIsEditable() has some notable comments:

    // Ideally we'd call ASSERT(!needsStyleRecalc()) here, but
    // ContainerNode::setFocus() calls setNeedsStyleRecalc(), so the assertion
    // would fire in the middle of Document::setFocusedNode().

So there are other callers. That means we can not recalcStyle in isEditablePosition easily.

[Hayato Ito]

It seems a bad idea that we do recalcStyle in isEditablePosition(Position) anyway.
Because that is used in assertion, like ASSERT(isEditablePosition(..)).
ASSERT should not have a side effect.

(In reply to comment #5)
> Things are not so simple. Let me summarize what the issue is in general:
> 
> - In applying editing command, If we mutate nodes which are children of a shadow host, it causes ElementShadow:: invalidateDistribution(), which is called from Element::childrenChanged(). As a result, shadow host is detached (and lazyAttached()) and its renderer is gone.
> - There are many places where we call isEditablePosition(Position) in edition code. The problem is Position's anchorNode might be a shadow host which is currently detached. isEditablePosition(Position) uses anchorNode's renderer to judge whether it is editable or not.
> 
> So we have to recalcStyle somehow in order to make the shadow host get renderer.
> Ideally, we should do recalcStyle in isEditablePosition(), but there are other callers of isEditablePosition() other than editing. eg. paintCaret(). Which would fire assertion ASSERT(!isPainting) in recalcStyle.
> 
> Node::rendererIsEditable() has some notable comments:
> 
>     // Ideally we'd call ASSERT(!needsStyleRecalc()) here, but
>     // ContainerNode::setFocus() calls setNeedsStyleRecalc(), so the assertion
>     // would fire in the middle of Document::setFocusedNode().
> 
> So there are other callers. That means we can not recalcStyle in isEditablePosition easily.

[Shinya Kawanaka]

The root cause of W88502 seems the same thing... calling invalidateDistribution() will remove renderer()...

[Hayato Ito]

Created attachment 147242 [details]
ensure Editable is available

[Ryosuke Niwa]

Comment on attachment 147242 [details]
ensure Editable is available

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

> Source/WebCore/ChangeLog:14
> +        isEditablePosition() assertion in the following insertNodeAt()
> +        operation. So we have to make sure that style is updated before asking
> +        isEditablePosition().

Where is insertNodeAt called in this case? We might need to call document()->updateLayoutIgnorePendingStylesheets() in its call site instead.

> Source/WebCore/editing/CompositeEditCommand.cpp:344
> +    ensureEditableCalculated(editingPosition);

What's the point of adding this function?
I'd just call document()->updateLayoutIgnorePendingStylesheets() here instead.

[Hayato Ito]

Thank you for the review.

(In reply to comment #9)
> (From update of attachment 147242 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147242&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        isEditablePosition() assertion in the following insertNodeAt()
> > +        operation. So we have to make sure that style is updated before asking
> > +        isEditablePosition().
> 
> Where is insertNodeAt called in this case? We might need to call document()->updateLayoutIgnorePendingStylesheets() in its call site instead.

Call stack is here. That is called from DeleteSelectionCommand::doApply(). The previous patch (https://bugs.webkit.org/attachment.cgi?id=146835&action=review) might tells you the exact position.

#0  0x00007f7166118716 in WebCore::CompositeEditCommand::insertNodeAt (this=0x7f7155f6c000, insertChild=..., editingPosition=...) at ../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:344
#1  0x00007f716612df6d in WebCore::DeleteSelectionCommand::doApply (this=0x7f7155f6c000) at ../../third_party/WebKit/Source/WebCore/editing/DeleteSelectionCommand.cpp:821
#2  0x00007f7166117d7c in WebCore::CompositeEditCommand::applyCommandToComposite (this=0x7f7155e38c80, prpCommand=...) at ../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:256
#3  0x00007f716611a3ad in WebCore::CompositeEditCommand::deleteSelection (this=0x7f7155e38c80, selection=..., smartDelete=false, mergeBlocksAfterDelete=true, replace=false, expandForSpecialElements=true, sanitizeMarkup=true) at ../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:579
#4  0x00007f7166192ca8 in WebCore::TypingCommand::deleteKeyPressed (this=0x7f7155e38c80, granularity=WebCore::CharacterGranularity, killRing=false) at 
...


> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:344
> > +    ensureEditableCalculated(editingPosition);
> 
> What's the point of adding this function?

I'd like to make the intention clear. See the follow up meta bug also https://bugs.webkit.org/show_bug.cgi?id=88968.
I am afraid we have to insert the similar code to other places. It might be better to name the function explicitly. I am okay to make that inline if needed.

> I'd just call document()->updateLayoutIgnorePendingStylesheets() here instead.

Thank you. updateLayoutIgnorePendingStylesheets() looks better than updateStyleIfNeeded(). But it has a comment:

> // FIXME: This is a bad idea and needs to be removed eventually.
> // Other browsers load stylesheets before they continue parsing the web page.
> // Since we don't, we can run JavaScript code that needs answers before the
> // stylesheets are loaded. Doing a layout ignoring the pending stylesheets
> // lets us get reasonable answers. The long term solution to this problem is
> // to instead suspend JavaScript execution.
> void Document::updateLayoutIgnorePendingStylesheets()

Do you still suggest using this?

[Ryosuke Niwa]

Comment on attachment 147242 [details]
ensure Editable is available

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

>>> Source/WebCore/ChangeLog:14
>>> +        isEditablePosition().
>> 
>> Where is insertNodeAt called in this case? We might need to call document()->updateLayoutIgnorePendingStylesheets() in its call site instead.
> 
> Call stack is here. That is called from DeleteSelectionCommand::doApply(). The previous patch (https://bugs.webkit.org/attachment.cgi?id=146835&action=review) might tells you the exact position.
> 
> #0  0x00007f7166118716 in WebCore::CompositeEditCommand::insertNodeAt (this=0x7f7155f6c000, insertChild=..., editingPosition=...) at ../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:344
> #1  0x00007f716612df6d in WebCore::DeleteSelectionCommand::doApply (this=0x7f7155f6c000) at ../../third_party/WebKit/Source/WebCore/editing/DeleteSelectionCommand.cpp:821
> #2  0x00007f7166117d7c in WebCore::CompositeEditCommand::applyCommandToComposite (this=0x7f7155e38c80, prpCommand=...) at ../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:256
> #3  0x00007f716611a3ad in WebCore::CompositeEditCommand::deleteSelection (this=0x7f7155e38c80, selection=..., smartDelete=false, mergeBlocksAfterDelete=true, replace=false, expandForSpecialElements=true, sanitizeMarkup=true) at ../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:579
> #4  0x00007f7166192ca8 in WebCore::TypingCommand::deleteKeyPressed (this=0x7f7155e38c80, granularity=WebCore::CharacterGranularity, killRing=false) at 
> ...

Okay, I think what we need to do is to call document()->updateLayoutIgnorePendingStylesheets() in isEditablePosition.

>>> Source/WebCore/editing/CompositeEditCommand.cpp:344
>>> +    ensureEditableCalculated(editingPosition);
>> 
>> What's the point of adding this function?
>> I'd just call document()->updateLayoutIgnorePendingStylesheets() here instead.
> 
> I'd like to make the intention clear. See the follow up meta bug also https://bugs.webkit.org/show_bug.cgi?id=88968.
> I am afraid we have to insert the similar code to other places. It might be better to name the function explicitly. I am okay to make that inline if needed.

Once we move this call to inside isEditablePosition, the indention is very clear.

[Hayato Ito]

Thank you for the review.

(In reply to comment #11)
> Okay, I think what we need to do is to call document()->updateLayoutIgnorePendingStylesheets() in isEditablePosition.

Yeah, that was what I thought at first, but I gave up that idea.
The previous comment #5 and #6 might explained why I gave up.

https://bugs.webkit.org/show_bug.cgi?id=88484#c5
https://bugs.webkit.org/show_bug.cgi?id=88484#c6


Another idea is to make ensureEditableCalculated(Position) returns bool (as a result of isEditablePosition) so that we don't have to call isEditablePosition(position) after that.

Instead of:
  ensureEditableCalculated(position);
  ASSERT(isEditablePosition(position));

We can:
  bool editable = ensureEditableCalculated(position);
  ASSERT(editable);

But I prefer the former to the latter. What do you think?





> 
> >>> Source/WebCore/editing/CompositeEditCommand.cpp:344
> >>> +    ensureEditableCalculated(editingPosition);
> >> 
> >> What's the point of adding this function?
> >> I'd just call document()->updateLayoutIgnorePendingStylesheets() here instead.
> > 
> > I'd like to make the intention clear. See the follow up meta bug also https://bugs.webkit.org/show_bug.cgi?id=88968.
> > I am afraid we have to insert the similar code to other places. It might be better to name the function explicitly. I am okay to make that inline if needed.
> 
> Once we move this call to inside isEditablePosition, the indention is very clear.

[Ryosuke Niwa]

(In reply to comment #12)
> Another idea is to make ensureEditableCalculated(Position) returns bool (as a result of isEditablePosition) so that we don't have to call isEditablePosition(position) after that.
> 
> Instead of:
>   ensureEditableCalculated(position);
>   ASSERT(isEditablePosition(position));
> 
> We can:
>   bool editable = ensureEditableCalculated(position);
>   ASSERT(editable);
> 
> But I prefer the former to the latter. What do you think?

It sounds like we're just working-around the real bug here. We should be able to call updateLayout() in isEditablePosition. If not, then we need to fix that problem first.

[Hayato Ito]

(In reply to comment #13)
> (In reply to comment #12)
> > Another idea is to make ensureEditableCalculated(Position) returns bool (as a result of isEditablePosition) so that we don't have to call isEditablePosition(position) after that.
> > 
> > Instead of:
> >   ensureEditableCalculated(position);
> >   ASSERT(isEditablePosition(position));
> > 
> > We can:
> >   bool editable = ensureEditableCalculated(position);
> >   ASSERT(editable);
> > 
> > But I prefer the former to the latter. What do you think?
> 
> It sounds like we're just working-around the real bug here. We should be able to call updateLayout() in isEditablePosition. If not, then we need to fix that problem first.

As I explained in comment #5,  we cannot easily call updateLayout() in isEditablePosion. That hit another assertion. Do you have any ideas to avoid assertion?

[Ryosuke Niwa]

(In reply to comment #14)
> As I explained in comment #5,  we cannot easily call updateLayout() in isEditablePosion. That hit another assertion. Do you have any ideas to avoid assertion?

Then we'll need to fix that assertion first.

[Hayato Ito]

Okay. Let me re-think how to refactor isEditablePosition and its caller other than editing.
My first rough idea is:
  - Make isEditablePosition() call updateLayoutXXX().
  - Have new function, like isEditablePositionWithoutUpdatingStyle(tentative name :) ), and make some callers call this new function.

isEditablePosition has already 2nd parameter which has default value. So I'd like to avoid adding third parameter to it. What do you think about it?

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Another idea is to make ensureEditableCalculated(Position) returns bool (as a result of isEditablePosition) so that we don't have to call isEditablePosition(position) after that.
> > > 
> > > Instead of:
> > >   ensureEditableCalculated(position);
> > >   ASSERT(isEditablePosition(position));
> > > 
> > > We can:
> > >   bool editable = ensureEditableCalculated(position);
> > >   ASSERT(editable);
> > > 
> > > But I prefer the former to the latter. What do you think?
> > 
> > It sounds like we're just working-around the real bug here. We should be able to call updateLayout() in isEditablePosition. If not, then we need to fix that problem first.
> 
> As I explained in comment #5,  we cannot easily call updateLayout() in isEditablePosion. That hit another assertion. Do you have any ideas to avoid assertion?

[Ryosuke Niwa]

(In reply to comment #16)
> Okay. Let me re-think how to refactor isEditablePosition and its caller other than editing.
> My first rough idea is:
>   - Make isEditablePosition() call updateLayoutXXX().
>   - Have new function, like isEditablePositionWithoutUpdatingStyle(tentative name :) ), and make some callers call this new function.
> 
> isEditablePosition has already 2nd parameter which has default value. So I'd like to avoid adding third parameter to it. What do you think about it?

The thing is, calling rendererIsEditable() when the style is not up-to-date gives you a bogus result because editability depends on styles.

[Hayato Ito]

One more question. Do you think it is okay that we have isXXX function which has a side effect?
I think it is not preferred in general, but it is allowed?
I am afraid that someone will use ASSERT(isEditablePosition(position)) in the future carelessly.

(In reply to comment #16)
> Okay. Let me re-think how to refactor isEditablePosition and its caller other than editing.
> My first rough idea is:
>   - Make isEditablePosition() call updateLayoutXXX().
>   - Have new function, like isEditablePositionWithoutUpdatingStyle(tentative name :) ), and make some callers call this new function.
> 
> isEditablePosition has already 2nd parameter which has default value. So I'd like to avoid adding third parameter to it. What do you think about it?
> 
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > Another idea is to make ensureEditableCalculated(Position) returns bool (as a result of isEditablePosition) so that we don't have to call isEditablePosition(position) after that.
> > > > 
> > > > Instead of:
> > > >   ensureEditableCalculated(position);
> > > >   ASSERT(isEditablePosition(position));
> > > > 
> > > > We can:
> > > >   bool editable = ensureEditableCalculated(position);
> > > >   ASSERT(editable);
> > > > 
> > > > But I prefer the former to the latter. What do you think?
> > > 
> > > It sounds like we're just working-around the real bug here. We should be able to call updateLayout() in isEditablePosition. If not, then we need to fix that problem first.
> > 
> > As I explained in comment #5,  we cannot easily call updateLayout() in isEditablePosion. That hit another assertion. Do you have any ideas to avoid assertion?

[Ryosuke Niwa]

(In reply to comment #18)
> One more question. Do you think it is okay that we have isXXX function which has a side effect?
> I think it is not preferred in general, but it is allowed?
> I am afraid that someone will use ASSERT(isEditablePosition(position)) in the future carelessly.

A lot of is~ functions has a side effect. e.g. isContentEditable() triggers a layout.

[Hayato Ito]

(In reply to comment #19)
> (In reply to comment #18)
> > One more question. Do you think it is okay that we have isXXX function which has a side effect?
> > I think it is not preferred in general, but it is allowed?
> > I am afraid that someone will use ASSERT(isEditablePosition(position)) in the future carelessly.
> 
> A lot of is~ functions has a side effect. e.g. isContentEditable() triggers a layout.

Okay. Thank you. Then nothing prevents us from calling updateLayout() in isEditablePosition.
In fact, I tried to do it and add third parameter to isEditablePositon() once. Let me proceed in this way.

[Hayato Ito]

To avoid hitting assertion caused by paintCaret(), it seems we also have to modify FrameSelection::isContentEditable() and VisibleSelection::isContentEditable so that we can pass a enum to isEditablePosition().

STDERR: ASSERTION FAILED: !view() || (!view()->isInLayout() && !view()->isPainting())
STDERR: ../../third_party/WebKit/Source/WebCore/dom/Document.cpp(1858) : virtual void WebCore::Document::updateStyleIfNeeded()

STDERR: [17052:17052:360193340672:ERROR:process_util_posix.cc(143)] Received signal 11
STDERR: 	base::debug::StackTrace::StackTrace() [0x7f50be3d3ade]
STDERR: 	base::(anonymous namespace)::StackDumpSignalHandler() [0x7f50be42fa55]
STDERR: 	0x7f50b51b64c0
STDERR: 	WebCore::Document::updateStyleIfNeeded() [0x7f50b9573336]
STDERR: 	WebCore::Document::updateLayout() [0x7f50b9573559]
STDERR: 	WebCore::Document::updateLayoutIgnorePendingStylesheets() [0x7f50b957369b]
STDERR: 	WebCore::isEditablePosition() [0x7f50b9feed46]
STDERR: 	WebCore::VisibleSelection::isContentEditable() [0x7f50b9fed8f0]
STDERR: 	WebCore::FrameSelection::isContentEditable() [0x7f50b8baa8ec]
STDERR: 	WebCore::RenderBlock::paintCaret() [0x7f50b936083a]
STDERR: 	WebCore::RenderBlock::paintObject() [0x7f50b9360f22]
STDERR: 	WebCore::RenderView::paint() [0x7f50b95056b9]

[Hayato Ito]

Created attachment 147486 [details]
call updatelayout in isEditablePosition

[Hayato Ito]

Created attachment 147487 [details]
minor update of changelog

[Hayato Ito]

I've uploaded a new patch where we call updateLayout in isEditablePosition().

We should feel shame that we have a lot of isXX function which have side effects. But I guess its WebKit's way.

[Build Bot]

Comment on attachment 147487 [details]
minor update of changelog

Attachment 147487 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12957215

[Shinya Kawanaka]

> We should feel shame that we have a lot of isXX function which have side effects. But I guess its WebKit's way.

It's very error-prone... I think we should fix them whenever possible...

[Hayato Ito]

Created attachment 147514 [details]
fix mac build

[Ryosuke Niwa]

(In reply to comment #24)
> I've uploaded a new patch where we call updateLayout in isEditablePosition().
> 
> We should feel shame that we have a lot of isXX function which have side effects. But I guess its WebKit's way.

Some functions ought to have side-effects. e.g. it's impossible to determine if something is editable without up-to-date style like it's impossible to determine the width of some element without up-to-date layout.

And if we require callers to make things up-to-date, then we'll end up calling updateLayout() and other functions that require non-dirty style or layout such as rendererIsEditable() in pairs, which is strictly worse than having a function that does automatically updates them for you.

The way I see, the bigger problem is the fact that VisiblePosition's constructor triggers layout.

[Ryosuke Niwa]

Comment on attachment 147514 [details]
fix mac build

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

> Source/WebCore/editing/FrameSelection.h:132
> -    bool isContentEditable() const { return m_selection.isContentEditable(); }
> +    bool isContentEditable(EUpdateStyle updateStyle = UpdateStyle) const { return m_selection.isContentEditable(updateStyle); }

A better approach will be to add rendererIsEditable() function since we already have Node::isContentEditable() and Node::rendererIsEditable().

> Source/WebCore/rendering/RenderBlock.cpp:2842
> +        isContentEditable = frame()->selection()->isContentEditable(DoNotUpdateStyle);
>      } else {
>          caretPainter = frame()->page()->dragCaretController()->caretRenderer();
>          isContentEditable = frame()->page()->dragCaretController()->isContentEditable();

We have the same problem with DragCaretController::isContentEditable.
Yet another approach is to use caretPainter->node()->rendererIsEditable() that might be cleaner because then you can reduce the code paths.

[Ryosuke Niwa]

In the future, I highly recommend to cc darin and enrica on your bugs. They're both editing experts.

[Hayato Ito]

Thank you for the review. I'll upload a new patch soon.

(In reply to comment #28)
> (In reply to comment #24)
> > I've uploaded a new patch where we call updateLayout in isEditablePosition().
> > 
> > We should feel shame that we have a lot of isXX function which have side effects. But I guess its WebKit's way.
> 
> Some functions ought to have side-effects. e.g. it's impossible to determine if something is editable without up-to-date style like it's impossible to determine the width of some element without up-to-date layout.
> 
> And if we require callers to make things up-to-date, then we'll end up calling updateLayout() and other functions that require non-dirty style or layout such as rendererIsEditable() in pairs, which is strictly worse than having a function that does automatically updates them for you.
> 
> The way I see, the bigger problem is the fact that VisiblePosition's constructor triggers layout.

Thank you for explanation. Yeah, most side-effects seem to be unavoidable. I think a side-effect issue might be worth discussing, but that is beyond this bug. Let's move forward. :)

(In reply to comment #29)
> (From update of attachment 147514 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147514&action=review
> 
> > Source/WebCore/editing/FrameSelection.h:132
> > -    bool isContentEditable() const { return m_selection.isContentEditable(); }
> > +    bool isContentEditable(EUpdateStyle updateStyle = UpdateStyle) const { return m_selection.isContentEditable(updateStyle); }
> 
> A better approach will be to add rendererIsEditable() function since we already have Node::isContentEditable() and Node::rendererIsEditable().

Okay. that sounds nice. Done.
But I can not get rid of the third parameter of isEditablePosition still. Since there is some code that should be shared.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:2842
> > +        isContentEditable = frame()->selection()->isContentEditable(DoNotUpdateStyle);
> >      } else {
> >          caretPainter = frame()->page()->dragCaretController()->caretRenderer();
> >          isContentEditable = frame()->page()->dragCaretController()->isContentEditable();
> 
> We have the same problem with DragCaretController::isContentEditable.
> Yet another approach is to use caretPainter->node()->rendererIsEditable() that might be cleaner because then you can reduce the code paths.

Done.

[Hayato Ito]

Created attachment 147728 [details]
add rendererIsEditable

[Ryosuke Niwa]

Comment on attachment 147728 [details]
add rendererIsEditable

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

> Source/WebCore/editing/htmlediting.cpp:150
>      Node* node = p.deprecatedNode();

This should be p.containerNode(). We may consider fixing it while we're at.

> Source/WebCore/editing/htmlediting.cpp:162
> +    switch (updateStyle) {
> +    case UpdateStyle:
> +        node->document()->updateLayoutIgnorePendingStylesheets();
> +        break;
> +    case DoNotUpdateStyle:
> +        break;
> +    default:
> +        ASSERT_NOT_REACHED();
> +        break;
> +    }

I wouldn't use a switch for this since we only have two values.
if (updateStyle == UpdateStyle)
     node->document()->updateLayoutIgnorePendingStylesheets();
else
    ASSERT(updateStyle == DoNotUpdateStyle);
is probably sufficient.

> Source/WebCore/editing/htmlediting.h:158
> +enum EUpdateStyle {
> +    UpdateStyle,
> +    DoNotUpdateStyle,
> +};

No need to place each value on its own line. You can just them all in one line.

> Source/WebCore/rendering/RenderBlock.cpp:2842
> +        isContentEditable = caretPainter ? caretPainter->node()->rendererIsEditable() : false;

You can do that same for the line 2839. And then we wouldn't need FrameSelection::rendererIsEditable!

> LayoutTests/editing/shadow/delete-characters-in-distributed-node-crash-expected.txt:3
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE

I wouldn't use js-test for this since you don't have any test cases.
It's probably better to just spit out PASS by document.write or innerHTML.

[Hayato Ito]

Comment on attachment 147728 [details]
add rendererIsEditable

Thank you for the review. Let me address your comment later. Now I have one notice:

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

>> Source/WebCore/rendering/RenderBlock.cpp:2842
>> +        isContentEditable = caretPainter ? caretPainter->node()->rendererIsEditable() : false;
> 
> You can do that same for the line 2839. And then we wouldn't need FrameSelection::rendererIsEditable!

I've found that DragCaretContoller::isContentEditable has a different behavior semantically from it's render->node()->rendererIsEditable().
Also FrameSelection::isContentEditable() and frameselection->caretRenderer()->rendererIsEditable have different behavior semantically.

So it seems we can not simply replace current code with renderer->node()->rendererIsEditable(). Let me think later further.

[Ryosuke Niwa]

Comment on attachment 147728 [details]
add rendererIsEditable

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

>>> Source/WebCore/rendering/RenderBlock.cpp:2842
>>> +        isContentEditable = caretPainter ? caretPainter->node()->rendererIsEditable() : false;
>> 
>> You can do that same for the line 2839. And then we wouldn't need FrameSelection::rendererIsEditable!
> 
> I've found that DragCaretContoller::isContentEditable has a different behavior semantically from it's render->node()->rendererIsEditable().
> Also FrameSelection::isContentEditable() and frameselection->caretRenderer()->rendererIsEditable have different behavior semantically.
> 
> So it seems we can not simply replace current code with renderer->node()->rendererIsEditable(). Let me think later further.

Do some tests fail? By the way, you don't even need to use caretPainter->node()->rendererIsEditable(). Just node()->rendererIsEditable() is sufficient because we check that caretPainter is equal to "this" pointer in the line 2845. All we care about here is whether this block is editable or not.

[Hayato Ito]

Created attachment 148676 [details]
update

[Hayato Ito]

Comment on attachment 147728 [details]
add rendererIsEditable

Thank you for the review. I've resumed on working on this bug.

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

>> Source/WebCore/editing/htmlediting.cpp:162
>> +    }
> 
> I wouldn't use a switch for this since we only have two values.
> if (updateStyle == UpdateStyle)
>      node->document()->updateLayoutIgnorePendingStylesheets();
> else
>     ASSERT(updateStyle == DoNotUpdateStyle);
> is probably sufficient.

Done.

>> Source/WebCore/editing/htmlediting.h:158
>> +};
> 
> No need to place each value on its own line. You can just them all in one line.

Done.

>>>> Source/WebCore/rendering/RenderBlock.cpp:2842
>>>> +        isContentEditable = caretPainter ? caretPainter->node()->rendererIsEditable() : false;
>>> 
>>> You can do that same for the line 2839. And then we wouldn't need FrameSelection::rendererIsEditable!
>> 
>> I've found that DragCaretContoller::isContentEditable has a different behavior semantically from it's render->node()->rendererIsEditable().
>> Also FrameSelection::isContentEditable() and frameselection->caretRenderer()->rendererIsEditable have different behavior semantically.
>> 
>> So it seems we can not simply replace current code with renderer->node()->rendererIsEditable(). Let me think later further.
> 
> Do some tests fail? By the way, you don't even need to use caretPainter->node()->rendererIsEditable(). Just node()->rendererIsEditable() is sufficient because we check that caretPainter is equal to "this" pointer in the line 2845. All we care about here is whether this block is editable or not.

editing/pasteboard/copy-element-with-conflicting-background-color-from-rule.html will fail if we use caretPainter->node()->renderer() or node()->rendererIsEditable() here.

It seems that there is a possibility that 'frame()->selection()->isContentEditable()' returns true, but caretPainter->node() (or this->node()) is null here. So I reverted the change of the previous patch.

>> LayoutTests/editing/shadow/delete-characters-in-distributed-node-crash-expected.txt:3
>> +TEST COMPLETE
> 
> I wouldn't use js-test for this since you don't have any test cases.
> It's probably better to just spit out PASS by document.write or innerHTML.

Done.

[Ryosuke Niwa]

(In reply to comment #37)
> It seems that there is a possibility that 'frame()->selection()->isContentEditable()' returns true, but caretPainter->node() (or this->node()) is null here. So I reverted the change of the previous patch.

Ah! We're probably at an anonymous renderer object. I think we can get the parent render object with non-null node() in that case.

[Ryosuke Niwa]

Comment on attachment 148676 [details]
update

Anyway the patch looks landable as is.

[Hayato Ito]

Thank you for the review.

(In reply to comment #38)
> (In reply to comment #37)
> > It seems that there is a possibility that 'frame()->selection()->isContentEditable()' returns true, but caretPainter->node() (or this->node()) is null here. So I reverted the change of the previous patch.
> 
> Ah! We're probably at an anonymous renderer object. I think we can get the parent render object with non-null node() in that case.

Can we assume that such an anonymous renderer has always a parent renderer and the parent renderer has always a node?

Maybe this should be done in another patch, but I am wondering whether the following is a right assumption or not.

    if (type == CursorCaret) {
        caretPainter = frame()->selection()->caretRenderer();
        if (!caretPainter)
            isContentEditable = false;
        else if (caretPainter->node())
            isContentEditable = caretPainter->node()->rendererIsEditable();
        else {
            ASSERT(caretPainter->parent());
            ASSERT(caretPainter->parent()->node());
            isContentEditable = caretPainter->parent()->node()->rendererIsEditable();
        }

[WebKit Review Bot]

Comment on attachment 148676 [details]
update

Clearing flags on attachment: 148676

Committed r120896: <http://trac.webkit.org/changeset/120896>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Comment on attachment 148676 [details]
update

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

> Source/WebCore/editing/VisibleSelection.h:31
> +#include "htmlediting.h"

We shouldn't be adding this. Removed it in http://trac.webkit.org/changeset/121526.