Bug 146805

Summary: [GTK] Crash when spell checker returns no guesses
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, commit-queue, gustavo, mcatanzaro, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
See Also: https://bugzilla.redhat.com/show_bug.cgi?id=1234629
https://bugs.webkit.org/show_bug.cgi?id=146885
Attachments:
Description Flags
Backtrace on master (r186333)
none
Patch none

Description Michael Catanzaro 2015-07-09 13:46:49 PDT 
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.
Comment 1 Martin Robinson 2015-07-09 13:49:48 PDT 
Can you confirm this with a recent build? I do not think that WebKitGTK+ uses WebTextCheckerClient.cpp any longer.
Comment 2 Michael Catanzaro 2015-07-09 14:12:29 PDT 
Created attachment 256517 [details]
Backtrace on master (r186333)
Comment 3 Michael Catanzaro 2015-07-09 14:41:17 PDT 
Created attachment 256522 [details]
Patch
Comment 4 WebKit Commit Bot 2015-07-09 14:43:43 PDT 
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
Comment 5 Martin Robinson 2015-07-09 14:50:46 PDT 
Comment on attachment 256522 [details]
Patch

Is it possible to remove the default case here?
Comment 6 Michael Catanzaro 2015-07-09 14:58:27 PDT 
(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.
Comment 7 Martin Robinson 2015-07-09 15:02:13 PDT 
(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...
Comment 8 Michael Catanzaro 2015-07-09 15:59:58 PDT 
> 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.
Comment 9 WebKit Commit Bot 2015-07-09 17:10:18 PDT 
Comment on attachment 256522 [details]
Patch

Clearing flags on attachment: 256522

Committed r186653: <http://trac.webkit.org/changeset/186653>
Comment 10 WebKit Commit Bot 2015-07-09 17:10:22 PDT 
All reviewed patches have been landed.  Closing bug.