Bug 76911 – [GTK] WebKitWebView won't work in a GtkOffscreenWindow

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Claudio Saavedra]

GtkOffscreenWindow, although a GtkWindow, shouldn't be used with standard GtkWindow API. WebKit::ChromeClient makes several calls to gtk_window_* methods on the toplevel that seem to return garbage when used with a GtkOffscreenWindow. As a result of this, it's not possible to use a WebKitWebView inside a GtkOffscreenWindow.

The attached example shows the issue. The stacktrace is this:


#0  0x0000000000000000 in ?? ()
#1  0x00007ffff5423014 in gdk_window_get_frame_extents (window=0x647480, rect=0x7fffffffc1e0) at gdkwindow.c:10092
#2  0x00007ffff624f95b in gtk_window_get_position (window=0x7e6090, root_x=0x7fffffffc2a0, root_y=0x7fffffffc2a4) at gtkwindow.c:4613
#3  0x00007ffff69f349d in WebKit::ChromeClient::windowRect() () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#4  0x00007ffff6fa462e in WebCore::Chrome::windowRect() const () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#5  0x00007ffff6a0f304 in webkitViewportAttributesRecompute () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#6  0x00007ffff6fd45fc in WebCore::Frame::setDocument(WTF::PassRefPtr<WebCore::Document>) () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#7  0x00007ffff6f3d133 in WebCore::DocumentWriter::begin(WebCore::KURL const&, bool, WebCore::Document*) () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#8  0x00007ffff6f43b94 in WebCore::FrameLoader::receivedFirstData() () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#9  0x00007ffff6f3df88 in WebCore::DocumentWriter::setEncoding(WTF::String const&, bool) () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#10 0x00007ffff6f328af in WebCore::DocumentLoader::commitData(char const*, unsigned long) () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#11 0x00007ffff6a00f1e in WebKit::FrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#12 0x00007ffff6f327dd in WebCore::DocumentLoader::commitLoad(char const*, int) () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#13 0x00007ffff6f79b98 in WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#14 0x00007ffff6f65175 in WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#15 0x00007ffff6f7904e in WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#16 0x00007ffff70935ea in WebCore::readCallback(_GObject*, _GAsyncResult*, void*) () from /opt/gnome-3.0/lib64/libwebkitgtk-3.0.so.0
#17 0x00007ffff4325cd3 in async_ready_callback_wrapper (source_object=0x7c78c0, res=0x866d80, user_data=0x7fffe8100870) at ginputstream.c:470
#18 0x00007ffff433f0c6 in g_simple_async_result_complete (simple=0x866d80) at gsimpleasyncresult.c:744
#19 0x00007ffff5c7c674 in read_async_done (stream=0x7c78c0) at soup-http-input-stream.c:691
#20 0x00007ffff5c7b8a8 in soup_http_input_stream_got_chunk (msg=0x7f5ae0, chunk_buffer=0x853130, stream=0x7c78c0) at soup-http-input-stream.c:298
#21 0x00007ffff4080821 in g_cclosure_marshal_VOID__BOXED (closure=0x826c20, return_value=0x0, n_param_values=2, param_values=0x843460, invocation_hint=0x7fffffffcb00, marshal_data=0x0) at gmarshal.c:574
#22 0x00007ffff407df5a in g_closure_invoke (closure=0x826c20, return_value=0x0, n_param_values=2, param_values=0x843460, invocation_hint=0x7fffffffcb00) at gclosure.c:774
#23 0x00007ffff4097cc5 in signal_emit_unlocked_R (node=0x81ddc0, detail=0, instance=0x7f5ae0, emission_return=0x0, instance_and_params=0x843460) at gsignal.c:3302
#24 0x00007ffff4096ebf in g_signal_emit_valist (instance=0x7f5ae0, signal_id=325, detail=0, var_args=0x7fffffffcd88) at gsignal.c:3033
#25 0x00007ffff4097417 in g_signal_emit (instance=0x7f5ae0, signal_id=325, detail=0) at gsignal.c:3090
#26 0x00007ffff5c7f749 in soup_message_got_chunk (msg=0x7f5ae0, chunk=0x853130) at soup-message.c:1082
#27 0x00007ffff5c84889 in io_handle_sniffing (msg=0x7f5ae0, done_reading=0) at soup-message-io.c:243
#28 0x00007ffff5c85003 in read_body_chunk (msg=0x7f5ae0) at soup-message-io.c:469
#29 0x00007ffff5c8629f in io_read (sock=0x8142f0, msg=0x7f5ae0) at soup-message-io.c:989
#30 0x00007ffff5c86b10 in io_unpause_internal (msg=0x7f5ae0) at soup-message-io.c:1214
#31 0x00007ffff3d7d3cd in g_idle_dispatch (source=0x8279e0, callback=0x7ffff5c86980 <io_unpause_internal>, user_data=0x7f5ae0) at gmain.c:4632
#32 0x00007ffff3d7aca3 in g_main_dispatch (context=0x64bda0) at gmain.c:2513
#33 0x00007ffff3d7b964 in g_main_context_dispatch (context=0x64bda0) at gmain.c:3050
#34 0x00007ffff3d7bb47 in g_main_context_iterate (context=0x64bda0, block=1, dispatch=1, self=0x82cf30) at gmain.c:3121
#35 0x00007ffff3d7bf70 in g_main_loop_run (loop=0x82f370) at gmain.c:3315
#36 0x00007ffff6096485 in gtk_main () at gtkmain.c:1162
#37 0x0000000000400a58 in main ()

[Claudio Saavedra]

Created attachment 123728 [details]
simple test program

[Claudio Saavedra]

Created attachment 123730 [details]
[GTK] WebKitWebView won't work in a GtkOffscreenWindow

[GTK] WebKitWebView won't work in a GtkOffscreenWindow
        https://bugs.webkit.org/show_bug.cgi?id=76911

        Reviewed by NOBODY (OOPS!).

        * WebCoreSupport/ChromeClientGtk.cpp:
        (WebKit::ChromeClient::windowRect):
        (WebKit::ChromeClient::setWindowRect):
        Do not call GtkWindow API in GtkOffscreenWindow
        * tests/testwebview.c: Add a simple test for offscreen windows.

[Martin Robinson]

Comment on attachment 123730 [details]
[GTK] WebKitWebView won't work in a GtkOffscreenWindow

This patch is awesome. The only thing I wonder is if WebKit2 has similar problems. Do you mind double checking that?

[Claudio Saavedra]

(In reply to comment #3)
> (From update of attachment 123730 [details])
> This patch is awesome. The only thing I wonder is if WebKit2 has similar problems. Do you mind double checking that?

For the same usecase, WK2 is not crashing. However, I see one place where the same assumption is made, but I don't really know how to check it:

 WebKitUIClient::getWindowFrame(WKPageRef page, const void*)

[Martin Robinson]

Comment on attachment 123730 [details]
[GTK] WebKitWebView won't work in a GtkOffscreenWindow

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

Thanks. I think you just need to make the same fix in WebKit2 and fix the few style nits below.

> Source/WebKit/gtk/tests/testwebview.c:369
> +    GtkWidget* web_view;

I think your asterisk is in the wrong place here.

> Source/WebKit/gtk/tests/testwebview.c:374
> +    window = gtk_offscreen_window_new();
> +    web_view = webkit_web_view_new();

Just define the variables where you use them.

[Claudio Saavedra]

Created attachment 124589 [details]
[GTK] WebKitWebView won't work in a GtkOffscreenWindow

[GTK] WebKitWebView won't work in a GtkOffscreenWindow
        https://bugs.webkit.org/show_bug.cgi?id=76911

        Reviewed by NOBODY (OOPS!).

        * WebCoreSupport/ChromeClientGtk.cpp:
        (WebKit::ChromeClient::windowRect):
        (WebKit::ChromeClient::setWindowRect):
        Do not call GtkWindow API in GtkOffscreenWindow
        * tests/testwebview.c: Add a simple test for offscreen windows.

[WebKit Review Bot]

Attachment 124589 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:114:  Extra space before ( in function call  [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c"
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review 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

[Claudio Saavedra]

Created attachment 124593 [details]
[GTK] WebKitWebView won't work in a GtkOffscreenWindow

[GTK] WebKitWebView won't work in a GtkOffscreenWindow
        https://bugs.webkit.org/show_bug.cgi?id=76911

        Reviewed by NOBODY (OOPS!).

        * WebCoreSupport/ChromeClientGtk.cpp:
        (WebKit::ChromeClient::windowRect):
        (WebKit::ChromeClient::setWindowRect):
        Do not call GtkWindow API in GtkOffscreenWindow
        * tests/testwebview.c: Add a simple test for offscreen windows.

[Martin Robinson]

Comment on attachment 124593 [details]
[GTK] WebKitWebView won't work in a GtkOffscreenWindow

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

> Source/WebKit/gtk/tests/testwebview.c:380
> +    loop = g_main_loop_new(NULL, TRUE);
> +
> +    GtkWidget *window = gtk_offscreen_window_new();
> +    GtkWidget *web_view = webkit_web_view_new();
> +
> +    gtk_container_add(GTK_CONTAINER(window), web_view);
> +    gtk_widget_show_all(window);
> +    g_signal_connect(web_view, "notify::load-status", G_CALLBACK(idle_quit_loop_cb), NULL);
> +    webkit_web_view_load_uri(WEBKIT_WEB_VIEW(web_view), base_uri);
> +
> +    g_main_loop_run(loop);
> +

Seems that the main loop is leaking here? I'll fix when landing.

[Carlos Garcia Campos]

(In reply to comment #0)
> GtkOffscreenWindow, although a GtkWindow, shouldn't be used with standard GtkWindow API. 

Do you know why? isn't it a GtkOffscreenWindow bug?

WebKit::ChromeClient makes several calls to gtk_window_* methods on the toplevel that seem to return garbage when used with a GtkOffscreenWindow. As a result of this, it's not possible to use a WebKitWebView inside a GtkOffscreenWindow.
> 

We are assuming a toplevel is a Gtkwindow in several places, I'm not sure it's correct, we should probably always check gtk_widget_is_toplevel() && GTK_IS_WINDOW() but that wouldn't work in this case either.

[Martin Robinson]

Created attachment 124788 [details]
More extensive version of Claudio's patch

[WebKit Review Bot]

Attachment 124788 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/WebCore.exp.in
Auto-merging Source/WebKit/mac/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 124788 [details]
More extensive version of Claudio's patch

Attachment 124788 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11388075

[Carlos Garcia Campos]

Comment on attachment 124788 [details]
More extensive version of Claudio's patch

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

> Source/WebCore/platform/gtk/GtkUtilities.cpp:48
> +bool gtkWidgetIsValidToplevel(GtkWidget* widget)
> +{
> +    return gtk_widget_is_top_level(widget) && GTK_IS_WINDOW(widget) && !GTK_IS_OFFSCREEN_WINDOW(widget);
> +}

I would call it gtkWidgetIsToplevelWindow, because it can be a valid toplevel, but not a GtkWindow. Also, this is always called after gtk_widget_get_top_level, why not something like gtkWidgetGetToplevelWindow() that returns the toplevel widget or NULL. Or maybe split into two methods for the cases where you already have the toplevel widget.

bool gtkWidgetIsToplevelWindow(GtkWidget* widget)
{
    return gtk_widget_is_top_level(widget) && GTK_IS_WINDOW(widget) && !GTK_IS_OFFSCREEN_WINDOW(widget);
}

GtkWidget* gtkWidgetGetToplevelWindow(GtkWidget* widget)
{
    GtkWidget* toplevel = gtk_widget_get_toplevel(widget);
    return gtkWidgetIsToplevelWindow(toplevel) ? toplevel : 0;
}

> Source/WebKit/gtk/tests/testwebview.c:382
> +static void test_webkit_web_view_offscreen_window()
> +{
> +    loop = g_main_loop_new(NULL, TRUE);
> +
> +    GtkWidget *window = gtk_offscreen_window_new();
> +    GtkWidget *web_view = webkit_web_view_new();
> +
> +    gtk_container_add(GTK_CONTAINER(window), web_view);
> +    gtk_widget_show_all(window);
> +    g_signal_connect(web_view, "notify::load-status", G_CALLBACK(idle_quit_loop_cb), NULL);
> +    webkit_web_view_load_uri(WEBKIT_WEB_VIEW(web_view), base_uri);
> +
> +    g_main_loop_run(loop);
> +
> +    gtk_widget_destroy(window);
> +    g_main_loop_unref(loop);

You should comment here that you are testing that using a WebView inside a GtkOffscreenWindow doesn't crash, because it's not obvious what the test is for.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:903
> -    if (gtk_widget_is_toplevel(toplevel) && gtk_window_has_toplevel_focus(GTK_WINDOW(toplevel))) {
> +    if (gtkWidgetIsValidToplevel(toplevel)) {

You have removed the has_toplevel_focus check, isn't it necessary anymore?

> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:115
> -    if (gtk_widget_is_toplevel(window) && gtk_widget_get_visible(window)) {
> +    if (gtkWidgetIsValidToplevel(window)) {

I'd say the get_visible() check is still needed here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:227
> -    if (gtk_widget_is_toplevel(toplevel) && gtk_window_has_toplevel_focus(GTK_WINDOW(toplevel))) {
> +    if (gtkWidgetIsValidToplevel(toplevel)) {

Here you removed the has_toplevel_focus check too.

[Martin Robinson]

(In reply to comment #15)
> (From update of attachment 124788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124788&action=review
> 
> > Source/WebCore/platform/gtk/GtkUtilities.cpp:48
> > +bool gtkWidgetIsValidToplevel(GtkWidget* widget)
> > +{
> > +    return gtk_widget_is_top_level(widget) && GTK_IS_WINDOW(widget) && !GTK_IS_OFFSCREEN_WINDOW(widget);
> > +}
> 
> I would call it gtkWidgetIsToplevelWindow, because it can be a valid toplevel, but not a GtkWindow. Also, this is always called after gtk_widget_get_top_level, why not something like gtkWidgetGetToplevelWindow() that returns the toplevel widget or NULL. Or maybe split into two methods for the cases where you already have the toplevel widget.

I changed this to widgetIsOnscreenToplevelWindow.

> bool gtkWidgetIsToplevelWindow(GtkWidget* widget)
> {
>     return gtk_widget_is_top_level(widget) && GTK_IS_WINDOW(widget) && !GTK_IS_OFFSCREEN_WINDOW(widget);
> }
> 
> GtkWidget* gtkWidgetGetToplevelWindow(GtkWidget* widget)
> {
>     GtkWidget* toplevel = gtk_widget_get_toplevel(widget);
>     return gtkWidgetIsToplevelWindow(toplevel) ? toplevel : 0;
> }

Skipped this for now.

> > Source/WebKit/gtk/tests/testwebview.c:382
> > +static void test_webkit_web_view_offscreen_window()
> > +{
> > +    loop = g_main_loop_new(NULL, TRUE);
> > +
> > +    GtkWidget *window = gtk_offscreen_window_new();
> > +    GtkWidget *web_view = webkit_web_view_new();
> > +
> > +    gtk_container_add(GTK_CONTAINER(window), web_view);
> > +    gtk_widget_show_all(window);
> > +    g_signal_connect(web_view, "notify::load-status", G_CALLBACK(idle_quit_loop_cb), NULL);
> > +    webkit_web_view_load_uri(WEBKIT_WEB_VIEW(web_view), base_uri);
> > +
> > +    g_main_loop_run(loop);
> > +
> > +    gtk_widget_destroy(window);
> > +    g_main_loop_unref(loop);
> 
> You should comment here that you are testing that using a WebView inside a GtkOffscreenWindow doesn't crash, because it's not obvious what the test is for.

Instead of a comment we should rename the test. I did that.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:903
> > -    if (gtk_widget_is_toplevel(toplevel) && gtk_window_has_toplevel_focus(GTK_WINDOW(toplevel))) {
> > +    if (gtkWidgetIsValidToplevel(toplevel)) {
> 
> You have removed the has_toplevel_focus check, isn't it necessary anymore?
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:115
> > -    if (gtk_widget_is_toplevel(window) && gtk_widget_get_visible(window)) {
> > +    if (gtkWidgetIsValidToplevel(window)) {
> 
> I'd say the get_visible() check is still needed here.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:227
> > -    if (gtk_widget_is_toplevel(toplevel) && gtk_window_has_toplevel_focus(GTK_WINDOW(toplevel))) {
> > +    if (gtkWidgetIsValidToplevel(toplevel)) {
> 
> Here you removed the has_toplevel_focus check too.

Indeed. A series of dumb mistakes. Thanks for pointing them out. I've fixed them.

[Martin Robinson]

Created attachment 124894 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 124894 [details]
Patch

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

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:784
> +    GtkWidget* toplevel = gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
> +    if (!widgetIsOnscreenToplevelWindow(toplevel))
> +        toplevel = 0;
> +
>      GtkWidget* dialog = gtk_file_chooser_dialog_new(_("Upload File"),
> -                                                    GTK_WINDOW(gtk_widget_get_toplevel(GTK_WIDGET(m_webView))),
> +                                                    GTK_WINDOW(toplevel),

toplevel might be null here, you should use something like:

toplevel ? GTK_WINDOW(toplevel) : 0

> Source/WebKit/gtk/tests/testwebview.c:367
> +static void test_webkit_web_view_in_offscreen_window_does_not_crash()

Much better than a comment :-)

[Martin Robinson]

Created attachment 124994 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 124994 [details]
Patch

LGTM

[Philippe Normand]

Comment on attachment 124994 [details]
Patch

Neat!

[WebKit Review Bot]

Comment on attachment 124994 [details]
Patch

Clearing flags on attachment: 124994

Committed r106559: <http://trac.webkit.org/changeset/106559>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

*** Bug 62090 has been marked as a duplicate of this bug. ***