[GTK] Assertion failure in Inspector::RemoteInspector::setRemoteInspectorClient when disposing WebKitWebContext

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

I came upon this crash when closing Epiphany:

ARGUMENT BAD: client, client
../../Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp(145) : void Inspector::RemoteInspector::setRemoteInspectorClient(Inspector::RemoteInspector::Client*)

Backtrace:

Thread 1 (Thread 0x7fcfcfc85f80 (LWP 3149)):
#0  0x00007fcfbf71197a in WTFCrash ()
    at ../../Source/WTF/wtf/Assertions.cpp:292
No locals.
#1  0x00007fcfbf6fac04 in (anonymous namespace)::RemoteInspector::setRemoteInspectorClient (
    this=0x7fcfc08c3380 <Inspector::RemoteInspector::singleton()::shared>, 
    client=0x0)
    at ../../Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp:145
        __PRETTY_FUNCTION__ = "void Inspector::RemoteInspector::setRemoteInspectorClient(Inspector::RemoteInspector::Client*)"
#2  0x00007fcfc61130da in WebKitAutomationClient::~WebKitAutomationClient (
    this=0xb1b590, __in_chrg=<optimized out>)
    at ../../Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:209
No locals.
#3  0x00007fcfc6113102 in WebKitAutomationClient::~WebKitAutomationClient (
    this=0xb1b590, __in_chrg=<optimized out>)
    at ../../Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:210
No locals.
#4  0x00007fcfc611573b in std::default_delete<WebKitAutomationClient>::operator() (this=0xbf54b0, __ptr=0xb1b590)
    at /usr/include/c++/6.3.1/bits/unique_ptr.h:76
No locals.
#5  0x00007fcfc6113dfd in std::unique_ptr<WebKitAutomationClient, std::default_delete<WebKitAutomationClient> >::~unique_ptr (this=0xbf54b0, 
    __in_chrg=<optimized out>) at /usr/include/c++/6.3.1/bits/unique_ptr.h:239
        __ptr = @0xbf54b0: 0xb1b590
#6  0x00007fcfc611332a in _WebKitWebContextPrivate::~_WebKitWebContextPrivate (
    this=0xbf53c0, __in_chrg=<optimized out>)
    at ../../Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:160
No locals.
#7  0x00007fcfc610e28e in webkit_web_context_finalize (object=0xbf54d0)
    at ../../Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:227
        self = 0xbf54d0
#8  0x00007fcfcf0b151a in g_object_unref (_object=0xbf54d0)
    at /home/mcatanzaro/Projects/GNOME/glib/gobject/gobject.c:3314
        weak_locations = <optimized out>
        old_ref = 1
        object = 0xbf54d0
        object = 0xbf54d0
#9  0x00007fcfcf923fc7 in ephy_embed_shell_dispose (object=0xb331d0)
    at ../../../../Projects/GNOME/epiphany/embed/ephy-embed-shell.c:122
        _pp = 0xb33070
        _p = <optimized out>
#10 0x00007fcfcf0b14a5 in g_object_unref (_object=0xb331d0)
    at /home/mcatanzaro/Projects/GNOME/glib/gobject/gobject.c:3277
        weak_locations = 0x0
        old_ref = <optimized out>
        object = 0xb331d0
        object = 0xb331d0
#11 0x0000000000402847 in main (argc=<optimized out>, argv=<optimized out>)
    at ../../../../Projects/GNOME/epiphany/src/ephy-main.c:436
        option_context = <optimized out>
        option_group = <optimized out>
        error = 0x0
        arbitrary_url = <optimized out>
        ctx = <optimized out>
        startup_flags = <optimized out>
        mode = <optimized out>
        status = 0
        flags = <optimized out>
        desktop_info = <optimized out>

[Michael Catanzaro]

So the problem is pretty simple. In WebKitWebContext.cpp, in the destructor of WebKitAutomationClient:

~WebKitAutomationClient()
    {
        Inspector::RemoteInspector::singleton().setRemoteInspectorClient(nullptr);
    }

This is problematic because (a) RemoteInspector::setRemoteInspectorClient asserts that its argument is nonnull, and (b) it also asserts that it has never been called before.

[Michael Catanzaro]

I'm actually confused why this does not cause Epiphany to crash on start. A new WebKitAutomationClient is constructed whenever constructing a WebKitWebContext. The WebKitAutomationClient constructor calls Inspector::RemoteInspector::singleton().setRemoteInspectorClient(this). Note that's on the singleton instance of the Inspector::RemoteInspector. That happens *every* time a WebKitWebContext is created, so the creation of a second WebKitWebContext will trigger a second call to RemoteInspector::setRemoteInspectorClient, which we have previously established can only be called once (because there is no way to ever unset RemoteInspector::m_client). And Epiphany always creates two WebKitWebContexts: the default WebKitWebContext (created by WebKit) and its own EphyWebContext. I must be missing something important, because no such crash is occurring on startup. I don't understand why.

[Michael Catanzaro]

Oh I get it, Epiphany actually does not create the default web context, because WebKit creates it lazily on-demand, and Epiphany just never uses it. I remember now previously being chided for using the default web context in Epiphany. That's fixed, and that's why it doesn't crash on start.

So I think we broke use of multiple WebKitWebContexts within the same UI process. Do we not have any tests for this, or did we just not notice because our debug bot is always red?

[Carlos Garcia Campos]

Yes, we have a client per WebProcessPool, but the RemoteInspector is a singleton and only allows one client. The asserts are indeed wrong, it's perfectly valid to set nullptr client, and both Cocoa and GTK+ implementations do that when the client is destroyed. One possible solution could be to allow setting more than one client, but I think we shouldn't allow to have automation enabled in more than one WebProcessPool at the same time.

[Carlos Garcia Campos]

Created attachment 309029 [details]
Patch

[Build 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

[Carlos Garcia Campos]

Committed r216243: <http://trac.webkit.org/changeset/216243>