Bug 195951

Summary: [GTK] REGRESSION(r243094): crash when launching minibrowser
Product: WebKit Reporter: Miguel Gomez <magomez>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cdumez, cgarcia, commit-queue, ews-watchlist, jdiggs, joepeck, pnormand
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 195758    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews112 for mac-highsierra none

Miguel Gomez
Reported 2019-03-19 09:54:47 PDT
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?
Attachments
Patch (1.36 KB, patch)
2019-03-20 03:16 PDT, Carlos Garcia Campos 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews112 for mac-highsierra (2.35 MB, application/zip)
2019-03-20 05:03 PDT, EWS Watchlist 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2019-03-19 10:13:23 PDT
Could you please paste the crash trace?
Chris Dumez
Comment 2 2019-03-19 10:18:31 PDT
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
Comment 3 2019-03-19 11:21:49 PDT
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
Comment 4 2019-03-19 11:23:50 PDT
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
Comment 5 2019-03-19 11:26:31 PDT
(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
Comment 6 2019-03-20 03:12:12 PDT
(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
Comment 7 2019-03-20 03:16:24 PDT
Created attachment 365333 [details] Patch
EWS Watchlist
Comment 8 2019-03-20 05:03:28 PDT
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
Comment 9 2019-03-20 05:03:30 PDT
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
Comment 10 2019-03-20 05:16:09 PDT
(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 11 2019-03-20 12:38:52 PDT
Comment on attachment 365333 [details] Patch r=me
WebKit Commit Bot
Comment 12 2019-03-20 13:18:55 PDT
Comment on attachment 365333 [details] Patch Clearing flags on attachment: 365333 Committed r243231: <https://trac.webkit.org/changeset/243231>
WebKit Commit Bot
Comment 13 2019-03-20 13:18:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.