Bug 147453 - [GTK] Crash in WebKit::BackingStore::createBackend running under Wayland
Summary: [GTK] Crash in WebKit::BackingStore::createBackend running under Wayland
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: 81456
  Show dependency treegraph
 
Reported: 2015-07-30 13:19 PDT by Michael Catanzaro
Modified: 2015-09-17 06:01 PDT (History)
9 users (show)

See Also:


Attachments
backtrace (113.78 KB, text/plain)
2015-07-30 13:19 PDT, Michael Catanzaro
no flags Details
Patch (2.57 KB, patch)
2015-09-16 09:04 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.54 KB, patch)
2015-09-16 09:17 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.04 KB, patch)
2015-09-16 20:09 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-30 13:19:28 PDT
Created attachment 257848 [details]
backtrace

My development build of r187587 crashes 100% on start when running in Wayland, in the UI process. Backtrace attached.

In WebKit::BackingStore::createBackend, this call:

gdk_window_create_similar_surface(gtk_widget_get_window(m_webPageProxy.viewWidget())

is returning null.

I'm using GTK+ commit b5a684de6887d233304a009e5f0126dcac4dc36c.
Comment 1 Carlos Garcia Campos 2015-07-31 00:27:29 PDT
I guess gdk_window_create_similar_surface() is returning NULL in wayland
Comment 2 Carlos Garnacho 2015-08-23 10:10:57 PDT
FWIW, that function seems to return NULL because gtk_widget_get_window() returns NULL, a quick check here seems to show that the WebKitWebPage widget doesn't seem to be realized at the moment this is called.

Haven't done much more investigation yet, so it's still unclear to me why this happens.
Comment 3 Carlos Alberto Lopez Perez 2015-09-15 07:46:26 PDT
I can't reproduce this problem on r189805 with GTK+ 3.16.4 (The one we have on the WebKitGTK+ JHBuild).


Maybe this has been caused by a change in GTK+ > 3.16?
Comment 4 Michael Catanzaro 2015-09-15 08:06:57 PDT
(In reply to comment #3)
> I can't reproduce this problem on r189805 with GTK+ 3.16.4 (The one we have
> on the WebKitGTK+ JHBuild).
> 
> 
> Maybe this has been caused by a change in GTK+ > 3.16?

Probably. I can still reproduce the crash 100% with r189754 and a branch of GTK+ from just prior to the 3.17.9 release.
Comment 5 Michael Catanzaro 2015-09-15 08:20:27 PDT
Confirming. I'm posting this with r189754 and GTK+ 3.16.0. Bisecting....
Comment 6 Michael Catanzaro 2015-09-15 08:44:48 PDT
The merge base 00544e9090b51d979a833dbbf2e3e6a9a9f49a55 is bad.
This means the bug has been fixed between 00544e9090b51d979a833dbbf2e3e6a9a9f49a55 and [a816ccd4968f1e221b92bfd1e2b2dc27703d6db5].
Comment 7 Michael Catanzaro 2015-09-15 09:39:37 PDT
What I did wrong was assume 3.16.0 was tagged on the master branch; it's true for previous stable releases but GTK+ branched early last cycle. Trying again....
Comment 8 Michael Catanzaro 2015-09-15 10:38:58 PDT
(In reply to comment #7)
> What I did wrong was assume 3.16.0 was tagged on the master branch; it's
> true for previous stable releases but GTK+ branched early last cycle. Trying
> again....

OK, it's actually not a GTK+ regression at all. Under <some unknown condition I haven't figured out> it crashes always regardless of the GTK+ version. Otherwise it doesn't.
Comment 9 Michael Catanzaro 2015-09-16 03:26:44 PDT
The condition *appears* to be: must have multiple tabs open, and the focused tab when Epiphany was last closed must be the first tab in the notebook. I screwed up my bisect by using the web browser during it, because I incorrectly assumed the crash happens always.

I say "appears" because I wound up discarding many other theories testing this, but so far that one seems to hold up.
Comment 10 Michael Catanzaro 2015-09-16 04:58:37 PDT
The crash also appears to occur if *any* tab in the notebook other than the tab which is first focused uses accelerated compositing. (But if the first tab in the notebook is initially focused, the crash will occur if no tab uses accelerated compositing.)
Comment 11 Michael Catanzaro 2015-09-16 05:03:36 PDT
(In reply to comment #10)
> The crash also appears to occur if *any* tab in the notebook other than the
> tab which is first focused uses accelerated compositing. (But if the first
> tab in the notebook is initially focused, the crash will occur if no tab
> uses accelerated compositing.)

At least, the page renders with the same sort of tearing and artifacts that we normally have on AC pages under X11; I know AC is disabled under Wayland, but something about this codepath must be one of the triggers. I'm able to reproduce the crash reliably when https://github.com/WebKit/webkit/commit/bb68c08356e9eb5755d30dc80bc25be29bcd6f3bis loaded in any tab other than the initially-focused tab.
Comment 12 Michael Catanzaro 2015-09-16 05:07:33 PDT
The link is https://github.com/WebKit/webkit/commit/bb68c08356e9eb5755d30dc80bc25be29bcd6f3b (I missed the space before writing "is") but I imagine any commit diff on GitHub would do.
Comment 13 Carlos Alberto Lopez Perez 2015-09-16 05:32:54 PDT
(In reply to comment #9)
> The condition *appears* to be: must have multiple tabs open, and the focused
> tab when Epiphany was last closed must be the first tab in the notebook. 

I don't use epiphany for testing, but the MiniBrowser. 

Is this something that also happens with the MiniBrowser or only with Epiphany?
Comment 14 Michael Catanzaro 2015-09-16 09:04:57 PDT
Created attachment 261309 [details]
Patch
Comment 15 WebKit Commit Bot 2015-09-16 09:07:33 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 16 Michael Catanzaro 2015-09-16 09:11:32 PDT
(In reply to comment #13)
> I don't use epiphany for testing, but the MiniBrowser. 
> 
> Is this something that also happens with the MiniBrowser or only with
> Epiphany?

I could only reproduce it with Epiphany, never with MiniBrowser.

Anyway, the problem was just as Carlos Garnacho said in comment #2. The crash didn't affect X11 since there is a separate codepath for that which doesn't require the WebView to be realized.
Comment 17 Michael Catanzaro 2015-09-16 09:17:02 PDT
Created attachment 261312 [details]
Patch

Just fixing a bug in the comment...
Comment 18 Carlos Garcia Campos 2015-09-16 09:37:02 PDT
Comment on attachment 261312 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:64
> +    // This function will be called during the construction of the WebKitWebView, before it is
> +    // realized. But, except when running under X11, the WebKitWebView must be realized before the

This is also called after a web process crash when the page proxy is reattached to a web process, and the web view is already realized.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:66
> +    // DrawingAreaProxy creates a BackingStore. We can't realize it until after it is constructed,
> +    // so schedule it to happen instead. This code relies on the fact that the RunLoop uses a queue:

I'm not sure I understand this. If wayland needs to the view to be realized why not call the realize in BackingStore::createBackend() without having to schedule it in the main loop?

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:71
> +        gtk_widget_realize(viewWidget.get());

We should check if the view is realized already or not.
Comment 19 Carlos Garcia Campos 2015-09-16 09:42:28 PDT
Also, maybe we should do that only for wayland, not need to rush the realize in X11, no?
Comment 20 Michael Catanzaro 2015-09-16 20:03:31 PDT
Thanks for the review, as always!

(In reply to comment #18)
> This is also called after a web process crash when the page proxy is
> reattached to a web process, and the web view is already realized.

OK, I will improve the comment.

> I'm not sure I understand this. If wayland needs to the view to be realized
> why not call the realize in BackingStore::createBackend() without having to
> schedule it in the main loop?

Um... I was just dumb. :p That is obviously much better....

> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:71
> > +        gtk_widget_realize(viewWidget.get());
> 
> We should check if the view is realized already or not.

It's a no-op if it has already been realized.

(In reply to comment #19)
> Also, maybe we should do that only for wayland, not need to rush the realize
> in X11, no?

I am moving it to the non-X11 codepath (since BackingStore::createBackend has a separate path for X11). Although there is some limited value in realizing early on both platforms: to make it somewhat less likely that a realization-related bug would occur on one platform but not the other in the future.
Comment 21 Michael Catanzaro 2015-09-16 20:09:19 PDT
Created attachment 261357 [details]
Patch
Comment 22 Carlos Garcia Campos 2015-09-16 22:47:35 PDT
Comment on attachment 261357 [details]
Patch

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

Ok, let's try this way then. Thanks!

> Source/WebKit2/UIProcess/cairo/BackingStoreCairo.cpp:68
> -    RefPtr<cairo_surface_t> surface = adoptRef(gdk_window_create_similar_surface(gtk_widget_get_window(m_webPageProxy.viewWidget()),
> +    GtkWidget* viewWidget = m_webPageProxy.viewWidget();
> +    gtk_widget_realize(viewWidget);
> +    RefPtr<cairo_surface_t> surface = adoptRef(gdk_window_create_similar_surface(gtk_widget_get_window(viewWidget),

Yes, much simpler :-)
Comment 23 WebKit Commit Bot 2015-09-17 06:01:04 PDT
Comment on attachment 261357 [details]
Patch

Clearing flags on attachment: 261357

Committed r189912: <http://trac.webkit.org/changeset/189912>
Comment 24 WebKit Commit Bot 2015-09-17 06:01:09 PDT
All reviewed patches have been landed.  Closing bug.