Bug 60090 – REGRESSION(r73886): Frequent crashes in replaceSelectionWithFragment

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

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

[Alexey Proskuryakov]

Is this reproducible?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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.

[Hajime Morrita]

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...

[Hajime Morrita]

Created attachment 92921 [details]
Patch

[Ryosuke Niwa]

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.

[Hajime Morrita]

Created attachment 92924 [details]
Patch

[Hajime Morrita]

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.

[Ryosuke Niwa]

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.

[Hajime Morrita]

Committed r86132: <http://trac.webkit.org/changeset/86132>

[Ademar Reis]

Revision r86132 cherry-picked into qtwebkit-2.2 with commit f38046d <http://gitorious.org/webkit/qtwebkit/commit/f38046d>

[Hajime Morrita]

(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.