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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 21 11:32:44 PDT 2017


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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #307373|review?                     |review-
              Flags|                            |

--- Comment #12 from Michael Catanzaro <mcatanzaro 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

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?

-- 
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/20170421/8b5fe3af/attachment-0001.html>


More information about the webkit-unassigned mailing list