Bug 60090

Summary: REGRESSION(r73886): Frequent crashes in replaceSelectionWithFragment
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Major CC: adele, ademar, ap, dcheng, enrica, levin, morrita, ojan
Priority: P2 Keywords: EasyFix, NeedsReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch rniwa: review+

Description Ryosuke Niwa 2011-05-03 17:22:24 PDT 
It's missing a null pointer check for m_spellChecker.

stack trace:

Thread 0 *CRASHED* ( EXC_BAD_ACCESS / KERN_INVALID_ADDRESS @ 0x1f0f6602 )

0x1f0f6602			
0x01d36cf5	 [Google Chrome Framework	 - Editor.cpp:443]	WebCore::Editor::replaceSelectionWithFragment
0x01d3c299	 [Google Chrome Framework	 - Editor.cpp:448]	WebCore::Editor::replaceSelectionWithText
0x01d43381	 [Google Chrome Framework	 - Editor.cpp:197]	WebCore::Editor::handleTextEvent
0x01eb038f	 [Google Chrome Framework	 - EventHandler.cpp:2762]	WebCore::EventHandler::defaultTextInputEventHandler
0x01cd0dcf	 [Google Chrome Framework	 - Node.cpp:3054]	WebCore::Node::defaultEventHandler
0x01ccc191	 [Google Chrome Framework	 - Node.cpp:2748]	WebCore::Node::dispatchGenericEvent
0x01ccc988	 [Google Chrome Framework	 - Node.cpp:2646]	WebCore::Node::dispatchEvent
0x01cba659	 [Google Chrome Framework	 - EventTarget.cpp:297]	WebCore::EventTarget::dispatchEvent
0x01d3a646	 [Google Chrome Framework	 - Editor.cpp:372]	WebCore::Editor::pasteAsPlainText
0x01d3c6ad	 [Google Chrome Framework	 - Editor.cpp:393]	WebCore::Editor::pasteAsPlainTextWithPasteboard
0x01d3c944	 [Google Chrome Framework	 - Editor.cpp:1299]	WebCore::Editor::paste
0x01d485b5	 [Google Chrome Framework	 - EditorCommand.cpp:888]	WebCore::executePaste
0x01d4944e	 [Google Chrome Framework	 - EditorCommand.cpp:1644]	WebCore::Editor::Command::execute
0x01662c9b	 [Google Chrome Framework	 - WebFrameImpl.cpp:1199]	WebKit::WebFrameImpl::executeCommand
0x007e2fb1	 [Google Chrome Framework	 - render_view.cc:1562]	RenderView::OnPaste
0x007e4a83	 [Google Chrome Framework	 - ../base/tuple.h:558]	RenderView::OnMessageReceived
0x011ffae2	 [Google Chrome Framework	 - message_router.cc:46]	MessageRouter::RouteMessage
0x011ff683	 [Google Chrome Framework	 - message_router.cc:38]	MessageRouter::OnMessageReceived
0x011f0881	 [Google Chrome Framework	 - child_thread.cc:167]	ChildThread::OnMessageReceived
0x0120f38a	 [Google Chrome Framework	 - ../base/tuple.h:551]	RunnableMethod<IPC::ChannelProxy::Context,void (IPC::ChannelProxy::Context::*)(const IPC::Message&),Tuple1<IPC::Message> >::Run
Comment 1 Alexey Proskuryakov 2011-05-04 11:23:08 PDT 
Is this reproducible?
Comment 2 Ryosuke Niwa 2011-05-04 11:27:44 PDT 
(In reply to comment #1)
> Is this reproducible?

I don't have a reduction but we're getting a lot of crash reports for Chrome 12.
Comment 3 Ryosuke Niwa 2011-05-04 12:04:07 PDT 
It seems like this is happening inside Chrome extensions. Maybe we don't have a spell checker inside extensions' renderers?

Regardless, this is a very easy fix.  We just need to do a null pointer check.
Comment 4 Hajime Morrita 2011-05-09 17:14:34 PDT 
Hmm. m_spellChecker should never be null. (It is always instantiated at the constructor.)
I'm looking, but simple adding null-check might not be sufficient...
Comment 5 Hajime Morrita 2011-05-09 22:25:55 PDT 
Created attachment 92921 [details]
Patch
Comment 6 Ryosuke Niwa 2011-05-09 22:37:26 PDT 
Comment on attachment 92921 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        SpellChecker uses TextCheckerClient, which belongs Page object,
> +        which is possibly destroyed during SpellChecker's lifetime.

Wow! That sounds like a serious issue. We have a raw pointer in SpellChecker.h

> Source/WebCore/editing/SpellChecker.cpp:119
>      if (!initRequest(node))
>          return;
> -    m_client->requestCheckingOfString(this, m_requestSequence, mask, m_requestText);
> +    client()->requestCheckingOfString(this, m_requestSequence, mask, m_requestText);

We should should have an early exit here as well when the page is gone.

> Source/WebCore/editing/SpellChecker.h:59
>      TextCheckerClient* m_client;

I don't think it's safe to store this pointer.
Comment 7 Hajime Morrita 2011-05-09 22:57:18 PDT 
Created attachment 92924 [details]
Patch
Comment 8 Hajime Morrita 2011-05-09 22:58:42 PDT 
Hi Ryosuke, thank you for the lightning review!

> > Source/WebCore/editing/SpellChecker.cpp:119
> >      if (!initRequest(node))
> >          return;
> > -    m_client->requestCheckingOfString(this, m_requestSequence, mask, m_requestText);
> > +    client()->requestCheckingOfString(this, m_requestSequence, mask, m_requestText);
> 
> We should should have an early exit here as well when the page is gone.
The check is done by caller side using canCheckAsynchronously().
This function also asserts it at the beginning.

> 
> > Source/WebCore/editing/SpellChecker.h:59
> >      TextCheckerClient* m_client;
> 
> I don't think it's safe to store this pointer.
Makes sense. removed.
Comment 9 Ryosuke Niwa 2011-05-09 23:01:13 PDT 
Comment on attachment 92924 [details]
Patch

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

> Source/WebCore/editing/SpellChecker.cpp:116
>      if (!initRequest(node))
>          return;

I see. initRequest asserts that canCheckAsynchronously returns true.
Comment 10 Hajime Morrita 2011-05-10 00:47:08 PDT 
Committed r86132: <http://trac.webkit.org/changeset/86132>
Comment 11 Ademar Reis 2011-05-10 13:20:38 PDT 
Revision r86132 cherry-picked into qtwebkit-2.2 with commit f38046d <http://gitorious.org/webkit/qtwebkit/commit/f38046d>
Comment 12 Hajime Morrita 2011-05-11 01:13:17 PDT 
(In reply to comment #11)
> Revision r86132 cherry-picked into qtwebkit-2.2 with commit f38046d <http://gitorious.org/webkit/qtwebkit/commit/f38046d>
Ademar, thanks for caring this.