Bug 88484 - [Shadow][Editing] Deleting character in distributed element caused a crash
Summary: [Shadow][Editing] Deleting character in distributed element caused a crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks: 82697
  Show dependency treegraph
 
Reported: 2012-06-06 19:27 PDT by Shinya Kawanaka
Modified: 2012-06-29 00:44 PDT (History)
6 users (show)

See Also:


Attachments
fix (2.06 KB, patch)
2012-06-11 05:20 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
ensure Editable is available (5.85 KB, patch)
2012-06-12 23:12 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
call updatelayout in isEditablePosition (9.59 KB, patch)
2012-06-13 22:34 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
minor update of changelog (9.60 KB, patch)
2012-06-13 22:35 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
fix mac build (10.35 KB, patch)
2012-06-14 01:13 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
add rendererIsEditable (9.69 KB, patch)
2012-06-14 22:05 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
update (9.01 KB, patch)
2012-06-20 16:34 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-06-06 19:27:10 PDT
<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.
Comment 1 Hayato Ito 2012-06-10 22:42:50 PDT
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.
Comment 2 Shinya Kawanaka 2012-06-10 23:51:09 PDT
(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...
Comment 3 Hayato Ito 2012-06-11 05:20:59 PDT
Created attachment 146835 [details]
fix
Comment 4 Hayato Ito 2012-06-11 05:21:48 PDT
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
Comment 5 Hayato Ito 2012-06-11 22:15:39 PDT
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.
Comment 6 Hayato Ito 2012-06-11 23:00:06 PDT
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.
Comment 7 Shinya Kawanaka 2012-06-12 01:24:27 PDT
The root cause of W88502 seems the same thing... calling invalidateDistribution() will remove renderer()...
Comment 8 Hayato Ito 2012-06-12 23:12:25 PDT
Created attachment 147242 [details]
ensure Editable is available
Comment 9 Ryosuke Niwa 2012-06-13 15:25:22 PDT
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.
Comment 10 Hayato Ito 2012-06-13 18:50:27 PDT
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?
Comment 11 Ryosuke Niwa 2012-06-13 19:41:11 PDT
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.
Comment 12 Hayato Ito 2012-06-13 19:53:10 PDT
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.
Comment 13 Ryosuke Niwa 2012-06-13 20:30:07 PDT
(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.
Comment 14 Hayato Ito 2012-06-13 20:43:57 PDT
(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?
Comment 15 Ryosuke Niwa 2012-06-13 20:46:39 PDT
(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.
Comment 16 Hayato Ito 2012-06-13 20:52:07 PDT
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?
Comment 17 Ryosuke Niwa 2012-06-13 20:55:13 PDT
(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.
Comment 18 Hayato Ito 2012-06-13 20:56:41 PDT
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?
Comment 19 Ryosuke Niwa 2012-06-13 20:58:02 PDT
(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.
Comment 20 Hayato Ito 2012-06-13 21:09:34 PDT
(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.
Comment 21 Hayato Ito 2012-06-13 21:30:51 PDT
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]
Comment 22 Hayato Ito 2012-06-13 22:34:03 PDT
Created attachment 147486 [details]
call updatelayout in isEditablePosition
Comment 23 Hayato Ito 2012-06-13 22:35:54 PDT
Created attachment 147487 [details]
minor update of changelog
Comment 24 Hayato Ito 2012-06-13 22:44:48 PDT
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.
Comment 25 Build Bot 2012-06-13 23:28:25 PDT
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
Comment 26 Shinya Kawanaka 2012-06-14 00:19:07 PDT
> 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...
Comment 27 Hayato Ito 2012-06-14 01:13:29 PDT
Created attachment 147514 [details]
fix mac build
Comment 28 Ryosuke Niwa 2012-06-14 02:20:38 PDT
(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.
Comment 29 Ryosuke Niwa 2012-06-14 02:53:37 PDT
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.
Comment 30 Ryosuke Niwa 2012-06-14 02:54:59 PDT
In the future, I highly recommend to cc darin and enrica on your bugs. They're both editing experts.
Comment 31 Hayato Ito 2012-06-14 21:25:31 PDT
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.
Comment 32 Hayato Ito 2012-06-14 22:05:04 PDT
Created attachment 147728 [details]
add rendererIsEditable
Comment 33 Ryosuke Niwa 2012-06-15 01:58:23 PDT
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.
Comment 34 Hayato Ito 2012-06-15 06:40:28 PDT
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.
Comment 35 Ryosuke Niwa 2012-06-15 06:48:32 PDT
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.
Comment 36 Hayato Ito 2012-06-20 16:34:24 PDT
Created attachment 148676 [details]
update
Comment 37 Hayato Ito 2012-06-20 16:54:58 PDT
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.
Comment 38 Ryosuke Niwa 2012-06-20 17:07:36 PDT
(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.
Comment 39 Ryosuke Niwa 2012-06-20 17:10:46 PDT
Comment on attachment 148676 [details]
update

Anyway the patch looks landable as is.
Comment 40 Hayato Ito 2012-06-20 17:41:48 PDT
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();
        }
Comment 41 WebKit Review Bot 2012-06-20 18:34:29 PDT
Comment on attachment 148676 [details]
update

Clearing flags on attachment: 148676

Committed r120896: <http://trac.webkit.org/changeset/120896>
Comment 42 WebKit Review Bot 2012-06-20 18:34:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 43 Ryosuke Niwa 2012-06-29 00:44:42 PDT
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.