Bug 173968

Summary: REGRESSION(r218896): ASSERT in WebPageProxy::dataCallback
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, bugs-noreply, buildbot, cdumez, commit-queue, dbates, japhet, mcatanzaro, rniwa
Priority: P2 Keywords: Gtk, Regression
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Carlos Garcia Campos 2017-06-29 06:34:48 PDT 
Several GTK+ unit tests are crashing because of this. The problem is that WebPageProxy::getLoadDecisionForIcon() sends 0 as callback ID when the decision is to not load the icon. Since r218896 we always notify the client even when the decision is not not load the icon, in which case the UI doesn't really expect a callback. When WebPageProxy::dataCallback is called with a 0 callback, CallbackMap::take crashes in RELEASE_ASSERT(callbackID).

1   0x7f3a21cf2c67 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x17) [0x7f3a21cf2c67]
2   0x7f3a2366b09d /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit12WebPageProxy12dataCallbackERKN3IPC13DataReferenceEm+0x15d) [0x7f3a2366b09d]
3   0x7f3a2397d003 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit12WebPageProxy17didReceiveMessageERN3IPC10ConnectionERNS1_7DecoderE+0x1d63) [0x7f3a2397d003]
4   0x7f3a235b0fc9 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC18MessageReceiverMap15dispatchMessageERNS_10ConnectionERNS_7DecoderE+0x299) [0x7f3a235b0fc9]
5   0x7f3a23692002 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit15WebProcessProxy17didReceiveMessageERN3IPC10ConnectionERNS1_7DecoderE+0x12) [0x7f3a23692002]
6   0x7f3a235ace8b /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection15dispatchMessageESt10unique_ptrINS_7DecoderESt14default_deleteIS2_EE+0x6b) [0x7f3a235ace8b]
7   0x7f3a235addbc /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection18dispatchOneMessageEv+0xec) [0x7f3a235addbc]
8   0x7f3a21d066dd /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF7RunLoop11performWorkEv+0x18d) [0x7f3a21d066dd]
9   0x7f3a21d3c039 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18(+0xe23039) [0x7f3a21d3c039]
10  0x7f3a26d0e5ca /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_main_context_dispatch+0x13a) [0x7f3a26d0e5ca]
11  0x7f3a26d0e948 /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(+0x49948) [0x7f3a26d0e948]
12  0x7f3a26d0ec62 /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_main_loop_run+0xc2) [0x7f3a26d0ec62]
13  0x7f3a276c4623 ./Tools/gtk/../../WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestResources(+0x8e5623) [0x7f3a276c4623]
14  0x7f3a26d34039 /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(+0x6f039) [0x7f3a26d34039]
15  0x7f3a26d34207 /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(+0x6f207) [0x7f3a26d34207]
16  0x7f3a26d34207 /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(+0x6f207) [0x7f3a26d34207]
17  0x7f3a26d343fe /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_test_run_suite+0xbe) [0x7f3a26d343fe]
18  0x7f3a26d34421 /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_test_run+0x11) [0x7f3a26d34421]
19  0x7f3a276c2514 ./Tools/gtk/../../WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestResources(main+0x114) [0x7f3a276c2514]
20  0x7f3a1b3222b1 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf1) [0x7f3a1b3222b1]
21  0x7f3a276c2a8a ./Tools/gtk/../../WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestResources(_start+0x2a) [0x7f3a276c2a8a]
Comment 1 Carlos Garcia Campos 2017-06-29 06:39:46 PDT 
Created attachment 314131 [details]
Patch
Comment 2 Michael Catanzaro 2017-06-29 07:45:22 PDT 
Comment on attachment 314131 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:1717
> +    RELEASE_ASSERT(callbackIdentifier);
> +    RELEASE_ASSERT(m_frame);

IMO there's no particular reason for these to be release asserts rather than normal asserts.
Comment 3 Carlos Garcia Campos 2017-06-29 08:21:10 PDT 
(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 314131 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314131&action=review
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1717
> > +    RELEASE_ASSERT(callbackIdentifier);
> > +    RELEASE_ASSERT(m_frame);
> 
> IMO there's no particular reason for these to be release asserts rather than
> normal asserts.

No that I haven't added it, but moved and it was already a release assert. Then I added the frame one, and used release assert too for consistency.
Comment 4 Brady Eidson 2017-06-29 10:30:57 PDT 
Comment on attachment 314131 [details]
Patch

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

>>> Source/WebCore/loader/DocumentLoader.cpp:1717
>>> +    RELEASE_ASSERT(m_frame);
>> 
>> IMO there's no particular reason for these to be release asserts rather than normal asserts.
> 
> No that I haven't added it, but moved and it was already a release assert. Then I added the frame one, and used release assert too for consistency.

Yup, this is fine.
Comment 5 WebKit Commit Bot 2017-06-29 12:19:20 PDT 
Comment on attachment 314131 [details]
Patch

Clearing flags on attachment: 314131

Committed r218954: <http://trac.webkit.org/changeset/218954>
Comment 6 WebKit Commit Bot 2017-06-29 12:19:22 PDT 
All reviewed patches have been landed.  Closing bug.