Bug 146805 – [GTK] Crash when spell checker returns no guesses

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Type into a text form:

WebKitWebView

Then type a space or enter, so that the spellchecker underlines WebKitWebView with the red squiggly line. Right click on it. The browser will crash. It seems to crash for any word when there are no spelling suggestions.

Truncated backtrace:
Thread no. 1 (10 frames)
 #0 size at /usr/src/debug/webkitgtk-2.8.3/Source/WTF/wtf/Vector.h:651
 #2 WebKit::WebTextCheckerClient::guessesForWord at /usr/src/debug/webkitgtk-2.8.3/Source/WebKit2/UIProcess/WebTextCheckerClient.cpp:160
 #3 WebKit::TextChecker::getGuessesForWord at /usr/src/debug/webkitgtk-2.8.3/Source/WebKit2/UIProcess/gtk/TextCheckerGtk.cpp:128
 #4 WebKit::WebPageProxy::getGuessesForWord at /usr/src/debug/webkitgtk-2.8.3/Source/WebKit2/UIProcess/WebPageProxy.cpp:4255
 #5 callMemberFunctionImpl<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WTF::String const&, WTF::String const&, WTF::Vector<WTF::String>&), std::tuple<WTF::String, WTF::String>, 0ul, 1ul, std::tuple<WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow> >, 0ul> at /usr/src/debug/webkitgtk-2.8.3/Source/WebKit2/Platform/IPC/HandleMessage.h:30
 #6 callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WTF::String const&, WTF::String const&, WTF::Vector<WTF::String>&), std::tuple<WTF::String, WTF::String>, std::make_index_sequence<2ul>, std::tuple<WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow> >, std::make_index_sequence<1ul> > at /usr/src/debug/webkitgtk-2.8.3/Source/WebKit2/Platform/IPC/HandleMessage.h:36
 #7 handleMessage<Messages::WebPageProxy::GetGuessesForWord, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WTF::String const&, WTF::String const&, WTF::Vector<WTF::String>&)> at /usr/src/debug/webkitgtk-2.8.3/Source/WebKit2/Platform/IPC/HandleMessage.h:105
 #8 WebKit::WebPageProxy::didReceiveSyncMessage at /usr/src/debug/webkitgtk-2.8.3/x86_64-redhat-linux-gnu/DerivedSources/WebKit2/WebPageProxyMessageReceiver.cpp:1283
 #9 IPC::MessageReceiverMap::dispatchSyncMessage at /usr/src/debug/webkitgtk-2.8.3/Source/WebKit2/Platform/IPC/MessageReceiverMap.cpp:104
 #10 WebKit::ChildProcessProxy::dispatchSyncMessage at /usr/src/debug/webkitgtk-2.8.3/Source/WebKit2/Shared/ChildProcessProxy.cpp:129

Full backtrace downstream. The backtrace in trunk is different but the crash still occurs.

[Martin Robinson]

Can you confirm this with a recent build? I do not think that WebKitGTK+ uses WebTextCheckerClient.cpp any longer.

[Michael Catanzaro]

Created attachment 256517 [details]
Backtrace on master (r186333)

[Michael Catanzaro]

Created attachment 256522 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 256522 [details]
Patch

Is it possible to remove the default case here?

[Michael Catanzaro]

(In reply to comment #5)
> Comment on attachment 256522 [details]
> Patch
> 
> Is it possible to remove the default case here?

The WebCore enumeration has about 50 more elements than the WebKitGTK+ enumeration, so to remove the default case, we would have to list those 50 elements above the ASSERT_NOT_REACHED() (to avoid compiler warnings from -Wswitch). So I prefer to keep the default case for this switch; it's really not worth the pain of having to update it whenever a new item is added to the WebCore enumeration.

It is possible to remove the default case from the two other switch cases in this file, though.

(The advantage of removing the default case is that -Wswitch will warn at build time if a value is missing.)

Note: I audited them all to confirm this was the only mistake in the file.

[Martin Robinson]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 256522 [details]
> > Patch
> > 
> > Is it possible to remove the default case here?
> 
> The WebCore enumeration has about 50 more elements than the WebKitGTK+
> enumeration, so to remove the default case, we would have to list those 50
> elements above the ASSERT_NOT_REACHED() (to avoid compiler warnings from
> -Wswitch). So I prefer to keep the default case for this switch; it's really
> not worth the pain of having to update it whenever a new item is added to
> the WebCore enumeration.


I wonder if a warning would be better here than an assertion? I guess this only affects debug builds...

[Michael Catanzaro]

> I wonder if a warning would be better here than an assertion?

I think an assertion is best, to make the issue more noticeable.

> I guess this only affects debug builds...

In master, yes. Release builds of 2.8 do crash, but this is two separate bugs that just happen to occur in the same situation.

[WebKit Commit Bot]

Comment on attachment 256522 [details]
Patch

Clearing flags on attachment: 256522

Committed r186653: <http://trac.webkit.org/changeset/186653>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.