Bug 147453 – [GTK] Crash in WebKit::BackingStore::createBackend running under Wayland

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

I guess gdk_window_create_similar_surface() is returning NULL in wayland

[Carlos Garnacho]

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.

[Carlos Alberto Lopez Perez]

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?

[Michael Catanzaro]

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

[Michael Catanzaro]

Confirming. I'm posting this with r189754 and GTK+ 3.16.0. Bisecting....

[Michael Catanzaro]

The merge base 00544e9090b51d979a833dbbf2e3e6a9a9f49a55 is bad.
This means the bug has been fixed between 00544e9090b51d979a833dbbf2e3e6a9a9f49a55 and [a816ccd4968f1e221b92bfd1e2b2dc27703d6db5].

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Carlos Alberto Lopez Perez]

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

[Michael Catanzaro]

Created attachment 261309 [details]
Patch

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

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 261312 [details]
Patch

Just fixing a bug in the comment...

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Also, maybe we should do that only for wayland, not need to rush the realize in X11, no?

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 261357 [details]
Patch

[Carlos Garcia Campos]

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 :-)

[WebKit Commit Bot]

Comment on attachment 261357 [details]
Patch

Clearing flags on attachment: 261357

Committed r189912: <http://trac.webkit.org/changeset/189912>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.