Bug 195951 – [GTK] REGRESSION(r243094): crash when launching minibrowser

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Miguel Gomez]

Since r243094 we're getting a crash when launching the minibrowser on the gtk port. The crash happens because the minibrowser is requesting the view's webInspector before loading any page there. This was valid before r243094 but now webkit_web_view_get_inspector() is returning null because the associated WebPageProxy doesn't have a running process (and WebPageProxy::inspector() returns null), even when the inspector is already created.

I'm not sure whether this is expected behavior and we should instruct the clients not to request the inspector until something has been loaded (and fix this by changing the minibrowser), or whether we could return the inspector even if the WebPageProxy doesn't have a process yet.

WDYT?

[Chris Dumez]

Could you please paste the crash trace?

[Chris Dumez]

3 ways we could fix this:
1. Update WebPageProxy::inspector() to return m_inspector even if !hasRunningProcess()
2. Update call site to not call inspector() until a load has been started (or null check as needed)
3. Add a call to launchInitialProcessIfNecessary() at the beginning of WebPageProxy::inspector() to maintain backward compatibility.

A Web inspector person like Joe would likely know which way is best. Otherwise, I'd go with 3 because it is the safest. That said, it means that the client would not benefit in delayed WebProcess launch when making such a call (meaning that it would not be able to use the WebProcess cache).

[Philippe Normand]

All I have currently is a release build:

(gdb) bt
#0  0x00007ff4dadc0d2a in _ZN6WebKit17WebInspectorProxy9setClientEOSt10unique_ptrINS_23WebInspectorProxyClientESt14default_deleteIS2_EE () from /home/phil/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#1  0x00007ff4dad435c4 in _Z24webkitWebInspectorCreatePN6WebKit17WebInspectorProxyE () from /home/phil/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#2  0x00007ff4dad2f008 in webkit_web_view_get_inspector () from /home/phil/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#3  0x0000555e807cec02 in browserTabConstructed ()
#4  0x00007ff4d84ec4ab in g_object_new_internal () at ../../Source/glib-2.58.1/gobject/gobject.c:1845
#5  0x00007ff4d84ee004 in g_object_new_valist () at ../../Source/glib-2.58.1/gobject/gobject.c:2128
#6  0x00007ff4d84ee32c in g_object_new () at ../../Source/glib-2.58.1/gobject/gobject.c:1648
#7  0x0000555e807ced7d in browser_tab_new ()
#8  0x0000555e807d311f in browser_window_append_view ()
#9  0x0000555e807c9d23 in main ()

[Philippe Normand]

With c++-filt:

(gdb) bt
#0  0x00007ff4dadc0d2a in WebKit::WebInspectorProxy::setClient(std::unique_ptr<WebKit::WebInspectorProxyClient, std::default_delete<WebKit::WebInspectorProxyClient> >&&) () from /home/phil/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#1  0x00007ff4dad435c4 in webkitWebInspectorCreate(WebKit::WebInspectorProxy*) () from /home/phil/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#2  0x00007ff4dad2f008 in webkit_web_view_get_inspector () from /home/phil/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#3  0x0000555e807cec02 in browserTabConstructed ()
#4  0x00007ff4d84ec4ab in g_object_new_internal () at ../../Source/glib-2.58.1/gobject/gobject.c:1845
#5  0x00007ff4d84ee004 in g_object_new_valist () at ../../Source/glib-2.58.1/gobject/gobject.c:2128
#6  0x00007ff4d84ee32c in g_object_new () at ../../Source/glib-2.58.1/gobject/gobject.c:1648
#7  0x0000555e807ced7d in browser_tab_new ()
#8  0x0000555e807d311f in browser_window_append_view ()
#9  0x0000555e807c9d23 in main ()

[Chris Dumez]

(In reply to Philippe Normand from comment #4)
> With c++-filt:
> 
> (gdb) bt
> #0  0x00007ff4dadc0d2a in
> WebKit::WebInspectorProxy::setClient(std::unique_ptr<WebKit::
> WebInspectorProxyClient,
> std::default_delete<WebKit::WebInspectorProxyClient> >&&) () from
> /home/phil/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #1  0x00007ff4dad435c4 in
> webkitWebInspectorCreate(WebKit::WebInspectorProxy*) () from
> /home/phil/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #2  0x00007ff4dad2f008 in webkit_web_view_get_inspector () from
> /home/phil/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #3  0x0000555e807cec02 in browserTabConstructed ()
> #4  0x00007ff4d84ec4ab in g_object_new_internal () at
> ../../Source/glib-2.58.1/gobject/gobject.c:1845
> #5  0x00007ff4d84ee004 in g_object_new_valist () at
> ../../Source/glib-2.58.1/gobject/gobject.c:2128
> #6  0x00007ff4d84ee32c in g_object_new () at
> ../../Source/glib-2.58.1/gobject/gobject.c:1648
> #7  0x0000555e807ced7d in browser_tab_new ()
> #8  0x0000555e807d311f in browser_window_append_view ()
> #9  0x0000555e807c9d23 in main ()

Any good reason you need to call webkit_web_view_get_inspector() this early? Is this something that could happen layer (e.g. lazily)?

[Carlos Garcia Campos]

(In reply to Chris Dumez from comment #2)
> 3 ways we could fix this:
> 1. Update WebPageProxy::inspector() to return m_inspector even if
> !hasRunningProcess()
> 2. Update call site to not call inspector() until a load has been started
> (or null check as needed)
> 3. Add a call to launchInitialProcessIfNecessary() at the beginning of
> WebPageProxy::inspector() to maintain backward compatibility.
> 
> A Web inspector person like Joe would likely know which way is best.
> Otherwise, I'd go with 3 because it is the safest. That said, it means that
> the client would not benefit in delayed WebProcess launch when making such a
> call (meaning that it would not be able to use the WebProcess cache).

I think 1 is safe enough. The WebInspectorProxy is created in WebPageProxy constructor, so it will always be available, and it's safe to use even before a process has been launched because m_inspectedPage is null-checked everywhere.

[Carlos Garcia Campos]

Created attachment 365333 [details]
Patch

[EWS Watchlist]

Comment on attachment 365333 [details]
Patch

Attachment 365333 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11578655

New failing tests:
http/tests/security/cross-origin-indexeddb.html

[EWS Watchlist]

Created attachment 365337 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Carlos Garcia Campos]

(In reply to Build Bot from comment #8)
> Comment on attachment 365333 [details]
> Patch
> 
> Attachment 365333 [details] did not pass mac-debug-ews (mac):
> Output: https://webkit-queues.webkit.org/results/11578655
> 
> New failing tests:
> http/tests/security/cross-origin-indexeddb.html

This is unrelated.

[Joseph Pecoraro]

Comment on attachment 365333 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 365333 [details]
Patch

Clearing flags on attachment: 365333

Committed r243231: <https://trac.webkit.org/changeset/243231>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.