Bug 191344 – Web Inspector: Restrict domains at the target level instead of only at the window level

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Restrict domains at the target level instead of only at the window level

  • Currently we use `InspectorBackend.activateDomain` to restrict whether or not `window.FooAgent` is available in the main debuggable.
  • Currently we use `InspectorBackend.workerSupportedDomains` to restrict whether or not `target.FooAgent` is available to a Worker target.

Lets unify these into a single path:

  • activateDomain knows if a domain is supported for a particular debuggableType/target
  • only expose supported domains on a Target, restrict them to the list activateDomain modified

[Joseph Pecoraro]

Created attachment 354055 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Comment on attachment 354055 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/ChangeLog:13
> +        by availabity being empty (meaning it is supported everywhere).

This means that `GenericTypes`, `Inspector`, `OverlayTypes`, and `ScriptProfiler will now all be available to workers.  I think that's fine for `GenericTypes` and `Inspector`, but what about `OverlayTypes` (there is no overlay page for workers) and `ScriptProfiler` (is this supported for worker JS)?

> Source/JavaScriptCore/inspector/protocol/Recording.json:4
> +    "availability": ["web"],

Theoretically, there's nothing preventing the `Recording` domain from being available everywhere, but as of now it's only used by `Canvas` so I suppose this is fine.

> Source/WebInspectorUI/ChangeLog:10
> +        that if we are checking for, or using a feature, we should ideally be using

Grammar: "for, or using, a"

> Source/WebInspectorUI/ChangeLog:12
> +        an unprefixed FooAgent. More changes to follow.

NIT: you add backticks to the other code snippets, so `FooAgent` should also probably have them

> Source/WebInspectorUI/ChangeLog:15
> +        * UserInterface/Base/DebuggableType.js:

NIT: I try to add "Added." and "Deleted."/"Removed." text after any file/function/selector that I modify in the ChangeLog

> Source/WebInspectorUI/ChangeLog:29
> +        clone all of the agents, we don't need to do anything special.

Grammar: the part after the comma should really be in parentheses

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:46
> +            this._supportedDomainsForDebuggableType.set(debuggableType, []);

NIT: you could use a `Set` instead of an array here

[Joseph Pecoraro]

Comment on attachment 354055 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/ChangeLog:13
>> +        by availabity being empty (meaning it is supported everywhere).
> 
> This means that `GenericTypes`, `Inspector`, `OverlayTypes`, and `ScriptProfiler will now all be available to workers.  I think that's fine for `GenericTypes` and `Inspector`, but what about `OverlayTypes` (there is no overlay page for workers) and `ScriptProfiler` (is this supported for worker JS)?

• `Inspector` is supported by workers but it was never used and looks like it isn't needed... so I'll remove it.
• `ScriptProfiler` is not currently supported by workers (I thought it was, and it should be in the future). For now I'll put an availability on it for ["web", "javascript"]
• `GenericTypes` and `OverlayTypes` are domains with types and no enums, so they don't actually generate anything in the frontend

>> Source/JavaScriptCore/inspector/protocol/Recording.json:4
>> +    "availability": ["web"],
> 
> Theoretically, there's nothing preventing the `Recording` domain from being available everywhere, but as of now it's only used by `Canvas` so I suppose this is fine.

Correct, but it has enum types so I didn't want it to be exposed in workers just yet.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:46
>> +            this._supportedDomainsForDebuggableType.set(debuggableType, []);
> 
> NIT: you could use a `Set` instead of an array here

We only ever append to the list, are guaranteed no duplicates, and iterate the list for new targets. This is perfect for an array, a Set will just be additional work for each of these operations for no benefit (at least at the moment).

[Joseph Pecoraro]

> • `Inspector` is supported by workers but it was never used and looks like
> it isn't needed... so I'll remove it.

Technically it could be reached if you did `inspect(...)` in the quick console of a Worker's execution context. However, again it was never actually initialized in workers so this wouldn't have worked. Secondly, there is nothing that you can inspect and show at the moment (storages aren't hooked up for workers yet).

[Joseph Pecoraro]

Created attachment 354127 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 354129 [details]
[PATCH] Proposed Fix

Included ChangeLog grammar fixes I forgot.

[EWS Watchlist]

Comment on attachment 354129 [details]
[PATCH] Proposed Fix

Attachment 354129 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9897712

New failing tests:
http/tests/misc/resource-timing-resolution.html

[EWS Watchlist]

Created attachment 354139 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Joseph Pecoraro]

Comment on attachment 354129 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:46
> +        this._agents = {};

Oops, I should remove the `this._agents = ...` line further above.

[Joseph Pecoraro]

Created attachment 354142 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 354142 [details]
[PATCH] Proposed Fix

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

r=me, with a few comments/concerns

> Source/WebCore/inspector/WorkerInspectorController.cpp:82
> +        commandLineAPIHost->init(nullptr, m_instrumentingAgents->webConsoleAgent(), nullptr, nullptr, nullptr);

This means that we won't be able to ever `inspect` anything.  I'm guessing that that's fine for now, as the only thing we are able to `inspect` inside a worker is a "function" (no DOM nodes, databases, or DOM storages).

> Source/WebInspectorUI/ChangeLog:56
> +        Regenerate protocol files now that workerSupportedDomains is unnecessary
> +        and explicit availability has been added to other domains.

Should we bother updating the availability of protocol files before iOS 10.3 (e.g. setting "Inspector" to "web" only), or is that not really an issue since worker support was introduced in iOS 10.3?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:179
> +        return this._supportedDomainsForDebuggableType.get(type);

Maybe add an assertion here?

    console.assert(Object.values(WI.DebuggableType).includes(type), "Unknown debuggable type", type);

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:46
> +        this._agents = {};

NIT: I know that this isn't an issue right now, but the ordering of this (set connection target, then set agents) scares me a bit, especially since `target` is a setter.  I'd personally rather put the connection setter below the agents initialization.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:49
> +            this._agents[domain] = connection._agents[domain];

Style: I try to use the member variable whenever possible, even in the constructor, so that there is as little chance for side-effect changes as possible.

    this._agents[domain] = this._connection._agents[domain];

[Blaze Burg]

Comment on attachment 354142 [details]
[PATCH] Proposed Fix

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

LGTM!

> Source/JavaScriptCore/ChangeLog:13
> +        by availabity being empty (meaning it is supported everywhere).

Nit: availability

> Source/JavaScriptCore/ChangeLog:35
> +        Allow availiabilty on a domain with only types.

Nit: availability

>> Source/WebInspectorUI/ChangeLog:56
>> +        and explicit availability has been added to other domains.
> 
> Should we bother updating the availability of protocol files before iOS 10.3 (e.g. setting "Inspector" to "web" only), or is that not really an issue since worker support was introduced in iOS 10.3?

I don't think it's worth bothering.

> Source/WebInspectorUI/UserInterface/Base/DebuggableType.js:-48
> -    {

I don't understand why Bugzilla is showing this. It was a copied file?

[Joseph Pecoraro]

Comment on attachment 354142 [details]
[PATCH] Proposed Fix

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

>> Source/WebCore/inspector/WorkerInspectorController.cpp:82
>> +        commandLineAPIHost->init(nullptr, m_instrumentingAgents->webConsoleAgent(), nullptr, nullptr, nullptr);
> 
> This means that we won't be able to ever `inspect` anything.  I'm guessing that that's fine for now, as the only thing we are able to `inspect` inside a worker is a "function" (no DOM nodes, databases, or DOM storages).

Correct. I mentioned that above. We can add this feature back in later, but older backends weren't even enabling this domain so it wouldn't have worked.

>>> Source/WebInspectorUI/ChangeLog:56
>>> +        and explicit availability has been added to other domains.
>> 
>> Should we bother updating the availability of protocol files before iOS 10.3 (e.g. setting "Inspector" to "web" only), or is that not really an issue since worker support was introduced in iOS 10.3?
> 
> I don't think it's worth bothering.

Actually, I'll need to add availability: ["javascript", "web"] on a few of them.

>> Source/WebInspectorUI/UserInterface/Base/DebuggableType.js:-48
>> -    {
> 
> I don't understand why Bugzilla is showing this. It was a copied file?

Oops, this was the diff algorithm I used. Normally I use -M100% but generating this one I didn't.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:179
>> +        return this._supportedDomainsForDebuggableType.get(type);
> 
> Maybe add an assertion here?
> 
>     console.assert(Object.values(WI.DebuggableType).includes(type), "Unknown debuggable type", type);

Kk, I had this in the past too, heh.

[Joseph Pecoraro]

https://trac.webkit.org/changeset/237997/webkit

[Radar WebKit Bug Importer]

<rdar://problem/45919586>