Bug 166680 – [GTK] Switch to use ENABLE_REMOTE_INSPECTOR instead of ENABLE_INSPECTOR_SERVER for the remote inspector

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We currently use a web inspector server based on web sockets, that is quite convenient because you can use any web browser (unless the browser doesn't support any JS used by our inspector). However, the ENABLE_REMOTE_INSPECTOR implementation has some other advantages. It allows to debug JavaScript applications, not only websites, and it also has support for web automation session, used to implement WebDriver.

[Carlos Garcia Campos]

Created attachment 303320 [details]
Patch

This doesn't use web sockets anymore, the implementation is completely different, but the way to use it is quite similar. To enable remote inspection you do exactly the same:

$ WEBKIT_INSPECTOR_SERVER=ip:port webkit-program

Exactly like we did with the web sockets implementation. But now, to remotely inspect you doesn't visit http://ip:port, but inspector://ip:port. That shows the list of debuggable targets, with a button to inspect them. A new window is always created now for every target instead of reusing the same browser view. For now it's required that both local and remote have the same version of the backend commands.

[WebKit Commit Bot]

Attachment 303320 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/gtk/RemoteInspectorClient.cpp:146:  Missing spaces around :  [whitespace/init] [4]
ERROR: Source/WTF/wtf/glib/GRefPtr.h:255:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/PlatformGTK.cmake:328:  Alphabetical sorting problem. "UIProcess/gtk/RemoteInspectorClient.cpp" should be before "UIProcess/gtk/RemoteWebInspectorProxyGtk.cpp".  [list/order] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitRemoteInspectorProtocolHandler.h"
Total errors found: 3 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 303323 [details]
Fix CMake "coding" style

[WebKit Commit Bot]

Attachment 303323 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/gtk/RemoteInspectorClient.cpp:146:  Missing spaces around :  [whitespace/init] [4]
ERROR: Source/WTF/wtf/glib/GRefPtr.h:255:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitRemoteInspectorProtocolHandler.h"
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 307373 [details]
Rebased patch

I've also renamed some files as Glib instead of Gtk, since they don't use GTK+ and could be used by other glib based ports.

[Build Bot]

Attachment 307373 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/glib/GRefPtr.h:255:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/glib/RemoteInspectorClient.cpp:155:  Missing spaces around :  [whitespace/init] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitRemoteInspectorProtocolHandler.h"
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Olivier Blin]

(In reply to Carlos Garcia Campos from comment #5)
> I've also renamed some files as Glib instead of Gtk, since they don't use
> GTK+ and could be used by other glib based ports.

For WPE, it could make sense to keep using ENABLE_INSPECTOR_SERVER.
Otherwise, inspecting an embedded device target would require a matching WebKit port with inspector:// on the host device. This can be a burden for HTML application developers.

WebAutomation APIs can be exposed with ENABLE_INSPECTOR_SERVER as well.

[Carlos Garcia Campos]

The inspector:// implementation is GTK+ specific, other ports can add their own frontend part while still re-using the remote inspector implementation. In any case, this patch is not removing the inspector server code, only the GTK+ implementation.

[Build Bot]

Comment on attachment 307373 [details]
Rebased patch

Attachment 307373 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3557255

New failing tests:
http/tests/inspector/network/resource-sizes-network.html

[Build Bot]

Created attachment 307382 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Michael Catanzaro]

In the interests of not losing access to this, here is Carlos's summary explanation (which ought to be added to the ChangeLog):

"""How does the new remote inspector work?

The web browser checks the presence of WEBKIT_INSPECTOR_SERVER environment variable at start up, the same way it was done with the WebSockets. If present, the RemoteInspectorServer is started in the UI process running a DBus service listening in the IP and port provided. The environment variable is propagated to the child web processes, that create a RemoteInspector object and connect to the RemoteInspectorServer. There’s one RemoteInspector per web process, and one debuggable target per WebView. Every RemoteInspector maintains a list of debuggable targets that is sent to the RemoteInspector server when a new target is added, removed or modified, or when explicitly requested by the RemoteInspectorServer.
When the debugger browser loads an inspector:// URL, a RemoteInspectorClient is created. The RemoteInspectorClient connects to the RemoteInspectorServer using the IP and port of the inspector:// URL and asks for the list of targets that is used by the custom protocol handler to create the web page. The RemoteInspectorServer works as a router, forwarding messages between RemoteInspector and RemoteInspectorClient objects."""

[Michael Catanzaro]

Comment on attachment 307373 [details]
Rebased patch

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

Biggest concern here is how you ignore D-Bus errors all over the place. That's going to make this really, really hard to debug if it ever breaks.

> Source/JavaScriptCore/inspector/remote/glib/RemoteConnectionToTargetGlib.cpp:46
> +bool RemoteConnectionToTarget::setup(bool isAutomaticInspection, bool automaticallyPause)

It would be great to clean this up to take enums instead of bools, of course in a separate patch to avoid unneeded changes to the Mac implementation in this patch.

> Source/JavaScriptCore/inspector/remote/glib/RemoteConnectionToTargetGlib.cpp:58
> +        auto castedTarget = downcast<RemoteInspectionTarget>(m_target);

Personally, I would have named this variable "target"

> Source/JavaScriptCore/inspector/remote/glib/RemoteConnectionToTargetGlib.cpp:67
> +        auto castedTarget = downcast<RemoteAutomationTarget>(m_target);

Ditto

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:72
> +    ASSERT(portPtr);
> +    *portPtr = '\0';

Not OK to use ASSERT to validate external input. Do real input validation that runs in release builds as well, otherwise it's *our* fault when this crashes due to a malformed WEBKIT_INSPECTOR_SERVER variable. Print a real error message when there's no colon.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:80
> +            GRefPtr<GDBusConnection> connection = adoptGRef(g_dbus_connection_new_for_address_finish(result, nullptr));
> +            inspector->setupConnection(WTFMove(connection));

Pass a GError* to g_dbus_connection_new_for_address_finish() and print the error message when it fails.

Please explain how you're sure it's safe that RemoteInspector::start completes asynchronously with no completion callback. How does the caller know when it is safe to call other methods of RemoteInspector? You're sure there are no race conditions here?

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:166
> +    g_dbus_connection_register_object(m_dbusConnection.get(), REMOTE_INSPECTOR_DBUS_OBJECT_PATH,
> +        introspectionData->interfaces[0], &s_interfaceVTable, this, nullptr, nullptr);

No error checking? You should pass a callback to at least check and print if there is an error. Let's not silently hide errors.

Also, please explain how you know it's safe that the inspector D-Bus object is registered asynchronously after this function completes. It seems likely that there are race conditions here, so you ought to prove there aren't any and add a comment so that future developers don't get confused.

On that note: maybe you should set m_dbusConnection in the completion callback of g_dbus_connection_register_object()?

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:192
> +    ASSERT(m_dbusConnection);
> +    if (!m_dbusConnection)
> +        return;

Please no, after an ASSERT you should assume the statement is always valid. Don't second-guess it here. I know it doesn't run in release builds, but if you feel the need for the extra check then you need to remove the ASSERT. I think it's better to keep just the ASSERT here.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:206
> +    g_dbus_connection_call(m_dbusConnection.get(), nullptr,
> +        INSPECTOR_DBUS_OBJECT_PATH, INSPECTOR_DBUS_INTERFACE, "SetTargetList",
> +        g_variant_builder_end(&builder), nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START,
> +        -1, nullptr, nullptr, nullptr);

"No error checking? You should pass a callback to at least check and print if there is an error. Let's not silently hide errors."

Also, this call will fail if the inspector D-Bus object registration has not yet completed. m_dbusConnection can already be valid even in that case. You could avoid this by setting m_dbusConnection in the completion callback of g_dbus_connection_register_object(). Would that be a good idea?

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:220
> +        std::lock_guard<Lock> lock(m_mutex);

I'm not really comfortable with your use of m_mutex. In the case of RemoteConnectionToTarget it was clear that you needed to lock m_targetMutex and it looked like you always handled that as needed. But in the case of RemoteInspector it seems like the requirements are stricter:

    // Targets can be registered from any thread at any time.
    // Any target can send messages over the XPC connection.
    // So lock access to all maps and state as they can change
    // from any thread.
    Lock m_mutex;

But in this file you modify tons of state (member variables) without locking this mutex. It's not really clear what the correct use of this mutex is. Please explain this. Probably the comment in RemoteInspector.h needs to be adjusted, as it's not clear to me.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:228
> +    ASSERT_ARG(target, target);

Umm it's correct, but it's not the usual style, everywhere else you just use ASSERT. Why not write ASSERT(target)?

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:254
> +        // FIXME: We should handle multiple debuggables trying to pause at the same time on different threads.
> +        // To make this work we will need to change m_automaticInspectionCandidateTargetIdentifier to be a per-thread value.
> +        // Multiple attempts on the same thread should not be possible because our nested run loop is in a special RWI mode.

Why is it not important to fix this before landing?

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:256
> +            LOG_ERROR("Skipping Automatic Inspection Candidate with pageId(%u) because we are already paused waiting for pageId(%u)", targetIdentifier, m_automaticInspectionCandidateTargetIdentifier);

I don't like the format you chose for printing this, with the parentheses and no space. I would write:

"Skipping Automatic Inspection Candidate with pageId %u because we are already paused waiting for pageId %u"

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:275
> +                LOG_ERROR("Skipping Automatic Inspection Candidate with pageId(%u) because we failed to receive a response in time.", m_automaticInspectionCandidateTargetIdentifier);

pageId %u

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:298
> +    // FIXME: Implement automatic inspection.

What's automatic inspection? This is different from automated inspection?

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:311
> +        -1, nullptr, nullptr, nullptr);

Again, please add error checking.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:60
> +    "    <method name='GetTargetList'/>"

This is weird, GetTargetList sounds like a getter function but it returns nothing. Maybe rename to AcquireTargetList.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:155
> +    GUniquePtr<char> dbusAddress(g_strdup_printf("tcp:host=%s,port=%u", address, port));

Are you familiar with the pitfalls of using D-Bus over TCP? I'm not, but I am aware that this is discouraged. We should look up best-practices and run the general idea past Alex to make sure we're not missing something problematic before committing this.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:177
> +void RemoteInspectorServer::connectionClosedCallback(GDBusConnection* connection, gboolean /*remotePeerVanished*/, GError*, RemoteInspectorServer* server)

Don't ignore errors.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:188
> +    g_dbus_connection_register_object(connection, INSPECTOR_DBUS_OBJECT_PATH, interfaceInfo(), &s_interfaceVTable, this, nullptr, nullptr);

Don't ignore errors.

Just to be sure, please add a comment that confirms there are no problems with this completing asynchronously.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:212
> +        nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, nullptr, nullptr, nullptr);

Don't ignore errors.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:234
> +            nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, nullptr, nullptr, nullptr);

Don't ignore errors.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:255
> +        nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, nullptr, nullptr, nullptr);

Don't ignore errors.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:274
> +        nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, nullptr, nullptr, nullptr);

Don't ignore errors.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:309
> +                nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, nullptr, nullptr, nullptr);

Don't ignore errors.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:331
> +        nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, nullptr, nullptr, nullptr);

Don't ignore errors.

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:350
> +        nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, nullptr, nullptr, nullptr);

Don't ignore errors.

> Source/WebKit2/UIProcess/API/gtk/WebKitRemoteInspectorProtocolHandler.cpp:145
> +                    "<td class=\"input\"><input type=\"button\" value=\"Inspect\" onclick=\"window.webkit.messageHandlers.inspector.postMessage('%" G_GUINT64_FORMAT ":%" G_GUINT64_FORMAT "');\"></td>"

I bet eventually somebody is going to be confused and disappointed that this breaks when JavaScript is disabled, but I know we don't have any decent solution for that now....

> Source/WebKit2/UIProcess/API/gtk/WebKitRemoteInspectorProtocolHandler.h:37
> +    RemoteInspectorProtocolHandler(WebKitWebContext* context);

explicit

> Source/WebKit2/UIProcess/API/gtk/WebKitRemoteInspectorProtocolHandler.h:40
> +    void inspect(const String& hostAndPort, uint64_t connectionID, uint64_t tatgetID);

targetID

> Source/WebKit2/UIProcess/glib/RemoteInspectorClient.cpp:162
> +            GRefPtr<GDBusConnection> connection = adoptGRef(g_dbus_connection_new_for_address_finish(result, nullptr));

Don't ignore errors.

> Source/WebKit2/UIProcess/glib/RemoteInspectorClient.cpp:190
> +    g_dbus_connection_register_object(m_dbusConnection.get(), REMOTE_INSPECTOR_CLIENT_OBJECT_PATH, introspectionData->interfaces[0], &s_interfaceVTable, this, nullptr, nullptr);

Don't ignore errors.

> Source/WebKit2/UIProcess/glib/RemoteInspectorClient.cpp:198
> +}

Don't ignore errors.

> Source/WebKit2/UIProcess/glib/RemoteInspectorClient.cpp:223
> +        nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, nullptr, nullptr, nullptr);

Don't ignore errors.

> Source/WebKit2/UIProcess/glib/RemoteInspectorClient.cpp:235
> +        nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, nullptr, nullptr, nullptr);

Don't ignore errors.

> Source/WebKit2/UIProcess/glib/RemoteInspectorClient.cpp:246
> +        nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START, -1, nullptr, nullptr, nullptr);

Don't ignore errors.

> Source/WebKit2/UIProcess/gtk/RemoteWebInspectorProxyGtk.cpp:69
> +#if ENABLE(DEVELOPER_MODE)
> +    // Allow developers to inspect the Web Inspector in debug builds without changing settings.
> +    preferences->setDeveloperExtrasEnabled(true);
> +    preferences->setLogsPageMessagesToSystemConsoleEnabled(true);
> +#endif

Maybe we should change these defaults project-wide instead of just for the remote inspector window?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/CMakeLists.txt:-98
> -ADD_WK2_TEST(InspectorTestServer InspectorTestServer.cpp)

Why can't you add a new test?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #12) 
> > Source/WebKit2/UIProcess/glib/RemoteInspectorClient.cpp:198
> > +}
> 
> Don't ignore errors.

(My context is off by one line here.)

[Joseph Pecoraro]

Comment on attachment 307373 [details]
Rebased patch

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

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:250
> +        // Don't allow automatic inspection unless it is allowed or we are stopped.
> +        if (!m_automaticInspectionEnabled || !m_enabled) {
> +            pushListingsSoon();
> +            return;
> +        }

Given currently m_automaticInspectionEnabled will always be false this path will always be taken and the rest of this method is unreachable code.

You might just want a comment here about Implementing automatic inspection (like the comment below).

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:254
>> +        // Multiple attempts on the same thread should not be possible because our nested run loop is in a special RWI mode.
> 
> Why is it not important to fix this before landing?

This matches the Cocoa port. It hasn't been a common scenario for people using the JavaScriptCore APIs so its not received much priority for fixing. And again this is only for the automatic inspection case.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:298
>> +    // FIXME: Implement automatic inspection.
> 
> What's automatic inspection? This is different from automated inspection?

Automatic inspection (as implementing in the Cocoa version of RemoteInspector) is the ability for a debugger to immediately be offered to attach to a debuggable target as soon as the target is registered.

An example using the JavaScriptCore ObjC API. An application may have code like the following:

    [[JSContext alloc] init] evaluateScript:@"var a = 1; var b = 2; oopsReferenceError; var c = a + b;"];

Safari can still have opened a remote inspector for this JSContext and step through that initial evaluation.

This is done by:

  • Remote debuggers indicating with some global system state that there is an automatic inspection client
  • Here in updateAutomaticInspectionCandidate if that global system state is true => go through the automatic inspection candidate process
    - during in this process we pause the application and ask remote debuggers (in order) if they want to debug this or not
    - we also have a timeout (the dispatch after above) to unpause the app if the remote debuggers did not respond.

On Mac we only use this for automatically debugging JSContexts to handle the situation shown above. And not uncommon for a JSContext to be created for a single evaluation and never used again (like the above example), so we needed to have a way to handle that debugging scenario. I believe on GTK this is being used more for Web Driver, so automatic inspection is not necessary. It may be a future feature of interest.

[Carlos Garcia Campos]

Comment on attachment 307373 [details]
Rebased patch

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

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:72
>> +    *portPtr = '\0';
> 
> Not OK to use ASSERT to validate external input. Do real input validation that runs in release builds as well, otherwise it's *our* fault when this crashes due to a malformed WEBKIT_INSPECTOR_SERVER variable. Print a real error message when there's no colon.

This is in the WebProcess, that should only receive a valid WEBKIT_INSPECTOR_SERVER. The Ui process does all the validation, and it doesn't propagte it to the child processes if it's not correct. So, in the web process we always assume a valid one.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:80
>> +            inspector->setupConnection(WTFMove(connection));
> 
> Pass a GError* to g_dbus_connection_new_for_address_finish() and print the error message when it fails.
> 
> Please explain how you're sure it's safe that RemoteInspector::start completes asynchronously with no completion callback. How does the caller know when it is safe to call other methods of RemoteInspector? You're sure there are no race conditions here?

We are the caller. Remoteinspector connects to RemoteInspectorServer that it has been setup synchronously in the UI process. And the RemoteInspectorClient also connects to the server. The server works as a router forwarding the messages between clients and inspectors. So, in this case, it's the remote inspector the one that once it has connected to the server pushes its listings calling SetTargetList. When the server receives a SetTargetList message from a client that didn't know before, it adds it to the list of remote inspectors. When a client connects to the server, it sends GetTargetList message, similar to what the remote inspector does with the SetTargetList. In this case, the client is a different browser, so it can happen that it fails to connect or the dbus is not yet ready, in that case the inspector page will just say there aren't targets. The user can then reload the page.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:166
>> +        introspectionData->interfaces[0], &s_interfaceVTable, this, nullptr, nullptr);
> 
> No error checking? You should pass a callback to at least check and print if there is an error. Let's not silently hide errors.
> 
> Also, please explain how you know it's safe that the inspector D-Bus object is registered asynchronously after this function completes. It seems likely that there are race conditions here, so you ought to prove there aren't any and add a comment so that future developers don't get confused.
> 
> On that note: maybe you should set m_dbusConnection in the completion callback of g_dbus_connection_register_object()?

g_dbus_connection_register_object() is not async, the callback is a GDestroyNotify for the vtable user data. We don't use it because the user data is the RemoteInspector object that is a singleton.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:192
>> +        return;
> 
> Please no, after an ASSERT you should assume the statement is always valid. Don't second-guess it here. I know it doesn't run in release builds, but if you feel the need for the extra check then you need to remove the ASSERT. I think it's better to keep just the ASSERT here.

I'll remove the assert

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:206
>> +        -1, nullptr, nullptr, nullptr);
> 
> "No error checking? You should pass a callback to at least check and print if there is an error. Let's not silently hide errors."
> 
> Also, this call will fail if the inspector D-Bus object registration has not yet completed. m_dbusConnection can already be valid even in that case. You could avoid this by setting m_dbusConnection in the completion callback of g_dbus_connection_register_object(). Would that be a good idea?

g_dbus_connection_register_object() is sync.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:220
>> +        std::lock_guard<Lock> lock(m_mutex);
> 
> I'm not really comfortable with your use of m_mutex. In the case of RemoteConnectionToTarget it was clear that you needed to lock m_targetMutex and it looked like you always handled that as needed. But in the case of RemoteInspector it seems like the requirements are stricter:
> 
>     // Targets can be registered from any thread at any time.
>     // Any target can send messages over the XPC connection.
>     // So lock access to all maps and state as they can change
>     // from any thread.
>     Lock m_mutex;
> 
> But in this file you modify tons of state (member variables) without locking this mutex. It's not really clear what the correct use of this mutex is. Please explain this. Probably the comment in RemoteInspector.h needs to be adjusted, as it's not clear to me.

Right, I need to take the mutex in several methods. I'll review it.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:228
>> +    ASSERT_ARG(target, target);
> 
> Umm it's correct, but it's not the usual style, everywhere else you just use ASSERT. Why not write ASSERT(target)?

I copied this from original RemoteInspector.mm and for got to remove it. We don't support automatic inspector yet, so we can probably remove this. If if eventually support it we can try to refactor this code to share it with cocoa implementation.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:60
>> +    "    <method name='GetTargetList'/>"
> 
> This is weird, GetTargetList sounds like a getter function but it returns nothing. Maybe rename to AcquireTargetList.

It's because it's kind of "async" way. The response of a GetTargetList message is a SetTargetList message.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:155
>> +    GUniquePtr<char> dbusAddress(g_strdup_printf("tcp:host=%s,port=%u", address, port));
> 
> Are you familiar with the pitfalls of using D-Bus over TCP? I'm not, but I am aware that this is discouraged. We should look up best-practices and run the general idea past Alex to make sure we're not missing something problematic before committing this.

The point of the remote inspector is to debug something in a different host, so a remote inspector with local sockets is pointless. This is supported by GLib, and nothing says in the docs or the code that it shouldn't be used, so I assume it works. I've tested it extensively on localhost both with the remote inspector itself and with the web driver.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:188
>> +    g_dbus_connection_register_object(connection, INSPECTOR_DBUS_OBJECT_PATH, interfaceInfo(), &s_interfaceVTable, this, nullptr, nullptr);
> 
> Don't ignore errors.
> 
> Just to be sure, please add a comment that confirms there are no problems with this completing asynchronously.

g_dbus_connection_register_object() is sync. Registering an object is just updating several hash maps.

>> Source/WebKit2/UIProcess/API/gtk/WebKitRemoteInspectorProtocolHandler.cpp:145
>> +                    "<td class=\"input\"><input type=\"button\" value=\"Inspect\" onclick=\"window.webkit.messageHandlers.inspector.postMessage('%" G_GUINT64_FORMAT ":%" G_GUINT64_FORMAT "');\"></td>"
> 
> I bet eventually somebody is going to be confused and disappointed that this breaks when JavaScript is disabled, but I know we don't have any decent solution for that now....

The web inspector is written in js, if js is disabled, the inspector will not work either.

>> Source/WebKit2/UIProcess/glib/RemoteInspectorClient.cpp:190
>> +    g_dbus_connection_register_object(m_dbusConnection.get(), REMOTE_INSPECTOR_CLIENT_OBJECT_PATH, introspectionData->interfaces[0], &s_interfaceVTable, this, nullptr, nullptr);
> 
> Don't ignore errors.

g_dbus_connection_register_object() is sync and never fails.

>> Source/WebKit2/UIProcess/gtk/RemoteWebInspectorProxyGtk.cpp:69
>> +#endif
> 
> Maybe we should change these defaults project-wide instead of just for the remote inspector window?

The WebInspectorProxy does the same. We could refactor to share more code like this, but this patch is already long enough.

>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/CMakeLists.txt:-98
>> -ADD_WK2_TEST(InspectorTestServer InspectorTestServer.cpp)
> 
> Why can't you add a new test?

Current test was based on pageList.json that no longer exists. I guess I could launch a server and load the inspector page and parse the generated html or something like that.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #15)
> Current test was based on pageList.json that no longer exists. I guess I
> could launch a server and load the inspector page and parse the generated
> html or something like that.

I know it seems fragile, but I think it would be valuable to detect when (when, not if :) this breaks.

So yeah, please study how to use that mutex safely, and also add error checking for the stuff that's not register_object (whoops).

[Carlos Garcia Campos]

Created attachment 307969 [details]
Updated patch

[Build Bot]

Attachment 307969 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/glib/GRefPtr.h:255:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/glib/RemoteInspectorClient.cpp:155:  Missing spaces around :  [whitespace/init] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitRemoteInspectorProtocolHandler.h"
Total errors found: 2 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 307969 [details]
Updated patch

Nice test.

[Michael Catanzaro]

Comment on attachment 307969 [details]
Updated patch

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

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:89
> +void RemoteInspector::stopInternal(StopSource)
> +{

I assume the mutex is already taken when this is called, right?

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:201
> +void RemoteInspector::pushListingsNow()
> +{

Do you need to lock the mutex here? If not, why not?

> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:343
> +void RemoteInspector::requestAutomationSession(const char* sessionID)
> +{

Do you need to lock the mutex here? If not, why not?

[Build Bot]

Comment on attachment 307969 [details]
Updated patch

Attachment 307969 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3594511

New failing tests:
fast/workers/worker-exception-during-navigation.html

[Build Bot]

Created attachment 307976 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Carlos Garcia Campos]

Comment on attachment 307969 [details]
Updated patch

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

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:89
>> +{
> 
> I assume the mutex is already taken when this is called, right?

Right

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:201
>> +{
> 
> Do you need to lock the mutex here? If not, why not?

It's always called with the mutex locked.

>> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp:343
>> +{
> 
> Do you need to lock the mutex here? If not, why not?

This is tricky. In case of web automation, there's a RemoteInspector in the UI process. The mutex is only needed in the web process, in the UI process the remote inspector is only used from the main thread.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #23)
> This is tricky. In case of web automation, there's a RemoteInspector in the
> UI process. The mutex is only needed in the web process, in the UI process
> the remote inspector is only used from the main thread.

Please add a comment.

[Carlos Garcia Campos]

Committed r215683: <http://trac.webkit.org/changeset/215683>

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #25)
> Committed r215683: <http://trac.webkit.org/changeset/215683>

This failed to build on our minimum dependencies baseline bot for Debian stable:

https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/1047/steps/compile-webkit/logs/stdio

[Michael Catanzaro]

Also got a warning to fix:

[1240/5820] Building CXX object Source...emote/glib/RemoteInspectorServer.cpp.o
../../Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:128:1: warning: missing initializer for member ‘_GDBusInterfaceVTable::padding’ [-Wmissing-field-initializers]
 };
 ^

[Carlos Garcia Campos]

(In reply to Carlos Alberto Lopez Perez from comment #26)
> (In reply to Carlos Garcia Campos from comment #25)
> > Committed r215683: <http://trac.webkit.org/changeset/215683>
> 
> This failed to build on our minimum dependencies baseline bot for Debian
> stable:
> 
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20Debian%20Stable%20%28Build%29/builds/1047/steps/compile-
> webkit/logs/stdio

I noticed it yesterday, but didn't have time to fix it, I'll do it today.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #27)
> Also got a warning to fix:
> 
> [1240/5820] Building CXX object
> Source...emote/glib/RemoteInspectorServer.cpp.o
> ../../Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:
> 128:1: warning: missing initializer for member
> ‘_GDBusInterfaceVTable::padding’ [-Wmissing-field-initializers]
>  };
>  ^

Also aware of it, but I don't know how to fix it. It's harmless anyway, it's because of the padding

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #28)
> (In reply to Carlos Alberto Lopez Perez from comment #26)
> > (In reply to Carlos Garcia Campos from comment #25)
> > > Committed r215683: <http://trac.webkit.org/changeset/215683>
> > 
> > This failed to build on our minimum dependencies baseline bot for Debian
> > stable:
> > 
> > https://build.webkit.org/builders/GTK%20Linux%2064-
> > bit%20Release%20Debian%20Stable%20%28Build%29/builds/1047/steps/compile-
> > webkit/logs/stdio
> 
> I noticed it yesterday, but didn't have time to fix it, I'll do it today.

The build failure is because gtk_window_set_titlebar() is available since 3.10, but gtk_window_get_titlebar() was added in 3.16. That's probably why in WebInspectorProxy we keep a pointer to the header bar. Anyway, I had plans to add a WebKitInspectorWindow to be used by both WebInspectorProxy and RemoteWebInspectorProxy, so I'll do that and fix the build at the same time.

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #30)
> (In reply to Carlos Garcia Campos from comment #28)
> > (In reply to Carlos Alberto Lopez Perez from comment #26)
> > > (In reply to Carlos Garcia Campos from comment #25)
> > > > Committed r215683: <http://trac.webkit.org/changeset/215683>
> > > 
> > > This failed to build on our minimum dependencies baseline bot for Debian
> > > stable:
> > > 
> > > https://build.webkit.org/builders/GTK%20Linux%2064-
> > > bit%20Release%20Debian%20Stable%20%28Build%29/builds/1047/steps/compile-
> > > webkit/logs/stdio
> > 
> > I noticed it yesterday, but didn't have time to fix it, I'll do it today.
> 
> The build failure is because gtk_window_set_titlebar() is available since
> 3.10, but gtk_window_get_titlebar() was added in 3.16. That's probably why
> in WebInspectorProxy we keep a pointer to the header bar. Anyway, I had
> plans to add a WebKitInspectorWindow to be used by both WebInspectorProxy
> and RemoteWebInspectorProxy, so I'll do that and fix the build at the same
> time.

See https://bugs.webkit.org/show_bug.cgi?id=171261

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #29)
> (In reply to Michael Catanzaro from comment #27)
> > Also got a warning to fix:
> > 
> > [1240/5820] Building CXX object
> > Source...emote/glib/RemoteInspectorServer.cpp.o
> > ../../Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:
> > 128:1: warning: missing initializer for member
> > ‘_GDBusInterfaceVTable::padding’ [-Wmissing-field-initializers]
> >  };
> >  ^
> 
> Also aware of it, but I don't know how to fix it. It's harmless anyway, it's
> because of the padding

You have to initialize it to nullptr.

[Michael Catanzaro]

Bug #171273