.
Created attachment 421850 [details] Patch v1.0
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 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 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 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
(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!
Created attachment 422140 [details] Patch v1.1 - Support fuzzier matching, fix eval error handling
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 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 :)
Created attachment 422457 [details] WIP v1.2 - Test now crashes...
<rdar://problem/75178876>
Created attachment 447633 [details] Patch v2.0 - Rebased, now-functional test
Created attachment 447636 [details] Patch v2.1 - Adjust function naming
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 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 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.
Created attachment 447662 [details] Patch v2.2 - Address review comments
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?
Created attachment 447751 [details] Patch v2.3 - Review nits
Created attachment 449002 [details] Patch v2.4 - rebased for commit-queue
Tools/Scripts/svn-apply failed to apply attachment 449002 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 449059 [details] Patch v2.5 - Rebase for commit-queue
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].