Web Inspector: Inspector doesnt support multiple frontends running in parallel

• Component: Web Inspector
• Status: REOPENED ---

[Andre Moreira Magalhaes]

WebKit inspector should allow connecting multiple frontends (e.g. Inspect Element UI + browser on WEBKIT_INSPECTOR_SERVER addr + eclipse chromedevtools plugin) and send responses to the correct frontends when receiving a method call.

[Radar WebKit Bug Importer]

<rdar://problem/24205647>

[BJ Burg]

We started work in this direction. It's not finished yet, but it would be good to continue it.

*** This bug has been marked as a duplicate of bug 148481 ***

[Andre Moreira Magalhaes]

Reopening this one as the change here is different than the one in bug 148481 and bug and bug 148902, where the original patch was sent in.

I will push separate patches for 148902 and here (with the one here depending on the one in 148902).

[Andre Moreira Magalhaes]

Created attachment 279805 [details]
Staged patch

Note that the original patch from bug 148902 was split in 2 (as requested on the bug), with this one handling the multiple remote frontends support.

Please use the combined patch on https://bugs.webkit.org/attachment.cgi?id=279620 to test functionality.

The new patches (when combined) are the same as the combined patch but with updated ChangeLog entries.

Not marking to review as this patch wont build without the one from bug 148902.

[Andre Moreira Magalhaes]

Created attachment 285015 [details]
Patch

[Andre Moreira Magalhaes]

(In reply to comment #5)
> Created attachment 285015 [details]
> Patch

Rebased patch - not marking as review as this one requires changes from bug #148902 and wont build otherwise.

[BJ Burg]

Comment on attachment 285015 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Web Inspector: Inspector doesnt support multiple frontends running in parallel

Nit: doesn't

I think this bug title should talk specifically about WebInspectorServer. The code path through RemoteInspector is separate and not addressed by this patch.

> Source/WebCore/inspector/InspectorController.cpp:335
>  {

I am wary of changes to InspectorController::show because it's relied upon in subtle ways when popping up a local frontend.

We need to decide what show() should do in the presence of multiple frontends. Should it tell all frontends that they are visible? Should it show/bringToFront only a local frontend? Should it bring to front the most recently connected frontend? I'm not sure, but we should decide something and make sure it actually works as intended (for both RemoteInspector and WebInspectorServer).

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:70
> +    for (auto it = m_connectionMap.begin(); it != end; ++it) {

This is unsafe, because it mutates m_connectionMap while iterating over it. You need to make a copy of the list of connections and iterate over that instead.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:73
> +            WebInspectorProxy* client = m_clientMap.get(connection->identifier());

Aside: The two map members in this class are poorly named. 'client' and 'connection' are too vague in this part of the code base. Ideally they should be mentioning webpages and frontends. The key type should be way more obvious- right now it seems to be connection->identifier(), but also sometimes the pageID is used too.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:97
> +        for (WebSocketServerConnection* connection : connections)

See comment above about mutating while iterating.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:109
> +    Vector<WebSocketServerConnection*> connections = m_connectionMap.get(pageIdForConnection);

A double-map seems kind of clunky and unnecessary, considering that there will only ever be 1 frontend most of the time. Can this code take the Vector<std::tuple<frontend, webpage>> type approach that's used in JSC / WebCore parts of Web Inspector?

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:185
> +    client->remoteFrontendConnected(reinterpret_cast<uint64_t>(connection));

Uh, let's not do this. It's a bad idea and also makes debugging horrible. If you really need to use an opaque ID, then have each connection carry it around to identify itself.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:194
> +    client->dispatchMessageFromRemoteFrontend(reinterpret_cast<uint64_t>(connection), message);

See above.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:213
> +        client->remoteFrontendDisconnected(reinterpret_cast<uint64_t>(connection));

See above.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.h:50
> +    void sendMessageOverConnection(uint64_t connectionID, unsigned pageIdForConnection, const String& message);

I have no idea what this method might do. Message from backend to frontend? Frontend to backend? It should be obvious. It is not clear from the design why there is a connection per-pageId instead of per-frontend. A frontend can't inspect more than one page at a time, at least in Mac ports.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.h:72
> +    HashMap<unsigned, Vector<WebSocketServerConnection*>> m_connectionMap;

See note above. This is not necessary.

> Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:57
> +WebInspectorRemoteManager::WebInspectorRemoteManager(WebPage* page)

If this object is owned by page, then m_page should be a Page& reference instead of a pointer.

> Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:67
> +uint64_t WebInspectorRemoteManager::tokenForRemoteFrontend(const RefPtr<WebInspector>& remoteFrontend) const

the const RefPtr<>& parameter type is unusual. Normally we would pass RefPtr<WebInspector>&&.

> Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:74
> +    return -1;

Use Optional<T> as the return type, not a magic number.

> Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.messages.in:1
> +# Copyright (C) 2010, 2014 Apple Inc. All rights reserved.

Copyright is outdated. Please update.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1334
> +    RefPtr<WebInspectorRemoteManager> m_inspectorRemoteManager;

This does not have shared ownership with other classes and it is nullable. So it should be a std::unique_ptr instead of a RefPtr.