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

Sat Apr 22 00:27:34 PDT 2017

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

--- Comment #15 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 307373
  --> https://bugs.webkit.org/attachment.cgi?id=307373
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170422/5732aa7a/attachment-0001.html>