Bug 222568

Summary: Web Inspector: Implement `frameURL` option for `devtools.inspectedWindow.eval` command
Product: WebKit Reporter: Patrick Angle <pangle>
Component: Web InspectorAssignee: Patrick Angle <pangle>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, drousso, ews-watchlist, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1.0
none
Patch v1.1 - Support fuzzier matching, fix eval error handling
none
WIP v1.2 - Test now crashes...
none
Patch v2.0 - Rebased, now-functional test
none
Patch v2.1 - Adjust function naming
none
Patch v2.2 - Address review comments
none
Patch v2.3 - Review nits
ews-feeder: commit-queue-
Patch v2.4 - rebased for commit-queue
none
Patch v2.5 - Rebase for commit-queue ews-feeder: commit-queue-

Description Patrick Angle 2021-03-01 11:13:39 PST
.
Comment 1 Patrick Angle 2021-03-01 11:29:45 PST
Created attachment 421850 [details]
Patch v1.0
Comment 2 Devin Rousso 2021-03-01 12:05:32 PST
Comment on attachment 421850 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:99
> +            frame = WI.networkManager.frames.find(frame => frame.url === frameURL);

Do we care about a situation where there's more than one `WI.Frame` with the same URL?  Is it an exact URL match, or just a prefix match (e.g. the actual URL can have a query parameter that the given `frameURL` doesn't have)?

Style: we always wrap arguments of arrow functions in `(` `)` even if there's only one

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:100
> +            if (!frame) {

Style: no `{` `}` around single-line control flow

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:116
> +        let evaluationContext = frame.pageExecutionContext;

I'm pretty sure it's impossible for a `WI.Frame` to not have a `pageExecutionContext`, but perhaps we should check and return a `WI.WebInspectorExtension.ErrorCode.InternalError` or `WI.WebInspectorExtension.ErrorCode.ContextDestroyed` if so.
Comment 3 Patrick Angle 2021-03-01 13:05:12 PST
Comment on attachment 421850 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:99
>> +            frame = WI.networkManager.frames.find(frame => frame.url === frameURL);
> 
> Do we care about a situation where there's more than one `WI.Frame` with the same URL?  Is it an exact URL match, or just a prefix match (e.g. the actual URL can have a query parameter that the given `frameURL` doesn't have)?
> 
> Style: we always wrap arguments of arrow functions in `(` `)` even if there's only one

Two excellent questions for which I do not have an answer at this time. The documentation is unclear as to how strict the matching is and wether query parameters are part of that match. Along the same train of through, I should probably also be considering escaped vs. unescaped matching as well.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:100
>> +            if (!frame) {
> 
> Style: no `{` `}` around single-line control flow

I always forget to remove these when I reduce the number of lines within to 1.
Comment 4 BJ Burg 2021-03-02 10:16:29 PST
Comment on attachment 421850 [details]
Patch v1.0

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

>>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:99
>>> +            frame = WI.networkManager.frames.find(frame => frame.url === frameURL);
>> 
>> Do we care about a situation where there's more than one `WI.Frame` with the same URL?  Is it an exact URL match, or just a prefix match (e.g. the actual URL can have a query parameter that the given `frameURL` doesn't have)?
>> 
>> Style: we always wrap arguments of arrow functions in `(` `)` even if there's only one
> 
> Two excellent questions for which I do not have an answer at this time. The documentation is unclear as to how strict the matching is and wether query parameters are part of that match. Along the same train of through, I should probably also be considering escaped vs. unescaped matching as well.

Chromium's test suite doesn't test any interesting cases here, so it's up to us. I'd prefer to remove string query parameters and normalize the URL if possible (to the extent that it would allow more matches).
Comment 5 Patrick Angle 2021-03-02 10:20:07 PST
Comment on attachment 421850 [details]
Patch v1.0

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

>>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:99
>>> +            frame = WI.networkManager.frames.find(frame => frame.url === frameURL);
>> 
>> Do we care about a situation where there's more than one `WI.Frame` with the same URL?  Is it an exact URL match, or just a prefix match (e.g. the actual URL can have a query parameter that the given `frameURL` doesn't have)?
>> 
>> Style: we always wrap arguments of arrow functions in `(` `)` even if there's only one
> 
> Two excellent questions for which I do not have an answer at this time. The documentation is unclear as to how strict the matching is and wether query parameters are part of that match. Along the same train of through, I should probably also be considering escaped vs. unescaped matching as well.

The API does not define whether this should be a prefix match only, or an exact match (nor do any existing tests for the API). In the absence of such rules, the best thing to do is probably something along the lines of...
if options.frameURL has query params
    only exactly match
else
    try to exactly match
    fall back to stripping query params for matching if there was no strict match
Comment 6 BJ Burg 2021-03-02 13:11:31 PST
(In reply to Patrick Angle from comment #5)
> Comment on attachment 421850 [details]
> Patch v1.0
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421850&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:99
> >>> +            frame = WI.networkManager.frames.find(frame => frame.url === frameURL);
> >> 
> >> Do we care about a situation where there's more than one `WI.Frame` with the same URL?  Is it an exact URL match, or just a prefix match (e.g. the actual URL can have a query parameter that the given `frameURL` doesn't have)?
> >> 
> >> Style: we always wrap arguments of arrow functions in `(` `)` even if there's only one
> > 
> > Two excellent questions for which I do not have an answer at this time. The documentation is unclear as to how strict the matching is and wether query parameters are part of that match. Along the same train of through, I should probably also be considering escaped vs. unescaped matching as well.
> 
> The API does not define whether this should be a prefix match only, or an
> exact match (nor do any existing tests for the API). In the absence of such
> rules, the best thing to do is probably something along the lines of...
> if options.frameURL has query params
>     only exactly match
> else
>     try to exactly match
>     fall back to stripping query params for matching if there was no strict
> match

Sounds reasonable!
Comment 7 Patrick Angle 2021-03-03 13:12:21 PST
Created attachment 422140 [details]
Patch v1.1 - Support fuzzier matching, fix eval error handling
Comment 8 BJ Burg 2021-03-03 16:21:28 PST
Comment on attachment 422140 [details]
Patch v1.1 - Support fuzzier matching, fix eval error handling

The code looks good, but I'd like to see a test to exercise the fuzzy/hairy frameURL matching code path.

This can be a new TestWebKitAPI file based on WKInspectorExtensionHost.mm. I think it's worth creating a separate file as the helper delegates will likely need to be different.
Comment 9 Devin Rousso 2021-03-03 16:22:38 PST
Comment on attachment 422140 [details]
Patch v1.1 - Support fuzzier matching, fix eval error handling

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:108
> +                        let knownFrameURLWithoutQueryParamsOrFragmentIdentifier = knownFrame.url.slice(0, -knownFrameURLParts.search.length - knownFrameURLParts.hash.length);

FYI since you've created a `URL` here, you could just `knownFrameURLParts.search = "";` and `knownFrameURLParts.hash = "";` and then compare `knownFrameURLParts.href === frameURL`.

I wanted to ask you to change from `new URL` to use `parseURL` (which is a WI utility), but if you do this then it's fine :)
Comment 10 Patrick Angle 2021-03-05 17:17:44 PST
Created attachment 422457 [details]
WIP v1.2 - Test now crashes...
Comment 11 Radar WebKit Bug Importer 2021-03-08 11:14:18 PST
<rdar://problem/75178876>
Comment 12 Patrick Angle 2021-12-20 13:52:44 PST
Created attachment 447633 [details]
Patch v2.0 - Rebased, now-functional test
Comment 13 Patrick Angle 2021-12-20 14:02:39 PST
Created attachment 447636 [details]
Patch v2.1 - Adjust function naming
Comment 14 Devin Rousso 2021-12-20 14:52:54 PST
Comment on attachment 447636 [details]
Patch v2.1 - Adjust function naming

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:138
> +            return {"error": `Unable to evaluate script in unknown frame with URL: ${frameURL}`};

This error is visible to the extension, right?  Is the error message standardized and/or does this match other browsers?

Style: the `"` are unnecessary for property keys that don't have whitespace :)

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:152
> +            WI.reportInternalError("evaluateScriptForExtension: No 'pageExecutionContext' was present for frame with URL: " + frame.url);

Is this actually an internal error?  Is it possible for an `<iframe>` to be created with some CSP/FeaturePolicy/etc. that disables JS?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:409
> +        if (frameURLParts.search.length || frameURLParts.hash.length)

I can understand not allowing matches based on the `hash`, but the `search` too?  That seems a bit restrictive given that lots of sites are implemented where the current "view" is based on the value of the `search`.

I wonder if we should just take the given `frameURL` as-is and just see if it exactly matches since that's what the extension explicitly asked for.  The only reasons I can think of not to do that is if the `url` of `WI.Frame` doesn't have a `search`/`hash` or if it doesn't match the behavior of other browsers.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:415
> +            if (!knownFrameURLParts.search.length && !knownFrameURLParts.hash.length)
> +                return false;

Why would we disqualify a `WI.Frame` if it's `url` doesn't have a `search` or `hash`?  Isn't that a "good" thing?

ditto (:409)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:398
> +    auto *testPagePath = [NSBundle.mainBundle pathForResource:@"WKInspectorExtensionEvaluateScriptOnPage" ofType:@"html" inDirectory:@"TestWebKitAPI.resources"];
> +    auto *testPageBaseURL = [NSURL fileURLWithPath:testPagePath.stringByDeletingLastPathComponent];
> +    auto *testPageFileURL = [NSURL fileURLWithPath:testPagePath.lastPathComponent relativeToURL:testPageBaseURL];

NIT: This seems quite excessive.  Can we use something like `-[NSBundle URLForResource:withExtension:subdirectory:]` instead?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:467
> +    [sharedInspectorExtension evaluateScript:@"goto fail" frameURL:nil contextSecurityOrigin:nil useContentScriptContext:false completionHandler:^(NSError *error, NSDictionary *result) {

NIT: Can we do a runtime failure instead?  Something like `[].x.x`?
Comment 15 Patrick Angle 2021-12-20 15:38:35 PST
Comment on attachment 447636 [details]
Patch v2.1 - Adjust function naming

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:152
>> +            WI.reportInternalError("evaluateScriptForExtension: No 'pageExecutionContext' was present for frame with URL: " + frame.url);
> 
> Is this actually an internal error?  Is it possible for an `<iframe>` to be created with some CSP/FeaturePolicy/etc. that disables JS?

CSP can indeed precent unsafe script evaluation on a page, but I've just tested this locally on a page with such a policy enforced and am still able to execute script from the Web Inspector console on the page. For FeaturePolicy, my understanding is that those APIs control what you can do in JavaScript, not wether JS is usable at all. Any error from running into a FeaturePolicy would therefor come from the :156-169 below as a result of execution.

However, in thinking through and testing this a bit further it is possible if a user has disabled JavaScript entirely that this will fail, so maybe it does need to be a non-internal error...

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:409
>> +        if (frameURLParts.search.length || frameURLParts.hash.length)
> 
> I can understand not allowing matches based on the `hash`, but the `search` too?  That seems a bit restrictive given that lots of sites are implemented where the current "view" is based on the value of the `search`.
> 
> I wonder if we should just take the given `frameURL` as-is and just see if it exactly matches since that's what the extension explicitly asked for.  The only reasons I can think of not to do that is if the `url` of `WI.Frame` doesn't have a `search`/`hash` or if it doesn't match the behavior of other browsers.

This behavior is based on the conversation from much earlier this year in comment #6. We start by attempting to strictly match the frame. (:404-406) Then, if the frameURL provided by the extension was not specific as to query params/hash we search more loosely for a matching frame (:412-420). This check exists because there is no need to spend the time comparing the provided frameURL against every known frame's URL sans `search` and `hash` because none of them will match under those conditions.

I defer to BJ who has been in the trenches on this for what behavior they think will be best here.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:415
>> +                return false;
> 
> Why would we disqualify a `WI.Frame` if it's `url` doesn't have a `search` or `hash`?  Isn't that a "good" thing?
> 
> ditto (:409)

Looking at this again, this is redundant anyways. A frame that hits this check will either not be a match anyways, or will have been returned as the match above on :406.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:398
>> +    auto *testPageFileURL = [NSURL fileURLWithPath:testPagePath.lastPathComponent relativeToURL:testPageBaseURL];
> 
> NIT: This seems quite excessive.  Can we use something like `-[NSBundle URLForResource:withExtension:subdirectory:]` instead?

Yeah, that works as well.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:467
>> +    [sharedInspectorExtension evaluateScript:@"goto fail" frameURL:nil contextSecurityOrigin:nil useContentScriptContext:false completionHandler:^(NSError *error, NSDictionary *result) {
> 
> NIT: Can we do a runtime failure instead?  Something like `[].x.x`?

Sure.
Comment 16 Devin Rousso 2021-12-20 15:44:36 PST
Comment on attachment 447636 [details]
Patch v2.1 - Adjust function naming

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

>>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:409
>>> +        if (frameURLParts.search.length || frameURLParts.hash.length)
>> 
>> I can understand not allowing matches based on the `hash`, but the `search` too?  That seems a bit restrictive given that lots of sites are implemented where the current "view" is based on the value of the `search`.
>> 
>> I wonder if we should just take the given `frameURL` as-is and just see if it exactly matches since that's what the extension explicitly asked for.  The only reasons I can think of not to do that is if the `url` of `WI.Frame` doesn't have a `search`/`hash` or if it doesn't match the behavior of other browsers.
> 
> This behavior is based on the conversation from much earlier this year in comment #6. We start by attempting to strictly match the frame. (:404-406) Then, if the frameURL provided by the extension was not specific as to query params/hash we search more loosely for a matching frame (:412-420). This check exists because there is no need to spend the time comparing the provided frameURL against every known frame's URL sans `search` and `hash` because none of them will match under those conditions.
> 
> I defer to BJ who has been in the trenches on this for what behavior they think will be best here.

Ah I missed :404 (pun unintended).  I think it might be worth doing this in steps, where we first remove the `hash` and `.find()` before then removing the `search` too.  Might be overkill, but I think we should handle `search` and `hash` separately.
Comment 17 Patrick Angle 2021-12-20 16:31:41 PST
Created attachment 447662 [details]
Patch v2.2 - Address review comments
Comment 18 Devin Rousso 2021-12-21 11:21:56 PST
Comment on attachment 447662 [details]
Patch v2.2 - Address review comments

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

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:406
> +        function findFrame(frameURL, adjustKnownFrameURL = null) {

Style: the ` = null` is unnecessary since `adjustKnownFrameURL` will default to `undefined` if not provided, which is handled in the same way by `?.` (we also prefer not providing default values for parameters)

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:419
> +        if (frameURLParts.search.length || frameURLParts.hash.length)

Hmm what if `frameURLParts` does have a `search` but not a `hash`?  Would we wanna try to remove the `hash` from `knownFrameURL` and see if we can match it then?
Comment 19 Patrick Angle 2021-12-21 13:56:35 PST
Created attachment 447751 [details]
Patch v2.3 - Review nits
Comment 20 BJ Burg 2022-01-12 15:09:49 PST
Created attachment 449002 [details]
Patch v2.4 - rebased for commit-queue
Comment 21 EWS 2022-01-12 15:26:12 PST
Tools/Scripts/svn-apply failed to apply attachment 449002 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 22 Patrick Angle 2022-01-13 07:48:14 PST
Created attachment 449059 [details]
Patch v2.5 - Rebase for commit-queue
Comment 23 EWS 2022-01-13 08:23:21 PST
Committed r287979 (246008@main): <https://commits.webkit.org/246008@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449059 [details].