Bug 173968 – REGRESSION(r218896): ASSERT in WebPageProxy::dataCallback

• Component: Page Loading
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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]

[Carlos Garcia Campos]

Created attachment 314131 [details]
Patch

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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

[Brady Eidson]

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.

[WebKit Commit Bot]

Comment on attachment 314131 [details]
Patch

Clearing flags on attachment: 314131

Committed r218954: <http://trac.webkit.org/changeset/218954>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.