Bug 146805 - [GTK] Crash when spell checker returns no guesses
Summary: [GTK] Crash when spell checker returns no guesses
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-09 13:46 PDT by Michael Catanzaro
Modified: 2015-07-11 20:48 PDT (History)
6 users (show)

See Also:


Attachments
Backtrace on master (r186333) (56.36 KB, text/plain)
2015-07-09 14:12 PDT, Michael Catanzaro
no flags Details
Patch (1.66 KB, patch)
2015-07-09 14:41 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.