Bug 230646 – [Cocoa] add _WKInspectorExtension SPI to evaluate script on an extension tab

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

Needed to write tests for some bugs.

[Radar WebKit Bug Importer]

<rdar://problem/83420328>

[Blaze Burg]

Created attachment 439171 [details]
Patch v1.0

[Blaze Burg]

Created attachment 439187 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 439187 [details]
Patch v1.0

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

r=me, neat stuff! =D

> Source/WebCore/inspector/InspectorFrontendHost.cpp:714
> +    if (!is<HTMLIFrameElement>(extensionFrame))

Instead of doing this check manually, why not just make the `extensionFrame` parameter be a `HTMLIFrameElement?
```
    [Conditional=INSPECTOR_EXTENSIONS] any evaluateScriptInExtensionTab(HTMLIFrameElement extensionFrame, DOMString scriptSource);
```

> Source/WebCore/inspector/InspectorFrontendHost.cpp:718
> +    Frame* contentFrame = extensionFrameElement.contentFrame();

Does this need a `RefPtr`?  Is it possible for the evaluated script to remove the frame?

> Source/WebCore/inspector/InspectorFrontendHost.cpp:727
> +    ValueOrException result =  contentFrame->script().evaluateInWorld(ScriptSourceCode(scriptSource), mainThreadNormalWorld());

Style: double space

Why not just use `auto` instead of adding the `using` above?

> Source/WebCore/inspector/InspectorFrontendHost.cpp:732
> +    return ExceptionOr<JSC::JSValue>(WTFMove(result.value()));

Is the `ExceptionOr<JSC::JSValue>` cast actually needed?  Isn't `result.value()` already a `JSC::JSValue`?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:182
> +        let iframe = tabContentView.iframeElement;

NIT: this kinda somewhat feels almost like a layering violation in that you're exposing internal details of the `WI.WebInspectorExtensionTabContentView`, but I suppose it's preferable to having the `InspectorFrontendHost` call here instead of inside view code :/

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:189
> +            let result = InspectorFrontendHost.evaluateScriptInExtensionTab(iframe, scriptSource);

Style: i'd inline this

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:192
> +            return {"error": error.message};

Style: you can omit the quotes around the key

> Source/WebInspectorUI/UserInterface/Main.html:30
> +        Note that some WebKit ports may set custom 'frame-src', 'img-src', and 'connect-src' directives via HTTP response header.

Do we need to file a bug on other ports about this?

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:41
>          this._sourceURL = sourceURL;

Do we actually need to save the `sourceURL` to a member property anymore?

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:61
> +    get iframeElement()

Style: you can make this into a single line getter up above

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionTesting.mm:48
> +            capturedBlock([NSError errorWithDomain:WKErrorDomain code:WKErrorUnknown userInfo:@{ NSLocalizedFailureReasonErrorKey: Inspector::extensionErrorToString(result.error())}], nil);

Style: missing space before `}`

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionTesting.mm:58
> +        id body = API::SerializedScriptValue::deserialize(valueOrException.value()->internalRepresentation(), 0);

NIT: I'd inline this

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:212
> +        weakThis->m_inspectorPage->sendWithAsyncReply(Messages::WebInspectorUIExtensionController::EvaluateScriptInExtensionTab {extensionTabID, scriptSource}, [completionHandler = WTFMove(completionHandler)](const IPC::DataReference& dataReference, const std::optional<WebCore::ExceptionDetails>& details, const std::optional<Inspector::ExtensionError> error) mutable {

missing `&` for `error`

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:219
> +                Expected<RefPtr<API::SerializedScriptValue>, WebCore::ExceptionDetails> returnedValue = makeUnexpected(details.value());

NIT: I'd just inline this

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:223
> +            completionHandler({ { API::SerializedScriptValue::adopt(Vector { dataReference.data(), dataReference.size() }).ptr() } });

NIT: Is the `Vector` necessary?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:382
> +        auto* frontendGlobalObject = weakThis->m_frontendClient->frontendAPIDispatcher().frontendGlobalObject();

NIT: Should we `Ref strongThis = *weakThis.get();` somewhere so that we don't keep having to deref?  Or perhaps also capture `this` and completely drop `weakThis->`?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:406
> +        ASSERT(result.has_value());

Why?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:420
> +        RefPtr<SerializedScriptValue> serializedResultValue = SerializedScriptValue::create(*frontendGlobalObject, resultPayload);

`auto`?

> Tools/ChangeLog:13
> +        This is a bug and will be addressed as part of https://bugs.webkit.org/show_bug.cgi?id=230758.

I'd maybe add this as a FIXME comment somewhere inside `WI.WebInspectorExtensionTabContentView` too

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:158
> +    auto scriptSource1 = @"window._secretValue = {'answer':42}";

you dont need `'` for simple object keys :)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:166
> +    auto scriptSource2 = @"window._secretValue";

Could we also do another test just to make sure that we're indeed actually evaluating inside `InspectorExtension-basic-tab.html` (e.g. `window.top !== window`)?  Maybe instead of setting `window._secretValue` you could have that HTML file set it and then retrieve it in this test so that you're actually verifying it comes from _that_ file.

[Patrick Angle]

Comment on attachment 439187 [details]
Patch v1.0

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

> Source/WebKit/ChangeLog:18
> +        Along the way, tweak Web Inspector's CSP policy to allow loading images from
> +        custom URL schemes as specified using _WKInspectorConfiguration.

Is this so we can load the tab icons into the main frame even under testing situations? Might be nice to give that example here.

> Source/WebKit/ChangeLog:27
> +        Cribbed from evaluateScript(). Call through to the shared extension controller.

Aside: I don't think it can be called cribbing when you clearly attribute that fact.

> Source/WebKit/UIProcess/API/APIInspectorExtension.h:52
> -    void reloadIgnoringCache(const std::optional<bool>& ignoreCache, const std::optional<WTF::String>& userAgent, const std::optional<WTF::String>& injectedScript,  WTF::CompletionHandler<void(Inspector::ExtensionEvaluationResult)>&&);
> +    void reloadIgnoringCache(const std::optional<bool>& ignoreCache, const std::optional<WTF::String>& userAgent, const std::optional<WTF::String>& injectedScript, WTF::CompletionHandler<void(Inspector::ExtensionEvaluationResult)>&&);

Huh?

> Tools/TestWebKitAPI/cocoa/TestInspectorURLSchemeHandler.mm:95
> +        [urlSchemeTask didReceiveResponse:urlResponse.get()];
> +        [urlSchemeTask didReceiveData:fileData];
> +        [urlSchemeTask didFinish];

FYI I think this will suffer from rdar://82814846 which hasn't been fixed yet for InspectorResourceURLSchemeHandler. Probably won't run into it unless you you plan on starting and stopping these tasks quickly in your test, but just an FYI. I can add a note to implement the fix here as well when I get to it.

[Blaze Burg]

(In reply to Patrick Angle from comment #5)
> Comment on attachment 439187 [details]
> Patch v1.0
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439187&action=review
> 
> > Source/WebKit/ChangeLog:18
> > +        Along the way, tweak Web Inspector's CSP policy to allow loading images from
> > +        custom URL schemes as specified using _WKInspectorConfiguration.
> 
> Is this so we can load the tab icons into the main frame even under testing
> situations? Might be nice to give that example here.

Yep, good idea.

> > Source/WebKit/ChangeLog:27
> > +        Cribbed from evaluateScript(). Call through to the shared extension controller.
> 
> Aside: I don't think it can be called cribbing when you clearly attribute
> that fact.

LOL, point taken.

> 
> > Source/WebKit/UIProcess/API/APIInspectorExtension.h:52
> > -    void reloadIgnoringCache(const std::optional<bool>& ignoreCache, const std::optional<WTF::String>& userAgent, const std::optional<WTF::String>& injectedScript,  WTF::CompletionHandler<void(Inspector::ExtensionEvaluationResult)>&&);
> > +    void reloadIgnoringCache(const std::optional<bool>& ignoreCache, const std::optional<WTF::String>& userAgent, const std::optional<WTF::String>& injectedScript, WTF::CompletionHandler<void(Inspector::ExtensionEvaluationResult)>&&);
> 
> Huh?

I think there was a trailing space or something. ¯\_(ツ)_/¯ 

> > Tools/TestWebKitAPI/cocoa/TestInspectorURLSchemeHandler.mm:95
> > +        [urlSchemeTask didReceiveResponse:urlResponse.get()];
> > +        [urlSchemeTask didReceiveData:fileData];
> > +        [urlSchemeTask didFinish];
> 
> FYI I think this will suffer from rdar://82814846 which hasn't been fixed
> yet for InspectorResourceURLSchemeHandler. Probably won't run into it unless
> you you plan on starting and stopping these tasks quickly in your test, but
> just an FYI. I can add a note to implement the fix here as well when I get
> to it.

Thanks for mentioning it. It indeed seems unlikely in an API test, but reducing the potential for flakiness is worth porting the fix to this simpler class.

[Blaze Burg]

Comment on attachment 439187 [details]
Patch v1.0

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

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:714
>> +    if (!is<HTMLIFrameElement>(extensionFrame))
> 
> Instead of doing this check manually, why not just make the `extensionFrame` parameter be a `HTMLIFrameElement?
> ```
>     [Conditional=INSPECTOR_EXTENSIONS] any evaluateScriptInExtensionTab(HTMLIFrameElement extensionFrame, DOMString scriptSource);
> ```

Sounds reasonable. I was hesitant at first to use more IDL magic since I had quite a hard time getting the result-wrapping code to work.

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:718
>> +    Frame* contentFrame = extensionFrameElement.contentFrame();
> 
> Does this need a `RefPtr`?  Is it possible for the evaluated script to remove the frame?

Good catch! I pasted the two uses of `contentFrame` into the function separately, and didn't realize the second use is unguarded.

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:727
>> +    ValueOrException result =  contentFrame->script().evaluateInWorld(ScriptSourceCode(scriptSource), mainThreadNormalWorld());
> 
> Style: double space
> 
> Why not just use `auto` instead of adding the `using` above?

I found ValueOrException to be helpful in remembering what the type of `result` actually is . The top-level result from the InspectorFrontendAPIDispatcher method is an Expected<Expected<>>, so we have to consider v.error(), v.result().result(), and v.result.error(). But here, result is equivalent to v.result().

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:732
>> +    return ExceptionOr<JSC::JSValue>(WTFMove(result.value()));
> 
> Is the `ExceptionOr<JSC::JSValue>` cast actually needed?  Isn't `result.value()` already a `JSC::JSValue`?

I'll try again, but my recollection is that this did not compile without the explicit constructor call.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:182
>> +        let iframe = tabContentView.iframeElement;
> 
> NIT: this kinda somewhat feels almost like a layering violation in that you're exposing internal details of the `WI.WebInspectorExtensionTabContentView`, but I suppose it's preferable to having the `InspectorFrontendHost` call here instead of inside view code :/

This controller creates the extension tab content views in the first place, so it seems fine to me.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:189
>> +            let result = InspectorFrontendHost.evaluateScriptInExtensionTab(iframe, scriptSource);
> 
> Style: i'd inline this

OK

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:192
>> +            return {"error": error.message};
> 
> Style: you can omit the quotes around the key

OK

>> Source/WebInspectorUI/UserInterface/Main.html:30
>> +        Note that some WebKit ports may set custom 'frame-src', 'img-src', and 'connect-src' directives via HTTP response header.
> 
> Do we need to file a bug on other ports about this?

I don't recall what happened the last time I relaxed the base CSP policy. Do other ports care about this? The only complication I'd imagine is that maybe some tests depend on loading images from file: protocol.

>> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:41
>>          this._sourceURL = sourceURL;
> 
> Do we actually need to save the `sourceURL` to a member property anymore?

I suppose in isolation it's not strictly necessary, but it's critical to debugging situations with multiple extension tabs.

The ultimate bug that I'm trying to fix is to make the iframe persistent. I am not sure if it is correct to create the iframe in the constructor.

>> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:61
>> +    get iframeElement()
> 
> Style: you can make this into a single line getter up above

ok

[Blaze Burg]

Created attachment 439507 [details]
Patch v2.0

New patch addresses all outstanding feedback.

[Devin Rousso]

Comment on attachment 439507 [details]
Patch v2.0

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

> Source/WebCore/inspector/InspectorFrontendHost.cpp:718
> +    Ref<Frame> protectedFrame(*frame);

I think you should `RefPtr` above to avoid any TOC/TOU issues

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:246
>      }

Style: missing trailing `,`

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:123
> +        let shouldSaveTab = this.selectedTabContentView?.constructor.shouldSaveTab() || this.selectedTabContentView?.constructor.shouldPinTab();

NIT: it'd be nice to avoid doing this outside the `console.assert` so that production builds (where `console.assert` are stripped) don't have this unnecessarily

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:47
> +        this._iframeElement.onload = () => this._extensionFrameDidLoad();

Style: `this._iframeElement.addEventListener("load", this._handleExtensionFrameLoad.bind(this));`

[Blaze Burg]

Created attachment 439523 [details]
Patch v2.0.1 (tvOS build fix)

[Blaze Burg]

Created attachment 439628 [details]
Patch v2.0.2 (iOS build fix)

[Blaze Burg]

(In reply to BJ Burg from comment #11)
> Created attachment 439628 [details]
> Patch v2.0.2 (iOS build fix)

Remaining EWS failures are `api-ios`. These look unrelated, so proceeding to cq+.


>    TestWebKitAPI.WebKitLegacy.AudioSessionCategoryIOS
>        2021-09-29 14:35:27.776 TestWebKitAPI[55097:7593733] nil host used in call to allowsSpecificHTTPSCertificateForHost
>        2021-09-29 14:35:27.776 TestWebKitAPI[55097:7593733] nil host used in call to allowsAnyHTTPSCertificateForHost:
>    TestWebKitAPI.WebKitLegacy.PreemptVideoFullscreen
>        2021-09-29 14:36:04.737 TestWebKitAPI[55127:7597038] nil host used in call to allowsSpecificHTTPSCertificateForHost
>        2021-09-29 14:36:04.737 TestWebKitAPI[55127:7597038] nil host used in call to allowsAnyHTTPSCertificateForHost:
>    TestWebKitAPI.WebKitLegacy.ScrollingDoesNotPauseMedia
>        2021-09-29 14:36:39.243 TestWebKitAPI[55139:7598658] nil host used in call to allowsSpecificHTTPSCertificateForHost
>        2021-09-29 14:36:39.243 TestWebKitAPI[55139:7598658] nil host used in call to allowsAnyHTTPSCertificateForHost:

[EWS]

Committed r283276 (242305@main): <https://commits.webkit.org/242305@main>

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