[webkit-reviews] review denied: [Bug 166680] [GTK] Switch to use ENABLE_REMOTE_INSPECTOR instead of ENABLE_INSPECTOR_SERVER for the remote inspector : [Attachment 307373] Rebased patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 21 11:32:44 PDT 2017
Michael Catanzaro <mcatanzaro at igalia.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 166680: [GTK] Switch to use ENABLE_REMOTE_INSPECTOR instead of
ENABLE_INSPECTOR_SERVER for the remote inspector
https://bugs.webkit.org/show_bug.cgi?id=166680
Attachment 307373: Rebased patch
https://bugs.webkit.org/attachment.cgi?id=307373&action=review
--- 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?
More information about the webkit-reviews
mailing list