Bug 151042 – Web Inspector: RemoteInspector should track targets and connections for remote automation

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

PFR soon.

[Radar WebKit Bug Importer]

<rdar://problem/23466749>

[Blaze Burg]

Created attachment 265923 [details]
Proposed Fix

This might not compile yet, I just rebased it before heading out.

[Joseph Pecoraro]

Any chance you could provide a diff that doesn't treat the renamed files as completely new?

[Build Bot]

Comment on attachment 265923 [details]
Proposed Fix

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 265925 [details]
Archive of layout-test-results from ews112 for mac-yosemite

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

[Joseph Pecoraro]

Comment on attachment 265923 [details]
Proposed Fix

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

Looking good, but I wasn't able to really determine the changes in the Debuggable classes due to all the new files. I'll take a deeper luck later (and maybe apply the patch locally to get a real diff).

> Source/JavaScriptCore/inspector/remote/RemoteControlledTarget.h:39
> +class JS_EXPORT_PRIVATE RemoteControlledTarget {

This name sounds weird to me. Maybe just "RemoteTarget"? I see the idea though. "RemoteTarget" sounds like something remote... Unfortunately here "Remote" is just a prefix, not a description.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:121
> -    HashMap<unsigned, std::pair<RemoteInspectorDebuggable*, RemoteInspectorDebuggableInfo>> m_debuggableMap;
> -    HashMap<unsigned, RefPtr<RemoteInspectorDebuggableConnection>> m_connectionMap;
> +    HashMap<unsigned, RemoteControlledTarget*> m_targetMap;
> +    HashMap<unsigned, RetainPtr<NSDictionary>> m_listingMap;

Oh, much nicer (listingMap instead of sticking the thread safe info into the debuggable map)!

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:688
> +    std::lock_guard<Lock> lock(m_mutex);

Does this lock guard anything? I think the only thing the block captures is self.

> Source/JavaScriptCore/runtime/JSGlobalObjectDebuggable.h:51
> -    virtual Inspector::RemoteInspectorDebuggable::DebuggableType type() const override { return Inspector::RemoteInspectorDebuggable::JavaScript; }
> +    virtual Inspector::RemoteControlledTarget::Type type() const override { return Inspector::RemoteControlledTarget::Type::JavaScript; }

Not sure how I feel about this naming. "RemoteControlled" seems prescriptive rather then descriptive. "Target" and "RemoteInspectionTarget" I think are good.

> Source/WebKit2/UIProcess/WebAutomationTarget.h:36
> +namespace Inspector {
> +class FrontendChannel;
> +}

Probably not needed since these are present in the virtual methods.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:475
> +    m_sharedAutomationTarget->setAutomationAllowed(!privateBrowsingEnabled);

This should be guarded by ENABLE(REMOTE_INSPECTOR).

[Blaze Burg]

Comment on attachment 265923 [details]
Proposed Fix

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

>> Source/JavaScriptCore/inspector/remote/RemoteControlledTarget.h:39
>> +class JS_EXPORT_PRIVATE RemoteControlledTarget {
> 
> This name sounds weird to me. Maybe just "RemoteTarget"? I see the idea though. "RemoteTarget" sounds like something remote... Unfortunately here "Remote" is just a prefix, not a description.

This was my least favorite name in this patch. Maybe ControllableTarget?

> Source/JavaScriptCore/inspector/remote/RemoteControlledTarget.h:55
> +    virtual void dispatchMessageFromRemote(const String& message) = 0;

...and yet the API is somewhat specific to *remote* controlling. It is not that different from local frontend channels, though.

>> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:121
>> +    HashMap<unsigned, RetainPtr<NSDictionary>> m_listingMap;
> 
> Oh, much nicer (listingMap instead of sticking the thread safe info into the debuggable map)!

This was the best solution I could come up with that is polymorphic and not terrible or requiring sending listings from main / target run loop.

>> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:688
>> +    std::lock_guard<Lock> lock(m_mutex);
> 
> Does this lock guard anything? I think the only thing the block captures is self.

Oops, I think it used to guard something in earlier patch, but now the inner lock suffices.

>> Source/JavaScriptCore/runtime/JSGlobalObjectDebuggable.h:51
>> +    virtual Inspector::RemoteControlledTarget::Type type() const override { return Inspector::RemoteControlledTarget::Type::JavaScript; }
> 
> Not sure how I feel about this naming. "RemoteControlled" seems prescriptive rather then descriptive. "Target" and "RemoteInspectionTarget" I think are good.

See above.

>> Source/WebKit2/UIProcess/WebAutomationTarget.h:36
>> +}
> 
> Probably not needed since these are present in the virtual methods.

Oops.

>> Source/WebKit2/UIProcess/WebProcessPool.cpp:475
>> +    m_sharedAutomationTarget->setAutomationAllowed(!privateBrowsingEnabled);
> 
> This should be guarded by ENABLE(REMOTE_INSPECTOR).

OK

[Blaze Burg]

Created attachment 265943 [details]
Proposed Fix v2 (better formatting)

Addressed comments, and told git to treat it as a rename harder. (git diff --staged -M0.5)

[Joseph Pecoraro]

Comment on attachment 265943 [details]
Proposed Fix v2 (better formatting)

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

r=me, but there are some necessary fixes.

> Source/JavaScriptCore/inspector/remote/ControllableTarget.cpp:27
> +#include "ControllableTarget.h"

All of the files in JavaScriptCore/inspector/remote started with the "Remote" prefix. I'm fine with either, but might be nice to be consistent even if the name isn't the greatest.

> Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.hSource/JavaScriptCore/inspector/remote/RemoteInspectorDebuggableConnection.h:94
> +    // Configuring the queue on which dispatches are performed.

None of these seem to have to do with configuring.

> Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.hSource/JavaScriptCore/inspector/remote/RemoteInspectorDebuggableConnection.h:123
> +    ControllableTarget* m_target { nullptr };
> +    unsigned m_identifier {0};

Style: Converge on one style.

> Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mmSource/JavaScriptCore/inspector/remote/RemoteInspectorDebuggableConnection.mm:162
> -            std::lock_guard<Lock> lock(m_debuggableMutex);
> -            if (!m_debuggable || !m_debuggable->remoteDebuggingAllowed() || m_debuggable->hasLocalDebugger()) {
> -                RemoteInspector::singleton().setupFailed(identifier());
> -                m_debuggable = nullptr;
> -            } else {
> -                m_debuggable->connect(this, isAutomaticInspection);
> +            std::lock_guard<Lock> lock(m_targetMutex);
> +            if (!m_target || !m_target->remoteControlAllowed()) {
> +                RemoteInspector::singleton().setupFailed(m_identifier);

This change should be documented in the ChangeLog. Previously we would have failed if inspecting a debuggable that had a local debugger. Now we don't. InspectorRouter allows multiple frontends now.

EDIT: Reading more of the patch I see you moved combined these two checks into remoteControlAllowed. Seems okay.

> Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mmSource/JavaScriptCore/inspector/remote/RemoteInspectorDebuggableConnection.mm:164
> +                return;

This early return is bad. It misses the necessary balancing "deref()" for this connection's action on a different queue.

> Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mmSource/JavaScriptCore/inspector/remote/RemoteInspectorDebuggableConnection.mm:177
> +            if (is<RemoteInspectionTarget>(m_target)) {
> +                auto castedTarget = downcast<RemoteInspectionTarget>(m_target);
> +                castedTarget->connect(this, isAutomaticInspection);
>                  m_connected = true;
>  
>                  if (automaticallyPause)
> -                    m_debuggable->pause();
> +                    castedTarget->pause();
> +            } else if (is<RemoteAutomationTarget>(m_target)) {
> +                auto castedTarget = downcast<RemoteAutomationTarget>(m_target);
> +                castedTarget->connect(this);
>              }

`m_connected = true` should be set for both of these cases in order to call disconnect when the connection is closed. Hoisting this above the if/elses seems fine.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:45
> +class RemoteConnectionToTarget;
> +class ControllableTarget;

Style: sort

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:487
> +    RetainPtr<NSMutableDictionary> listing = [[NSMutableDictionary alloc] init];

This is a leak. It needs adoptNS(...).

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:523
> +    RetainPtr<NSMutableDictionary> listing = [[NSMutableDictionary alloc] init];

This is a leak. It needs adoptNS(...).

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:621
> +        updateHasActiveDebugSession();

We may want this for Automation as well. At the moment this is basically dead code with a FIXME, but its intention is to inform the UIProcess to prevent screen idling behavior on iOS.

> Source/WebKit2/UIProcess/WebAutomationTarget.h:36
> +namespace Inspector {
> +class FrontendChannel;
> +}

Nit: Not needed.

> Source/WebKit2/UIProcess/WebProcessPool.h:543
> +    std::unique_ptr<WebAutomationTarget> m_sharedAutomationTarget;

Is the "shared" prefix significant? Shared with what?

[Blaze Burg]

Created attachment 266023 [details]
Addressed Comments

We may want to hold off landing this until a better location for RemoteAutomationTarget is figured out. Or, if it ends up requiring other changes (to accomodate Host Application being reported earlier), then maybe land this and change in a followup bug.

[Blaze Burg]

(In reply to comment #10)
> Created attachment 266023 [details]
> Addressed Comments
> 
> We may want to hold off landing this until a better location for
> RemoteAutomationTarget is figured out. Or, if it ends up requiring other
> changes (to accomodate Host Application being reported earlier), then maybe
> land this and change in a followup bug.

A potential solution:

 * Host (UIProcess) sets up RemoteInspector connection immediately, instead of on first WebProcess creation.
 * Add an SPI or setting somewhere that can flag a WebProcessPool as being in Remote Automation mode. This causes the UIProcess to instantiate an owned WebAutomationTarget that calls RemoteInspector::{,un}registerTarget.

We definitely should split WebAutomationTarget and the WK2 bits from this patch, which is mainly a refactoring of RemoteInspector.

[Blaze Burg]

Committed r192753: <http://trac.webkit.org/changeset/192753>

[Csaba Osztrogonác]

(In reply to comment #12)
> Committed r192753: <http://trac.webkit.org/changeset/192753>

It broke the Apple Mac cmake build.

[Blaze Burg]

(In reply to comment #13)
> (In reply to comment #12)
> > Committed r192753: <http://trac.webkit.org/changeset/192753>
> 
> It broke the Apple Mac cmake build.

Woops, I should start checking that. This was detectable just in the deps checking, no build required. Fixed in http://trac.webkit.org/changeset/192759 (I messed up the commit message, wrong revision!)