Bug 51389 – Assertion failure after removing a selection in keydown handler

• Component: HTML Editing
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Created attachment 77103 [details]
Test case

Open the attached page, drag-select the second word in the editbox up to the end of the text content, hit any alpha key on the keyboard. The following crash occurs:

>	WebKit.dll!WebCore::DeleteFromTextNodeCommand::DeleteFromTextNodeCommand(WTF::PassRefPtr<WebCore::Text> node=NULL, unsigned int offset=6, unsigned int count=1)  Line 42 + 0x44 bytes	C++
 	WebKit.dll!WebCore::DeleteFromTextNodeCommand::create(WTF::PassRefPtr<WebCore::Text> node=NULL, unsigned int offset=6, unsigned int count=1)  Line 39 + 0x32 bytes	C++
 	WebKit.dll!WebCore::CompositeEditCommand::replaceTextInNode(WTF::PassRefPtr<WebCore::Text> node={...}, unsigned int offset=6, unsigned int count=1, const WTF::String & replacementText={...})  Line 339 + 0x22 bytes	C++
 	WebKit.dll!WebCore::InsertTextCommand::performTrivialReplace(const WTF::String & text={...}, bool selectInsertedText=false)  Line 94	C++
 	WebKit.dll!WebCore::InsertTextCommand::input(const WTF::String & text={...}, bool selectInsertedText=false)  Line 120 + 0x10 bytes	C++
 	WebKit.dll!WebCore::TypingCommand::insertTextRunWithoutNewlines(const WTF::String & text={...}, bool selectInsertedText=false)  Line 390	C++
 	WebKit.dll!WebCore::TypingCommand::insertText(const WTF::String & text={...}, bool selectInsertedText=false)  Line 366	C++
 	WebKit.dll!WebCore::TypingCommand::doApply()  Line 290	C++
 	WebKit.dll!WebCore::EditCommand::apply()  Line 92 + 0xf bytes	C++
 	WebKit.dll!WebCore::applyCommand(WTF::PassRefPtr<WebCore::EditCommand> command={m_document={m_styleSelector={...} m_didCalculateStyleSelector=true m_frame=0x00a6c318 ...} m_startingSelection={...} m_endingSelection={...} ...})  Line 215	C++
 	WebKit.dll!WebCore::TypingCommand::insertText(WebCore::Document * document=0x06c7f980, const WTF::String & text={...}, const WebCore::VisibleSelection & selectionForInsertion={...}, bool selectInsertedText=false, bool insertedTextIsComposition=false)  Line 194 + 0x14 bytes	C++
 	WebKit.dll!WebCore::Editor::insertTextWithoutSendingTextEvent(const WTF::String & text={...}, bool selectInsertedText=false, WebCore::Event * triggeringEvent=0x06d9f748)  Line 1196 + 0x1c bytes	C++
 	WebKit.dll!WebCore::Editor::handleTextEvent(WebCore::TextEvent * event=0x06d9f748)  Line 203 + 0x12 bytes	C++
 	WebKit.dll!WebCore::EventHandler::defaultTextInputEventHandler(WebCore::TextEvent * event=0x06d9f748)  Line 2704 + 0x15 bytes	C++
 	WebKit.dll!WebCore::Node::defaultEventHandler(WebCore::Event * event=0x06d9f748)  Line 2952	C++
 	WebKit.dll!WebCore::Node::dispatchGenericEvent(WTF::PassRefPtr<WebCore::Event> prpEvent=NULL)  Line 2666 + 0x1b bytes	C++
 	WebKit.dll!WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event> prpEvent=NULL)  Line 2580 + 0x12 bytes	C++
 	WebKit.dll!WebCore::EventTarget::dispatchEvent(WTF::PassRefPtr<WebCore::Event> event=NULL, int & ec=-858993460)  Line 289 + 0x19 bytes	C++
 	WebKit.dll!WebCore::EventHandler::handleTextInputEvent(const WTF::String & text={...}, WebCore::Event * underlyingEvent=0x06dad6a8, bool isLineBreak=false, bool isBackTab=false)  Line 2680	C++
 	WebKit.dll!WebCore::Editor::insertText(const WTF::String & text={...}, WebCore::Event * triggeringEvent=0x06dad6a8)  Line 1172	C++
 	WebKit.dll!WebView::handleEditingKeyboardEvent(WebCore::KeyboardEvent * evt=0x06dad6a8)  Line 1888 + 0x27 bytes	C++
 	WebKit.dll!WebEditorClient::handleKeyboardEvent(WebCore::KeyboardEvent * evt=0x06dad6a8)  Line 614 + 0xf bytes	C++
 	WebKit.dll!WebCore::Editor::handleKeyboardEvent(WebCore::KeyboardEvent * event=0x06dad6a8)  Line 172 + 0x16 bytes	C++
 	WebKit.dll!WebCore::EventHandler::defaultKeyboardEventHandler(WebCore::KeyboardEvent * event=0x06dad6a8)  Line 2428	C++
 	WebKit.dll!WebCore::Node::defaultEventHandler(WebCore::Event * event=0x06dad6a8)  Line 2938	C++
 	WebKit.dll!WebCore::Node::dispatchGenericEvent(WTF::PassRefPtr<WebCore::Event> prpEvent=NULL)  Line 2666 + 0x1b bytes	C++
 	WebKit.dll!WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event> prpEvent=NULL)  Line 2580 + 0x12 bytes	C++
 	WebKit.dll!WebCore::Node::dispatchKeyEvent(const WebCore::PlatformKeyboardEvent & key={...})  Line 2724 + 0x19 bytes	C++
 	WebKit.dll!WebCore::EventHandler::keyEvent(const WebCore::PlatformKeyboardEvent & initialKeyEvent={...})  Line 2316 + 0x13 bytes	C++
 	WebKit.dll!WebView::keyPress(unsigned int charCode=102, long keyData=2162689, bool systemKeyDown=false)  Line 1971 + 0x13 bytes	C++
 	WebKit.dll!WebView::WebViewWndProc(HWND__ * hWnd=0x02190514, unsigned int message=258, unsigned int wParam=102, long lParam=2162689)  Line 2096 + 0x12 bytes	C++

This crash requires a workaround in the Web Inspector code (WebCore/inspector/front-end/StylesSidebarPane.js).

[Alexey Proskuryakov]

This is an assertion failure - is there a crash in release mode? But the assertion failure looks quite scary.

    ASSERT(m_offset + m_count <= m_node->length());

[Ryosuke Niwa]

(In reply to comment #1)
> This is an assertion failure - is there a crash in release mode? But the assertion failure looks quite scary.
> 
>     ASSERT(m_offset + m_count <= m_node->length());

Let me take a look.

[Ryosuke Niwa]

The problem is that Range::processContents (called by deleteContents) isn't updating the selection when modifying the text node and/or removing the nodes.  Maybe the correct fix is to removeAllRanges if the selection and the range from which contents are removed intersect in processContents when the action type is EXTRACT_CONTENTS or CLONE_CONTENTS.

[Darin Adler]

(In reply to comment #3)
> The problem is that Range::processContents (called by deleteContents) isn't updating the selection when modifying the text node and/or removing the nodes.

Presumably Range::processContents is doing DOM manipulation that could also be called directly from JavaScript. If so, then a change to Range would not be an acceptable fix. The fix needs to also handle the case where the same manipulation was done without using Range.

Is that right?

[Ryosuke Niwa]

Created attachment 77148 [details]
generalized demo

(In reply to comment #4)
> (In reply to comment #3)
> > The problem is that Range::processContents (called by deleteContents) isn't updating the selection when modifying the text node and/or removing the nodes.
> 
> Presumably Range::processContents is doing DOM manipulation that could also be called directly from JavaScript. If so, then a change to Range would not be an acceptable fix. The fix needs to also handle the case where the same manipulation was done without using Range.
> 
> Is that right?

You're absolutely right. This problem is prevalent in all DOM manipulation APIs.  I attached a simplified test case.

[Ryosuke Niwa]

Created attachment 77186 [details]
fixes the bug

[Ryosuke Niwa]

(In reply to comment #6)
> Created an attachment (id=77186) [details]
> fixes the bug

In this patch, I'm removing the selection whenever the start or the end container of the selection is modified.  But this might be too aggressive.  We could be a little smarter and don't invalidate the selection as much.

For example, if we have the selection such as "[hello] world" and added WebKit to the end, then the selection can stay as in "[hello] world WebKit".  Likewise, if we had "hello [WebKit]" and inserted " world" before " WebKit", the selection can survive as "hello world [WebKit]".

[Ryosuke Niwa]

Created attachment 77268 [details]
new approach work in progress

I'll try to finish this up tomorrow.

[Ryosuke Niwa]

Created attachment 77367 [details]
new approach without splitText fix

[Ryosuke Niwa]

(In reply to comment #9)
> Created an attachment (id=77367) [details]
> new approach without splitText fix

New patch looks clearly better so I'm taking this approach. I'm going to fix splitText in a separate patch because it'll require some behavioral changes not directly related to this bug.

[Darin Adler]

Comment on attachment 77367 [details]
new approach without splitText fix

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

> WebCore/dom/CharacterData.cpp:154
> +    if (document() && document()->frame() && document()->frame()->selection())
> +        document()->frame()->selection()->textWillBeReplaced(this, offsetOfReplacedData, oldLength, newLength);

We do need to null-check document()->frame(), but we do not need to null-check document() or selection().

> WebCore/editing/SelectionController.cpp:263
> +    if (position.offsetInContainerNode() > static_cast<int>(offset) && position.offsetInContainerNode() < static_cast<int>(offset + oldLength))

I suggest putting the typecast on the other side. It’s easy to be sure that casting a non-negative int to unsigned is safe. Harder to know that casting an unsigned to an int is safe.

> WebCore/editing/SelectionController.cpp:276
> +    if (isNone() || !node || highestAncestor(node)->nodeType() == Node::DOCUMENT_FRAGMENT_NODE)
> +        return;

I think it might be better to check highestAncestor(node) != node->document() rather than checking the type of the highest ancestor. But is that check needed? Won’t the logic do the right thing without a special case for that?

> WebCore/editing/SelectionController.h:181
> +    void respondToNodeModification(Node* node, bool baseRemoved, bool extentRemoved, bool startRemoved, bool endRemoved);

I suggest omitting the argument name “node” here.

[Ryosuke Niwa]

Thanks for the review, Darin.  This will be my last patch this year.  Happy holidays.

(In reply to comment #11)
> (From update of attachment 77367 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77367&action=review
> 
> > WebCore/dom/CharacterData.cpp:154
> > +    if (document() && document()->frame() && document()->frame()->selection())
> > +        document()->frame()->selection()->textWillBeReplaced(this, offsetOfReplacedData, oldLength, newLength);
> 
> We do need to null-check document()->frame(), but we do not need to null-check document() or selection().

Oops, I forgot to remove those.  Will fix.

> > WebCore/editing/SelectionController.cpp:263
> > +    if (position.offsetInContainerNode() > static_cast<int>(offset) && position.offsetInContainerNode() < static_cast<int>(offset + oldLength))
> 
> I suggest putting the typecast on the other side. It’s easy to be sure that casting a non-negative int to unsigned is safe. Harder to know that casting an unsigned to an int is safe.

Will do.

> > WebCore/editing/SelectionController.cpp:276
> > +    if (isNone() || !node || highestAncestor(node)->nodeType() == Node::DOCUMENT_FRAGMENT_NODE)
> > +        return;
> 
> I think it might be better to check highestAncestor(node) != node->document() rather than checking the type of the highest ancestor. But is that check needed? Won’t the logic do the right thing without a special case for that?

I copied the condition already present in nodeWillBeRemoved.  Do you want me to change both of these places?

> > WebCore/editing/SelectionController.h:181
> > +    void respondToNodeModification(Node* node, bool baseRemoved, bool extentRemoved, bool startRemoved, bool endRemoved);
> 
> I suggest omitting the argument name “node” here.

Oops. Will do.

[Darin Adler]

Comment on attachment 77367 [details]
new approach without splitText fix

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

> LayoutTests/ChangeLog:12
> +        Many editing tests had to be rebaselined due to different editing delegate dumps
> +        caused by WebKit's properly adjusting selection's offsets as editing commands modify the DOM.

It’s possible some actual WebKit clients might have behavior that depends on what the editing delegate sees. That’s most likely for non-web-browser clients on Mac OS X, such as the Mail application. We’ll have to be on the lookout for problems caused there.

The code they have running in response to delegate methods has no doubt been tested only with the old less-correct offsets, and might accidentally rely on them.

>>> WebCore/editing/SelectionController.cpp:276
>>> +        return;
>> 
>> I think it might be better to check highestAncestor(node) != node->document() rather than checking the type of the highest ancestor. But is that check needed? Won’t the logic do the right thing without a special case for that?
> 
> I copied the condition already present in nodeWillBeRemoved.  Do you want me to change both of these places?

Yes, I think you should. Specifically checking for a document fragment doesn’t make much sense. Nodes can be outside the document tree and not be in a fragment tree, and there’s nothing special about the fragment tree.

Of course, we need to make sure we don’t break something by accident. It’s always possible something is accidentally relying on that logic.

[Ryosuke Niwa]

(In reply to comment #13)
> It’s possible some actual WebKit clients might have behavior that depends on what the editing delegate sees. That’s most likely for non-web-browser clients on Mac OS X, such as the Mail application. We’ll have to be on the lookout for problems caused there.
> 
> The code they have running in response to delegate methods has no doubt been tested only with the old less-correct offsets, and might accidentally rely on them.

A good insight.  I hope for the best.

> Yes, I think you should. Specifically checking for a document fragment doesn’t make much sense. Nodes can be outside the document tree and not be in a fragment tree, and there’s nothing special about the fragment tree.

Ok, so use highestAncestor(node) != node->document() or remove them?

> Of course, we need to make sure we don’t break something by accident. It’s always possible something is accidentally relying on that logic.

I think it's an optimization. nodeWillBeRemoved has a comment:

    // There can't be a selection inside a fragment, so if a fragment's node is being removed,
    // the selection in the document that created the fragment needs no adjustment.
    if (node && highestAncestor(node)->nodeType() == Node::DOCUMENT_FRAGMENT_NODE)
        return;

[Darin Adler]

(In reply to comment #14)
> > Yes, I think you should. Specifically checking for a document fragment doesn’t make much sense. Nodes can be outside the document tree and not be in a fragment tree, and there’s nothing special about the fragment tree.
> 
> Ok, so use highestAncestor(node) != node->document() or remove them?
> 
> > Of course, we need to make sure we don’t break something by accident. It’s always possible something is accidentally relying on that logic.
> 
> I think it's an optimization. nodeWillBeRemoved has a comment:
> 
>     // There can't be a selection inside a fragment, so if a fragment's node is being removed,
>     // the selection in the document that created the fragment needs no adjustment.
>     if (node && highestAncestor(node)->nodeType() == Node::DOCUMENT_FRAGMENT_NODE)
>         return;

If it’s an optimization, then the node->document() version is a better optimization, so lets do that one.

[Ryosuke Niwa]

(In reply to comment #15)
> If it’s an optimization, then the node->document() version is a better optimization, so lets do that one.

Ok, will do.  FYI, the early exit in nodeWillBeRemoved was added by http://trac.webkit.org/changeset/30062.

[Ryosuke Niwa]

Hi Darin,

(In reply to comment #16)
> (In reply to comment #15)
> > If it’s an optimization, then the node->document() version is a better optimization, so lets do that one.

I take my words back and not making this change because it turned out that this change causes editing/pasteboard/paste-plaintext-nowrap.html to crash and the following tests to fail:
editing/deleting/delete-all-text-in-text-field-assertion.html
editing/deleting/delete-ligature-001.html
editing/pasteboard/copy-in-password-field.html

[Ryosuke Niwa]

Committed r74593: <http://trac.webkit.org/changeset/74593>

[WebKit Review Bot]

http://trac.webkit.org/changeset/74593 might have broken Qt Linux Release

[Benjamin (Ben) Kalman]

Created attachment 77389 [details]
GTK rebaseline

[Ryosuke Niwa]

Comment on attachment 77389 [details]
GTK rebaseline

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

> LayoutTests/platform/gtk/editing/deleting/delete-line-end-ws-001-expected.txt:71
> -layer at (0,0) size 820x581
> -  RenderView at (0,0) size 800x581
> -layer at (0,0) size 800x581
> -  RenderBlock {HTML} at (0,0) size 800x581
> -    RenderBody {BODY} at (8,64) size 784x453
> +layer at (0,0) size 820x582
> +  RenderView at (0,0) size 800x582
> +layer at (0,0) size 800x582
> +  RenderBlock {HTML} at (0,0) size 800x582
> +    RenderBody {BODY} at (8,64) size 784x454

Mn... we shouldn't have layout changes.  It's odd that we have these changes.

> LayoutTests/platform/gtk/editing/deleting/delete-line-end-ws-002-expected.txt:71
> -layer at (0,0) size 820x581
> -  RenderView at (0,0) size 800x581
> -layer at (0,0) size 800x581
> -  RenderBlock {HTML} at (0,0) size 800x581
> -    RenderBody {BODY} at (8,64) size 784x453
> +layer at (0,0) size 820x582
> +  RenderView at (0,0) size 800x582
> +layer at (0,0) size 800x582
> +  RenderBlock {HTML} at (0,0) size 800x582
> +    RenderBody {BODY} at (8,64) size 784x454

Ditto.

[Benjamin (Ben) Kalman]

Created attachment 77395 [details]
GTK rebaseline take 2

[Ryosuke Niwa]

Comment on attachment 77395 [details]
GTK rebaseline take 2

LGTM.  I'm landing this manually now. Thanks a lot for doing this for me.  I really appreciate it.

[Ryosuke Niwa]

Committed r74604: <http://trac.webkit.org/changeset/74604>

[Alexander Pavlov (apavlov)]

I'd like to note that the assertion still occurs on the original test case (https://bugs.webkit.org/attachment.cgi?id=77103). Should I reopen the bug or file another one?

[Darin Adler]

(In reply to comment #25)
> I'd like to note that the assertion still occurs on the original test case.

LOL!

> Should I reopen the bug or file another one?

I think you should rename this back, and reopen it. But really either is OK.

[Alexander Pavlov (apavlov)]

Renaming to the original summary and reopening, as suggested by Darin.

[Ryosuke Niwa]

(In reply to comment #25)
> I'd like to note that the assertion still occurs on the original test case (https://bugs.webkit.org/attachment.cgi?id=77103). Should I reopen the bug or file another one?

Oh, oops. I'm sorry about that. I should have checked it. Maybe the original test case called splitText, which is the one function I didn't fix in my patch that has been landed.

[Ryosuke Niwa]

Created attachment 77447 [details]
fixes the bug

[Alexander Pavlov (apavlov)]

Hmm, I haven't seen the bug occur for the past five days or so, but after a bit more testing I'm seeing a strange glitch: the entire editbox contents are cleared away once the Space key is pressed (on Chromium r70347/Linux), while alphanumeric keys replace the selection as they should.

[Ryosuke Niwa]

Could someone review my patch?

[Darin Adler]

Comment on attachment 77447 [details]
fixes the bug

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

Despite the fact that the code changes and test case look quite good to me, I am going to say review- because I feel the patch does not contain sufficient change log or source code comments to make it clear why these changes are correct.

> WebCore/editing/SelectionController.cpp:266
> -    if ((type == EndPointIsStart && static_cast<unsigned>(position.offsetInContainerNode()) >= offset + oldLength)
> +    if (((type == EndPointIsStart || oldLength) && static_cast<unsigned>(position.offsetInContainerNode()) >= offset + oldLength)

I don’t understand this change. It may well be correct, but nothing in the change log explains it.

This is one reason I like per-function change log comments. I believe I would have had a better chance of understanding this with a per-function comment. But a “why” comment in the code would also be welcome.

> WebKit/mac/ChangeLog:8
> +        Preserve selection when unmarking text.

I don’t understand the relationship of this change to the bug you’re fixing. Again, I don’t think this is incorrect, but rather than you are not explaining yourself sufficiently in the patch’s change log or comments.

Under what circumstances should any call site call confirmComposition rather than confirmCompositionWithoutDisturbingSelection? Why isn’t this one of those circumstances? What does this have to do with the assertion failure?

Is there a test case that covers this?

[Ryosuke Niwa]

(In reply to comment #32)
> (From update of attachment 77447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77447&action=review
> 
> Despite the fact that the code changes and test case look quite good to me, I am going to say review- because I feel the patch does not contain sufficient change log or source code comments to make it clear why these changes are correct.

A fair point.  I'll update the patch shortly.

[Ryosuke Niwa]

Created attachment 78294 [details]
added more inline comments

[Ryosuke Niwa]

Comment on attachment 77447 [details]
fixes the bug

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

>> WebKit/mac/ChangeLog:8
>> +        Preserve selection when unmarking text.
> 
> I don’t understand the relationship of this change to the bug you’re fixing. Again, I don’t think this is incorrect, but rather than you are not explaining yourself sufficiently in the patch’s change log or comments.
> 
> Under what circumstances should any call site call confirmComposition rather than confirmCompositionWithoutDisturbingSelection? Why isn’t this one of those circumstances? What does this have to do with the assertion failure?
> 
> Is there a test case that covers this?

I know there is a test that fails without this change, so I added to the change log entry.  However, I can't recall or figure out why I had to call confirmCompositionWithoutDisturbingSelection instead of confirmComposition.  FYI, that function was added by http://trac.webkit.org/changeset/25547/.

[Ryosuke Niwa]

(In reply to comment #35)
> I know there is a test that fails without this change, so I added to the change log entry.  However, I can't recall or figure out why I had to call confirmCompositionWithoutDisturbingSelection instead of confirmComposition.  FYI, that function was added by http://trac.webkit.org/changeset/25547/.

I investigated a little again.  It seems that the test had an error:

textInputController.setMarkedText("P", 1, 0);
testInput.value="PAS";
eventSender.keyDown("S");

should be:

textInputController.setMarkedText("P", 0, 1);
testInput.value="PAS";
eventSender.keyDown("S");

instead.  The test had been passing prior to my patch because we weren't correcting offset :(

Alexey, could you verify that this is indeed the correct fix?

[Ryosuke Niwa]

Created attachment 84033 [details]
fixes the bug

[Ryosuke Niwa]

Created attachment 84035 [details]
added one more rebaseline

[Alexey Proskuryakov]

> Alexey, could you verify that this is indeed the correct fix?

The corrected test makes more sense.

This test is supposed to mimic steps to reproduce from bug 32905. So, the easiest way to confirm is to enable TextInput log channel, follow the steps from bug 32905, and check what messages the input method actually sends.

[Ryosuke Niwa]

(In reply to comment #39)
> > Alexey, could you verify that this is indeed the correct fix?
> 
> The corrected test makes more sense.

Except that... neither the original nor the corrected test catches a similar regression.  I'm seeing a regression on bing.com now with my patch.  When I type "nihao" and press down keys a couple of times to select "nihaoma", pressing deleting key deletes "o".  It seems like this bug is specific to Chinese IME.  I tested few cases but the problem doesn't reproduce when I'm typing Japanese.

> This test is supposed to mimic steps to reproduce from bug 32905. So, the easiest way to confirm is to enable TextInput log channel, follow the steps from bug 32905, and check what messages the input method actually sends.

Google.com has Google Instant and we can no longer reproduce the bug.

[Ryosuke Niwa]

Created attachment 84158 [details]
bing screenshot

[Ryosuke Niwa]

The problem seems to be that when down key is pressed, IME clears the composition but with my patch it doesn't.  So IME thinks that the composition is still going on and responds to key down event.  Does anyone know how IME is canceling composition?  Sadly, I can't resolve this bug until someone can teach me where to look.  As of now, I can't find any useful information out of the code.

[Ryosuke Niwa]

Created attachment 84176 [details]
bing bug reduction

Here's a reduction of the problem I see on bing.com.

[Ryosuke Niwa]

(In reply to comment #43)
> Created an attachment (id=84176) [details]
> bing bug reduction
> 
> Here's a reduction of the problem I see on bing.com.

Note the regression demonstrated by this test already exists in Google Chrome.  Bashi and I spent sometime investigating this bug in relation to the bug 49523 but concluded that they are slightly different bugs.

[Ryosuke Niwa]

Ok, I think I know what needs to be done.  It seems like Mac port is relying on respondToChangedSelection to cancel composition.  We just need to call respondToChangedSelection in textWillBeReplaced after updating layout.  This might be a bit dangerous though as recalcStyle calls textWillBeReplaced indirectly.

[Ryosuke Niwa]

Created attachment 84193 [details]
fixes the bug

Here's new patch.  This patch seems to fix all the problems we've seen.  I'm just sad that all of these changes must happen simultaneously and requires a lot of rebaselines :(

[Ryosuke Niwa]

Can someone review my patch?  I'd like to check in this patch as soon as possible as it's very likely to conflict with other patches.

[Darin Adler]

Comment on attachment 84193 [details]
fixes the bug

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

I’d like to see these changes land separately; especially if that makes it clearer which of them causes the change in observed test results. Is there some reason we can’t do it that way?

I’m going to say review- because there are enough specific things wrong here that it should not land as-is.

> Source/WebCore/ChangeLog:31
> [phone]  Ryosuke Niwa  <[e-mail]>

Double change log.

> Source/WebCore/dom/CharacterData.cpp:157
> -    if (document()->frame())
> -        document()->frame()->selection()->textWillBeReplaced(this, offsetOfReplacedData, oldLength, newLength);
>      RefPtr<StringImpl> oldData = m_data;
>      m_data = newData;
>      updateRenderer(offsetOfReplacedData, oldLength);
> +    if (document()->frame())
> +        document()->frame()->selection()->textWillBeReplaced(this, offsetOfReplacedData, oldLength, newLength);

Would you be willing to land this change separately in its own patch?

> Source/WebCore/editing/SelectionController.cpp:287
> -    if ((type == EndPointIsStart && static_cast<unsigned>(position.offsetInContainerNode()) >= offset + oldLength)
> +    if (((type == EndPointIsStart || oldLength) && static_cast<unsigned>(position.offsetInContainerNode()) >= offset + oldLength)
>          || (type == EndPointIsEnd && static_cast<unsigned>(position.offsetInContainerNode()) > offset + oldLength))

I find this function and change quite confusing. I can almost understand it given the change log comments. Could you add some “why” comments to the source file?

All these calls to static_cast<unsigned>(position.offsetInContainerNode()) make me think the whole function would be way easier to read if that was put in a local variable.

> Source/WebCore/editing/SelectionController.cpp:321
> +        // Since this function can be called while recalculating style, we do as little as possible.
> +        // Notify IME of the selection change so that it clears the current composition.

I don’t understand this comment. The things that need to be done, need to be done. So what does it mean to do as little as possible? We should *always* do as little as possible. Maybe it would be clearer if you said something more specific? I am quite worried about this!

> LayoutTests/ChangeLog:16
> +        Also fixed platform/mac/editing/input/selection-change-closes-typing.html
> +        so that it calls setMarkedText with offset 0 and length 1 as supposed to
> +        offset 1 and length 0.

Would you be willing to land this change first in a separate patch?

[Ryosuke Niwa]

(In reply to comment #48)
> > Source/WebCore/ChangeLog:31
> > [phone]  Ryosuke Niwa  <[e-mail]>
> 
> Double change log.

Oops!

> > Source/WebCore/dom/CharacterData.cpp:157
> > -    if (document()->frame())
> > -        document()->frame()->selection()->textWillBeReplaced(this, offsetOfReplacedData, oldLength, newLength);
> >      RefPtr<StringImpl> oldData = m_data;
> >      m_data = newData;
> >      updateRenderer(offsetOfReplacedData, oldLength);
> > +    if (document()->frame())
> > +        document()->frame()->selection()->textWillBeReplaced(this, offsetOfReplacedData, oldLength, newLength);
> 
> Would you be willing to land this change separately in its own patch?

Mn... this change may no longer be needed.  I'll test and see what happens.

> > Source/WebCore/editing/SelectionController.cpp:287
> > -    if ((type == EndPointIsStart && static_cast<unsigned>(position.offsetInContainerNode()) >= offset + oldLength)
> > +    if (((type == EndPointIsStart || oldLength) && static_cast<unsigned>(position.offsetInContainerNode()) >= offset + oldLength)
> >          || (type == EndPointIsEnd && static_cast<unsigned>(position.offsetInContainerNode()) > offset + oldLength))
> 
> I find this function and change quite confusing. I can almost understand it given the change log comments. Could you add some “why” comments to the source file?
>
> All these calls to static_cast<unsigned>(position.offsetInContainerNode()) make me think the whole function would be way easier to read if that was put in a local variable.

Will do.

> > Source/WebCore/editing/SelectionController.cpp:321
> > +        // Since this function can be called while recalculating style, we do as little as possible.
> > +        // Notify IME of the selection change so that it clears the current composition.
> 
> I don’t understand this comment. The things that need to be done, need to be done. So what does it mean to do as little as possible? We should *always* do as little as possible. Maybe it would be clearer if you said something more specific? I am quite worried about this!

"do as little as possible" wasn't particularly good phrasing.  What I meant to say is that we can't call setSelection here due to the issues addressed around http://trac.webkit.org/browser/trunk/Source/WebCore/editing/SelectionController.cpp#L245.  The comment also refers to the fact we don't want to disrupt rendering engine here because it can be called during style recalc.

> > LayoutTests/ChangeLog:16
> > +        Also fixed platform/mac/editing/input/selection-change-closes-typing.html
> > +        so that it calls setMarkedText with offset 0 and length 1 as supposed to
> > +        offset 1 and length 0.
> 
> Would you be willing to land this change first in a separate patch?

Will do.

[Darin Adler]

(In reply to comment #49)
> What I meant to say is that we can't call setSelection here due to the issues addressed around http://trac.webkit.org/browser/trunk/Source/WebCore/editing/SelectionController.cpp#L245.

If we can}t call setSelection, then the comment needs to state why it’s OK not to call setSelection.

> The comment also refers to the fact we don't want to disrupt the rendering engine here because it can be called during style recalc.

Good point, but it is a bit vague. Obviously we don’t want to disrupt anything! I approve of the sentiment but would like the comment to be as clear and precise as possible.

[Ryosuke Niwa]

(In reply to comment #50)
> (In reply to comment #49)
> > What I meant to say is that we can't call setSelection here due to the issues addressed around http://trac.webkit.org/browser/trunk/Source/WebCore/editing/SelectionController.cpp#L245.
> 
> If we can}t call setSelection, then the comment needs to state why it’s OK not to call setSelection.
> 
> > The comment also refers to the fact we don't want to disrupt the rendering engine here because it can be called during style recalc.
> 
> Good point, but it is a bit vague. Obviously we don’t want to disrupt anything! I approve of the sentiment but would like the comment to be as clear and precise as possible.

It turned out that we can call setSelection here.  Will post a patch soon.

[Ryosuke Niwa]

Created attachment 84363 [details]
Fixed per Darin's comment

[Ryosuke Niwa]

(In reply to comment #49)
> > > Source/WebCore/dom/CharacterData.cpp:157
> > > -    if (document()->frame())
> > > -        document()->frame()->selection()->textWillBeReplaced(this, offsetOfReplacedData, oldLength, newLength);
> > >      RefPtr<StringImpl> oldData = m_data;
> > >      m_data = newData;
> > >      updateRenderer(offsetOfReplacedData, oldLength);
> > > +    if (document()->frame())
> > > +        document()->frame()->selection()->textWillBeReplaced(this, offsetOfReplacedData, oldLength, newLength);
> > 
> > Would you be willing to land this change separately in its own patch?
> 
> Mn... this change may no longer be needed.  I'll test and see what happens.

Now I remember.  This change needs to be done for the bug 55354.

> > > Source/WebCore/editing/SelectionController.cpp:287
> > > -    if ((type == EndPointIsStart && static_cast<unsigned>(position.offsetInContainerNode()) >= offset + oldLength)
> > > +    if (((type == EndPointIsStart || oldLength) && static_cast<unsigned>(position.offsetInContainerNode()) >= offset + oldLength)
> > >          || (type == EndPointIsEnd && static_cast<unsigned>(position.offsetInContainerNode()) > offset + oldLength))
> > 
> > I find this function and change quite confusing. I can almost understand it given the change log comments. Could you add some “why” comments to the source file?

I rewrote the condition and added some comments.

> > > Source/WebCore/editing/SelectionController.cpp:321
> > > +        // Since this function can be called while recalculating style, we do as little as possible.
> > > +        // Notify IME of the selection change so that it clears the current composition.
> > 
> > I don’t understand this comment. The things that need to be done, need to be done. So what does it mean to do as little as possible? We should *always* do as little as possible. Maybe it would be clearer if you said something more specific? I am quite worried about this!
> 
> "do as little as possible" wasn't particularly good phrasing.  What I meant to say is that we can't call setSelection here due to the issues addressed around http://trac.webkit.org/browser/trunk/Source/WebCore/editing/SelectionController.cpp#L245.  The comment also refers to the fact we don't want to disrupt rendering engine here because it can be called during style recalc.

I call setSelection now.

[Ryosuke Niwa]

Thanks for the review, Darin.  Since this patch is likely to require massive rebaselines on non-Mac ports, I sent out an email to webkit-dev to ask for the best time to land this patch.

[Ryosuke Niwa]

Committed r80121: <http://trac.webkit.org/changeset/80121>

[WebKit Review Bot]

http://trac.webkit.org/changeset/80121 might have broken Qt Linux Release