Bug 74097 – [Chromium] Continue compositions at the current selection if composition node is removed.

• Component: New Bugs
• Status: RESOLVED WORKSFORME

[John Knottenbelt]

[Chromium] Continue compositions at the current selection if composition node is removed.

[John Knottenbelt]

Created attachment 118407 [details]
Patch

[John Knottenbelt]

Created attachment 118408 [details]
Patch

[John Knottenbelt]

Created attachment 126046 [details]
Patch

[Hironori Bono]

Greetings,

I have added a couple of WebKit reviewers because I'm not a reviewer.

Regards,

Hironori Bono

[Tony Chang]

Comment on attachment 126046 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1526
>          if (!node || !node->isContentEditable())
> -            return false;
> +            editor->cancelComposition();

Is it possible for there to be no current selection?  For example, if the node is no longer content editable.  Should there be a separate test for that case?

> LayoutTests/fast/events/ime-composition-events-002.html:47
> +    textInputController.setMarkedText('1', 0, 1);

You may need to skip this test on Qt and GTK+.  Last I checked, they don't yet implement textInputController.

[Ryosuke Niwa]

Comment on attachment 126046 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:15
> +        When this happens, the WebCore editor becomes stuck because it stays focused on
> +        this now removed node, on which a composition cannot be set because the node is
> +        no longer editable.

That sounds like a WebCore bug. I don't think we should be fixing it in Chromium code unless that's what all other ports do.

[Ryosuke Niwa]

+ap since he probably knows whether Mac and other ports have the same issue or not.

[John Knottenbelt]

Comment on attachment 126046 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:15
>> +        no longer editable.
> 
> That sounds like a WebCore bug. I don't think we should be fixing it in Chromium code unless that's what all other ports do.

I tried the layout test with mac webkit's DumpRenderTree on mac os x, and it passed fine (without my patch), so I think the problem is rather in how we are calling into WebCore rather than the WebCore editor code itself. I will change the ChangeLog to clarify.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1526
>> +            editor->cancelComposition();
> 
> Is it possible for there to be no current selection?  For example, if the node is no longer content editable.  Should there be a separate test for that case?

Good idea, I'll add one.

>> LayoutTests/fast/events/ime-composition-events-002.html:47
>> +    textInputController.setMarkedText('1', 0, 1);
> 
> You may need to skip this test on Qt and GTK+.  Last I checked, they don't yet implement textInputController.

Thanks for the heads up. I tried to build Qt and GKT today locally, but was set back by not having the right build dependencies. However, I did notice some seemingly non-trivial implementation of textInputController here: Tools/DumpRenderTree/gtk/TextInputController.cpp and Tools/DumpRenderTree/qt/TextInputControllerQt.cpp

[Tony Chang]

Comment on attachment 126046 [details]
Patch

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

r- to remove from the review queue while john adds another test case.

>>> LayoutTests/fast/events/ime-composition-events-002.html:47
>>> +    textInputController.setMarkedText('1', 0, 1);
>> 
>> You may need to skip this test on Qt and GTK+.  Last I checked, they don't yet implement textInputController.
> 
> Thanks for the heads up. I tried to build Qt and GKT today locally, but was set back by not having the right build dependencies. However, I did notice some seemingly non-trivial implementation of textInputController here: Tools/DumpRenderTree/gtk/TextInputController.cpp and Tools/DumpRenderTree/qt/TextInputControllerQt.cpp

Ah, you're right, it looks like textInputController.setMarkedText and insertText are implemented in both GTK and Qt.  My mistake!

[John Knottenbelt]

Created attachment 127860 [details]
Patch

[Hironori Bono]

Greetings,

Sorry for my slow response.
In my personal opinion, your looks good. But I would like to test this change with existing IMEs to check if your change does not cause any regressions or side effects tomorrow. I will update my comment when I finish testing your change.

Regards,

Hironori Bono

[Hironori Bono]

Greetings,

Unfortunately, this change causes an assertion error when quickly typing text with Microsoft IMEs on the "Search" box of facebook. (This crash seems to happen when typing text with Microsoft IMEs while facebook is rendering search suggestions.) The following list is the stack trace of this assertion error.

webkit.dll!WebCore::Node::isFocusable()  Line 890 + 0x32 bytes	C++
webkit.dll!WebCore::HTMLFormControlElement::isFocusable()  Line 305	C++
webkit.dll!WebCore::HTMLInputElement::isMouseFocusable()  Line 441 + 0x8 bytes	C++
webkit.dll!WebCore::FrameSelection::setFocusedNodeIfNeeded()  Line 1821 + 0x18 bytes	C++
webkit.dll!WebCore::FrameSelection::setSelection(const WebCore::VisibleSelection & newSelection={...}, unsigned int options=0, WebCore::FrameSelection::CursorAlignOnScroll align=AlignCursorOnScrollIfNeeded, WebCore::TextGranularity granularity=CharacterGranularity)  Line 291	C++
webkit.dll!WebCore::Editor::selectComposition()  Line 1279	C++
webkit.dll!WebCore::Editor::setComposition(const WTF::String & text={...}, const WTF::Vector<WebCore::CompositionUnderline,0> & underlines={...}, unsigned int selectionStart=4, unsigned int selectionEnd=4)  Line 1358	C++
webkit.dll!WebKit::WebViewImpl::setComposition(const WebKit::WebString & text={...}, const WebKit::WebVector<WebKit::WebCompositionUnderline> & underlines={...}, int selectionStart=4, int selectionEnd=4)  Line 1708 + 0x5a bytes	C++
content.dll!RenderWidget::OnImeSetComposition(const std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & text="とうｋｙ", const std::vector<WebKit::WebCompositionUnderline,std::allocator<WebKit::WebCompositionUnderline> > & underlines=[1]({startOffset=0 endOffset=4 color=4278190080 ...}), int selection_start=4, int selection_end=4)  Line 1263 + 0x5d bytes	C++
content.dll!RenderViewImpl::OnImeSetComposition(const std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > & text="とうｋｙ", const std::vector<WebKit::WebCompositionUnderline,std::allocator<WebKit::WebCompositionUnderline> > & underlines=[1]({startOffset=0 endOffset=4 color=4278190080 ...}), int selection_start=4, int selection_end=4)  Line 4723	C++
content.dll!DispatchToMethod<RenderWidget,void (__thiscall RenderWidget::*)(std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > const &,std::vector<WebKit::WebCompositionUnderline,std::allocator<WebKit::WebCompositionUnderline> > const &,int,int),std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> >,std::vector<WebKit::WebCompositionUnderline,std::allocator<WebKit::WebCompositionUnderline> >,int,int>(RenderWidget * obj=0x007363e8, void (const std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &, const std::vector<WebKit::WebCompositionUnderline,std::allocator<WebKit::WebCompositionUnderline> > &, int, int)* method=0x598fd5e7, const Tuple4<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> >,std::vector<WebKit::WebCompositionUnderline,std::allocator<WebKit::WebCompositionUnderline> >,int,int> & arg={...})  Line 566 + 0x34 bytes	C++
content.dll!ViewMsg_ImeSetComposition::Dispatch<RenderWidget,RenderWidget,void (__thiscall RenderWidget::*)(std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > const &,std::vector<WebKit::WebCompositionUnderline,std::allocator<WebKit::WebCompositionUnderline> > const &,int,int)>(const IPC::Message * msg=0x04f6ced4, RenderWidget * obj=0x007363e8, RenderWidget * sender=0x007363e8, void (const std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &, const std::vector<WebKit::WebCompositionUnderline,std::allocator<WebKit::WebCompositionUnderline> > &, int, int)* func=0x598fd5e7)  Line 977 + 0x81 bytes	C++
content.dll!RenderWidget::OnMessageReceived(const IPC::Message & message={...})  Line 216 + 0x9f bytes	C++
content.dll!RenderViewImpl::OnMessageReceived(const IPC::Message & message={...})  Line 817 + 0xc bytes	C++
content.dll!MessageRouter::RouteMessage(const IPC::Message & msg={...})  Line 46 + 0x13 bytes	C++
content.dll!MessageRouter::OnMessageReceived(const IPC::Message & msg={...})  Line 38 + 0x13 bytes	C++
content.dll!ChildThread::OnMessageReceived(const IPC::Message & msg={...})  Line 201 + 0x17 bytes	C++
ipc.dll!IPC::ChannelProxy::Context::OnDispatchMessage(const IPC::Message & message={...})  Line 268 + 0x19 bytes	C++
ipc.dll!base::internal::RunnableAdapter<void (__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const &)>::Run(IPC::ChannelProxy::Context * object=0x006da8f0, const IPC::Message & a1={...})  Line 188 + 0x21 bytes	C++
ipc.dll!base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void (__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const &)>,void __cdecl(IPC::ChannelProxy::Context * const &,IPC::Message const &)>::MakeItSo(base::internal::RunnableAdapter<void (__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const &)> runnable={...}, IPC::ChannelProxy::Context * const & a1=0x006da8f0, const IPC::Message & a2={...})  Line 897	C++
ipc.dll!base::internal::Invoker<2,base::internal::BindState<base::internal::RunnableAdapter<void (__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const &)>,void __cdecl(IPC::ChannelProxy::Context *,IPC::Message const &),void __cdecl(IPC::ChannelProxy::Context *,IPC::Message)>,void __cdecl(IPC::ChannelProxy::Context *,IPC::Message const &)>::Run(base::internal::BindStateBase * base=0x04f6ceb8)  Line 1254 + 0x2a bytes	C++
base.dll!base::Callback<void __cdecl(void)>::Run()  Line 272 + 0xe bytes	C++
base.dll!MessageLoop::RunTask(const base::PendingTask & pending_task={...})  Line 460	C++
base.dll!MessageLoop::DeferOrRunPendingTask(const base::PendingTask & pending_task={...})  Line 473	C++
base.dll!MessageLoop::DoWork()  Line 660 + 0xc bytes	C++
base.dll!base::MessagePumpDefault::Run(base::MessagePump::Delegate * delegate=0x0028f5d0)  Line 28 + 0xf bytes	C++
base.dll!MessageLoop::RunInternal()  Line 417 + 0x2a bytes	C++
base.dll!MessageLoop::RunHandler()  Line 391	C++
base.dll!MessageLoop::Run()  Line 301	C++
content.dll!RendererMain(const content::MainFunctionParams & parameters={...})  Line 241 + 0x19 bytes	C++
content.dll!`anonymous namespace'::RunNamedProcessTypeMain(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & process_type="renderer", const content::MainFunctionParams & main_function_params={...}, content::ContentMainDelegate * delegate=0x0028fa68)  Line 271 + 0x12 bytes	C++
content.dll!`anonymous namespace'::ContentMainRunnerImpl::Run()  Line 500 + 0x14 bytes	C++
content.dll!content::ContentMain(HINSTANCE__ * instance=0x013a0000, sandbox::SandboxInterfaceInfo * sandbox_info=0x0028fbd4, content::ContentMainDelegate * delegate=0x0028fa68)  Line 35 + 0x1a bytes	C++
chrome.dll!ChromeMain(HINSTANCE__ * instance=0x013a0000, sandbox::SandboxInterfaceInfo * sandbox_info=0x0028fbd4)  Line 28 + 0x14 bytes	C++
chrome.exe!MainDllLoader::Launch(HINSTANCE__ * instance=0x013a0000, sandbox::SandboxInterfaceInfo * sbox_info=0x0028fbd4)  Line 424 + 0xd bytes	C++
chrome.exe!wWinMain(HINSTANCE__ * instance=0x013a0000, HINSTANCE__ * __formal=0x00000000, HINSTANCE__ * __formal=0x00000000, HINSTANCE__ * __formal=0x00000000)  Line 36 + 0x10 bytes	C++
chrome.exe!__tmainCRTStartup()  Line 578 + 0x35 bytes	C
chrome.exe!wWinMainCRTStartup()  Line 403	C
kernel32.dll!757c339a() 	

Regards,

Hironori Bono

[John Knottenbelt]

Thanks for the testing, Hironori-san! I'm flying back to London tomorrow, but next week I will try to make a test case for this crash.

[Adam Barth]

Comment on attachment 127860 [details]
Patch

Sounds like we should mark this patch r- for now while we investigate the crash.

[Adam Barth]

Removing AndroidHitList because John seems to have abandoned this change.  John, if you plan to pick this up again and want to put it back on my radar, please feel free to add AndroidHitList again.

[Adam Barth]

This patch does not appear to be needed anymore.