Bug 151576 – Add a WebKit SPI for registering an automation controller with RemoteInspector

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Blaze Burg]

We may want to create a WebAutomationTarget while no web content tabs are open. But, the initial connection is currently established when creating the first WebProcessPool. Let's move the initialization earlier, independently of hooking up WebAutomationTarget itself.

[Radar WebKit Bug Importer]

<rdar://problem/23651703>

[Blaze Burg]

Current plan is to make this registration lazy and also the point where the application delegate is installed.

 - AutomationTargetController is a delegate with two jobs: decide whether remote automation is allowed, and handle requests for new AutomationTargets.

 - The delegate is plugged in to RemoteInspector with a new SPI.

 - WebKit clients can decide when they want to install this delegate based on some policy. We don't want to do it by default, since not all WK clients want to support automation. In that case, there's no point registering with the daemon.

 - Generally, if the client supports automation and it and can be turned on or off dynamically, then the client should register immediately so the automation target is listed as enabled or disabled.

[Blaze Burg]

Created attachment 266948 [details]
Proposed Fix

[Blaze Burg]

Created attachment 266955 [details]
Proposed Fix (Rebased)

[Blaze Burg]

Created attachment 266966 [details]
Proposed Fix (rebased)

[Joseph Pecoraro]

Comment on attachment 266966 [details]
Proposed Fix (rebased)

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

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:40
> +OBJC_CLASS RemoteInspectorObjCAdapter;

I think we may want to name this differently, or at least underscore prefix it. ObjC class names are global names that can collide with anyone's program that uses this public framework (JavaScriptCore.framework). ObjC frameworks avoid collisions by using name prefixes (NS, Web, WK, JS). My naive suggestion for this class would be _RemoteInspectorObjCAdapter, or JSRemoteInspectorObjCAdapter.

In fact, I'm surprised you don't get a build error from the "Tools/Scripts/check-for-inappropriate-objc-class-names" build phase of JavaScriptCore. Maybe because this class is defined only inside of an implementation file this is okay? I'm still cautious.

---

On a related note, you can drive-by remove this now stale comment because the thing it was commenting has since been removed (mid 2014).

Source/JavaScriptCore/llvm/InitializeLLVMMac.cpp
// Use the "JS" prefix to make check-for-inappropriate-objc-class-names happy. I
// think this is better than hacking that script.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:47
> +// RemoteInspectorObjCAdapter is a helper object used as a notification observer
> +// for the sole purpose of getting back into the C++ code from an ObjC block/caller.

I feel like this comment is covered by the ObjCAdapter suffix.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:347
> +    [center addObserver:m_automationAvailabilityObserver.leakRef() selector:@selector(clientCapabilitiesDidChange) name:@WIRAutomationAvailabilityChangedNotification object:nil];

Why `leakRef` and not `get`?

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:391
> +    [[NSNotificationCenter defaultCenter] removeObserver:m_automationAvailabilityObserver.leakRef()];

Why `leakRef` and not `get`?

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.mm:35
> +using namespace Inspector;
> +
> +class RemoteInspectorClientAdapter : public RemoteInspector::Client {

I almost think this would be cleared without the using namespace, so we it is easy to know this is Inspector::RemoteInspector::Client. But maybe that is just me. Most of the time these types of classes are defined in header files where `using` statements are avoided, so normally the full qualifiers are there.

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.mm:98
> +- (void)setDelegate:(id <_WKAutomationControllerDelegate>)delegate
> +{
> +    // Force the old adapter to disconnect from RemoteInspector first.
> +    _clientAdapter = nil;
> +
> +    if (delegate) {
> +        _delegate = delegate;
> +        _clientAdapter = std::make_unique<RemoteInspectorClientAdapter>(self);
> +    }
> +}

If a client wants to clear their delegate via `[[_WKAutomationController sharedController] setDelegate:nil]` this code would not have cleared `delegate`.

Seems we would unconditionally want the `_delegate = delegate;` statement.

[Blaze Burg]

Comment on attachment 266966 [details]
Proposed Fix (rebased)

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

>> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.mm:98
>> +}
> 
> If a client wants to clear their delegate via `[[_WKAutomationController sharedController] setDelegate:nil]` this code would not have cleared `delegate`.
> 
> Seems we would unconditionally want the `_delegate = delegate;` statement.

Ok. We should only install the Client if delegate is no-nil, though.

[Blaze Burg]

Created attachment 267038 [details]
Proposed Fix

[Joseph Pecoraro]

Comment on attachment 267038 [details]
Proposed Fix

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

Looks good to me after addressing these issues. Will need a WK2 Owner I believe for the new WK2 class.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:255
> +void RemoteInspector::setRemoteInspectorClient(RemoteInspector::Client* client)
> +{
> +    ASSERT_ARG(client, client);
> +    ASSERT(!m_client);

This requires that there be a client. But above we may be clearing the client in some cases by passing nullptr here.

I tend to write this assert as:

    ASSERT(!!m_client != !!client);

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.mm:33
> +class RemoteInspectorClientAdapter : public Inspector::RemoteInspector::Client {

Some large amount of this file will need #if ENABLE(REMOTE_INSPECTOR) as Inspector::RemoteInspector is not available otherwise.

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.mm:53
> +RemoteInspectorClientAdapter::~RemoteInspectorClientAdapter()
> +{
> +    Inspector::RemoteInspector::singleton().setRemoteInspectorClient(nullptr);
> +}

This would cause the assert above.

[Blaze Burg]

Created attachment 267040 [details]
Proposed Fix (nits)

[Anders Carlsson]

Comment on attachment 267040 [details]
Proposed Fix (nits)

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.h:39
> +@property (nonatomic, assign) id <_WKAutomationControllerDelegate> delegate;

This should be weak.

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.mm:43
> +    _WKAutomationController *m_controller;

This should use WeakObjCPtr.

[Blaze Burg]

Created attachment 267130 [details]
Proposed Fix (more nits)

Comment on attachment 267130 [details]
Proposed Fix (more nits)

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.h:32
> +@protocol _WKAutomationControllerDelegate <NSObject>

A protocol definition also needs availability.

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.h:35
> +- (BOOL)allowsRemoteAutomation;
> +- (void)requestAutomationSession;
> +@end

This protocol doesn’t follow the normal delegate pattern: (1) it has required methods and (2) the methods don’t have an argument which is the delegating object…

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.h:40
> +@property (nonatomic, weak) id <_WKAutomationControllerDelegate> delegate;
> ++ (_WKAutomationController *)sharedController;

…moreover, it is highly unusual to give a singleton object a  delegate.

Does the automation controller really need to be a global shared resource? We try to avoid that in the WebKit API. Can’t this be a per-WKWebView, per-WKWebViewConfiguration or per-WKProcessPool object?

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.mm:78
> +    id <_WKAutomationControllerDelegate> _delegate;

Since the property is declared weak, you should use a WeakObjCPtr ivar, so that clients get the expected behavior.

[Blaze Burg]

(In reply to comment #13)
> Comment on attachment 267130 [details]
> Proposed Fix (more nits)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267130&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.h:32
> > +@protocol _WKAutomationControllerDelegate <NSObject>
> 
> A protocol definition also needs availability.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.h:35
> > +- (BOOL)allowsRemoteAutomation;
> > +- (void)requestAutomationSession;
> > +@end
> 
> This protocol doesn’t follow the normal delegate pattern: (1) it has
> required methods and (2) the methods don’t have an argument which is the
> delegating object…

Will fix.

> > Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.h:40
> > +@property (nonatomic, weak) id <_WKAutomationControllerDelegate> delegate;
> > ++ (_WKAutomationController *)sharedController;
> 
> …moreover, it is highly unusual to give a singleton object a  delegate.
> 
> Does the automation controller really need to be a global shared resource?
> We try to avoid that in the WebKit API. Can’t this be a per-WKWebView,
> per-WKWebViewConfiguration or per-WKProcessPool object?

It is per browser context; it's the thing that creates new windows in automation mode in response to requests over XPC (endpoint is in JavaScriptCore). It can't be per-WKWebView since there may not be any web views open yet.

I will rewrite this to be a WKProcessPool delegate. It seems the most appropriate place in the modern API.

> > Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationController.mm:78
> > +    id <_WKAutomationControllerDelegate> _delegate;
> 
> Since the property is declared weak, you should use a WeakObjCPtr ivar, so
> that clients get the expected behavior.

OK

[Blaze Burg]

Created attachment 267487 [details]
Proposed Fix (v2)

New patch, now it's rewritten as a WKProcessPool delegate based off _WKDownloadDelegate.

[Joseph Pecoraro]

Comment on attachment 267487 [details]
Proposed Fix (v2)

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

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:389
> +    [[NSNotificationCenter defaultCenter] removeObserver:m_automationAvailabilityObserver.get()];

Since all we would do with this is re-create it in start. You could release the observer: m_automationAvailabilityObserver = nil;

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:54
> +
>  

Nit: 1 newline instead of 2.

> Source/WebKit2/UIProcess/Cocoa/AutomationClient.h:41
> +class AutomationClient final : public API::AutomationClient, Inspector::RemoteInspector::Client {

Should have ENABLE(REMOTE_INSPECTOR) around this class I think, otherwise the Inspector::RemoteInspector::Client is not available. We get lucky due to the WK_API_ENABLED. Maybe in such cases we've been dropping the known to be enabled ENABLE guards?

> Source/WebKit2/UIProcess/Cocoa/AutomationClient.mm:46
> +    // We don't want RemoteInspector to open an XPC connection unless it can report inspection targets,
> +    // automation targets, or we receive a notification to pre-emptively register with webinspectord.
> +    RemoteInspector::startDisabled();

I'm not sure this makes sense. If the Application sets an AutomationClient (via an explicit API call) and allows automation then it should be available to remote debuggers regardless of whether or not it has inspection targets, right? Seems we can just remove this.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:292
> +
> +

Nit: 1 newline instead of 2.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:648
> -    Inspector::RemoteInspector::singleton(); 
> +    Inspector::RemoteInspector::singleton().start();

I think this breaks the existing auto-start behavior, so r- for this.

By default, the RemoteInspector will auto-start when created (inside RemoteInspector::singleton()) so that developers don't have to do anything to have things be debuggable. However, some processes may choose to opt-out of this auto-start behavior (for whatever reason they want but often because they just want to disable remote inspection in their process) by using the JSRemoteInspectorDisableAutoStart() SPI. The explicit call to start above will start, regardless of the auto-start setting, which would have started inside construction. I see now that you're using RemoteInspector::startDisabled above, but in a way that would break the SPI others are using.

[Blaze Burg]

Comment on attachment 267487 [details]
Proposed Fix (v2)

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

>> Source/WebKit2/UIProcess/Cocoa/AutomationClient.h:41
>> +class AutomationClient final : public API::AutomationClient, Inspector::RemoteInspector::Client {
> 
> Should have ENABLE(REMOTE_INSPECTOR) around this class I think, otherwise the Inspector::RemoteInspector::Client is not available. We get lucky due to the WK_API_ENABLED. Maybe in such cases we've been dropping the known to be enabled ENABLE guards?

OK.

>> Source/WebKit2/UIProcess/Cocoa/AutomationClient.mm:46
>> +    RemoteInspector::startDisabled();
> 
> I'm not sure this makes sense. If the Application sets an AutomationClient (via an explicit API call) and allows automation then it should be available to remote debuggers regardless of whether or not it has inspection targets, right? Seems we can just remove this.

I'll remove it.

>> Source/WebKit2/UIProcess/WebProcessPool.cpp:648
>> +    Inspector::RemoteInspector::singleton().start();
> 
> I think this breaks the existing auto-start behavior, so r- for this.
> 
> By default, the RemoteInspector will auto-start when created (inside RemoteInspector::singleton()) so that developers don't have to do anything to have things be debuggable. However, some processes may choose to opt-out of this auto-start behavior (for whatever reason they want but often because they just want to disable remote inspection in their process) by using the JSRemoteInspectorDisableAutoStart() SPI. The explicit call to start above will start, regardless of the auto-start setting, which would have started inside construction. I see now that you're using RemoteInspector::startDisabled above, but in a way that would break the SPI others are using.

I don't think this change is necessary, so I'll revert it.

[Blaze Burg]

Created attachment 267501 [details]
Proposed Fix (v2, revised)

[Joseph Pecoraro]

Comment on attachment 267501 [details]
Proposed Fix (v2, revised)

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

Looks good to me! Needs a WK2 Owner

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:52
> +@property (nonatomic, weak, setter=_setAutomationDelegate:) id<_WKAutomationDelegate> _automationDelegate;

Style: "id<" => "id <"

Comment on attachment 267501 [details]
Proposed Fix (v2, revised)

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

> Source/JavaScriptCore/ChangeLog:27
> +        * inspector/remote/RemoteInspector.h:
> +        * inspector/remote/RemoteInspector.mm:
> +        (-[JSRemoteInspectorObjCAdapter clientCapabilitiesDidChange]):
> +        (Inspector::RemoteInspector::setRemoteInspectorClient):
> +        (Inspector::RemoteInspector::start):
> +        (Inspector::RemoteInspector::stopInternal):
> +        (Inspector::RemoteInspector::pushListingsNow):
> +        * inspector/remote/RemoteInspectorConstants.h:

Missing function-level comments.

> Source/WebKit2/ChangeLog:36
> +        * PlatformMac.cmake:
> +        * UIProcess/API/APIAutomationClient.h: Added.
> +        (API::AutomationClient::~AutomationClient):
> +        (API::AutomationClient::allowsRemoteAutomation):
> +        (API::AutomationClient::requestAutomationSession):
> +        * UIProcess/API/Cocoa/WKProcessPool.mm:
> +        (-[WKProcessPool _automationDelegate]):
> +        (-[WKProcessPool _setAutomationDelegate:]):
> +        * UIProcess/API/Cocoa/WKProcessPoolPrivate.h:
> +        * UIProcess/API/Cocoa/_WKAutomationDelegate.h: Added.
> +        * UIProcess/Cocoa/AutomationClient.h: Added.
> +        * UIProcess/Cocoa/AutomationClient.mm: Added.
> +        (WebKit::AutomationClient::AutomationClient):
> +        (WebKit::AutomationClient::~AutomationClient):
> +        (WebKit::AutomationClient::remoteAutomationAllowed):
> +        (WebKit::AutomationClient::requestAutomationSession):
> +        * UIProcess/WebProcessPool.cpp:
> +        (WebKit::WebProcessPool::WebProcessPool):
> +        (WebKit::WebProcessPool::setAutomationClient):
> +        * UIProcess/WebProcessPool.h:
> +        * WebKit2.xcodeproj/project.pbxproj:

Ditto.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:40
>  OBJC_CLASS NSDictionary;
>  OBJC_CLASS NSString;
> +OBJC_CLASS JSRemoteInspectorObjCAdapter;

These should remain sorted.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:255
> +    ASSERT_ARG(client, client);
> +    ASSERT(!!m_client != !!client);

If the first assertion hold, then the second assertion is simply that m_client is null, so it should be written that way.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:346
> +    m_automationAvailabilityObserver = adoptNS([JSRemoteInspectorObjCAdapter new]);
> +    NSNotificationCenter *center = [NSNotificationCenter defaultCenter];
> +    [center addObserver:m_automationAvailabilityObserver.get() selector:@selector(clientCapabilitiesDidChange) name:@WIRAutomationAvailabilityChangedNotification object:nil];
> +

It’s unusual to have an API where the client is the sender of notifications rather than an observer, and it’s strange to implement it like this in JavaScriptCore. Is there a reason why the API for this couldn’t be a method on WKProcessPool?

> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationDelegate.h:33
> +- (BOOL)_allowsRemoteAutomation;
> +- (void)_requestAutomationSession;

As mentioned before, delegate methods should take the delegating objects as an argument:
- (BOOL)_processPoolAllowsRemoteAutomation:(WKPorcessPool *)processPool;
and something like
- (void)_processPoolDidRequestAutomationSession:(WKPorcessPool *)processPool;

What’s the significance of “remote” in the names? One method has it and the other doesn’t.

[Blaze Burg]

Comment on attachment 267501 [details]
Proposed Fix (v2, revised)

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

>> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:255
>> +    ASSERT(!!m_client != !!client);
> 
> If the first assertion hold, then the second assertion is simply that m_client is null, so it should be written that way.

Oops, this used to make more sense. Will fix.

>> Source/JavaScriptCore/inspector/remote/RemoteInspector.mm:346
>> +
> 
> It’s unusual to have an API where the client is the sender of notifications rather than an observer, and it’s strange to implement it like this in JavaScriptCore. Is there a reason why the API for this couldn’t be a method on WKProcessPool?

The essential design issue here is that RemoteInspector needs to eagerly report status changes (via XPC) that originate way above JSC, in WK2 or its clients. Notifications are the easiest way to jump the abstraction boundaries. This could instead be a notification-like SPI on RemoteInspector (JSRemoteInspectorAutomationAvailabilityChanged), but I don't see how it's any cleaner.

We can't move this reporting code to WKProcessPool, as RemoteInspector is used for JSContext inspection.

>> Source/WebKit2/UIProcess/API/Cocoa/_WKAutomationDelegate.h:33
>> +- (void)_requestAutomationSession;
> 
> As mentioned before, delegate methods should take the delegating objects as an argument:
> - (BOOL)_processPoolAllowsRemoteAutomation:(WKPorcessPool *)processPool;
> and something like
> - (void)_processPoolDidRequestAutomationSession:(WKPorcessPool *)processPool;
> 
> What’s the significance of “remote” in the names? One method has it and the other doesn’t.

Oops, will fix.

In the first method, "Remote" means that automation is being controlled by an external program over XPC, rather than locally inside UIProcess. (We want to support local and remote automation and have independent settings for each.)
The second method is called in both the local and remote cases to create a new automation window, so it doesn't mention 'remote'.

[Blaze Burg]

Created attachment 268231 [details]
Proposed Fix v2.1

New patch adds the delegating self argument to _WKAutomationDelegate and other fixes. I didn't change the NSNotification from RemoteInspector since it's unclear that there's a better way to do it.

[Blaze Burg]

Created attachment 268338 [details]
Proposed Fix v3

This v3 patch removes the hacky NSNotification listener from RemoteInspector in favor of a WKProcessPool SPI t to achieve the same effect.

Comment on attachment 268338 [details]
Proposed Fix v3

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

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:52
> +@property (nonatomic, weak, setter=_setAutomationDelegate:) id <_WKAutomationDelegate> _automationDelegate;

Needs availability.

> Source/WebKit2/UIProcess/Cocoa/AutomationClient.h:27
> +#ifndef AutomationClient_h
> +#define AutomationClient_h

No need for this in an Objective-C header

> Source/WebKit2/UIProcess/WebProcessPool.cpp:1078
> +#if PLATFORM(COCOA)

Why?

> Source/WebKit2/UIProcess/WebProcessPool.h:254
> +#if PLATFORM(COCOA)

Why?

[Blaze Burg]

(In reply to comment #24)
> Comment on attachment 268338 [details]
> Proposed Fix v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268338&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:52
> > +@property (nonatomic, weak, setter=_setAutomationDelegate:) id <_WKAutomationDelegate> _automationDelegate;
> 
> Needs availability.

OK

> 
> > Source/WebKit2/UIProcess/Cocoa/AutomationClient.h:27
> > +#ifndef AutomationClient_h
> > +#define AutomationClient_h
> 
> No need for this in an Objective-C header
> 
> > Source/WebKit2/UIProcess/WebProcessPool.cpp:1078
> > +#if PLATFORM(COCOA)
> 
> Why?
> 
> > Source/WebKit2/UIProcess/WebProcessPool.h:254
> > +#if PLATFORM(COCOA)
> 
> Why?

Hmm, I guess it's redundant with ENABLE(REMOTE_INSPECTOR), since only Cocoa ports use RemoteInspector.

[Blaze Burg]

Committed r194646: <http://trac.webkit.org/changeset/194646>