Bug 211904

Summary: Add sourceURL to _evaluateJavaScript: so the scripts appear in Web Inspector
Product: WebKit Reporter: Timothy Hatcher <timothy>
Component: WebKit APIAssignee: Timothy Hatcher <timothy>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, berto, cgarcia, ews-watchlist, gustavo, hi, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Timothy Hatcher 2020-05-14 10:06:01 PDT
We were missing the URL, using the frame's document URL instead of the original source URL. This caused two issues, one the error messages were in the wrong location when shown in the console. And the script would not appear in the Sources tab of the Inspector.
Comment 1 Timothy Hatcher 2020-05-14 10:06:07 PDT
<rdar://problem/62074376>
Comment 2 Timothy Hatcher 2020-05-14 11:28:46 PDT Comment hidden (obsolete)
Comment 3 Timothy Hatcher 2020-05-14 11:49:46 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2020-05-14 11:50:36 PDT Comment hidden (obsolete)
Comment 5 Devin Rousso 2020-05-14 11:55:02 PDT
Comment on attachment 399388 [details]
Patch

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

r=me, but I think you missed some cases in GLib code

> Source/WebCore/bindings/js/RunJavaScriptParameters.h:50
> +    RunJavaScriptParameters(const String& source, const URL& sourceURL, bool runAsAsyncFunction, Optional<ArgumentWireBytesMap>&& arguments, bool forceUserGesture)

I feel like we should just always use `URL&&`.

> Source/WebCore/bindings/js/ScriptController.cpp:594
> +    URL sourceURL = parameters.sourceURL;

Do we need to pull this out into a local or can we just inline it?

NIT: `auto`?

> Source/WebCore/bindings/js/ScriptController.cpp:645
> +    URL sourceURL = parameters.sourceURL;
> +    String sourceURLString = sourceURL.string();

NIT: `auto`?

> Source/WebCore/bindings/js/ScriptController.cpp:-645
> -    String sourceURL = jsSourceCode.provider()->url().string();

I think we can/should keep this logic so that we get the resolved `sourceURL` from the script itself, rather than what was passed in (just in case we add some logic later that mucks with it).
```
    String sourceURLString = jsSourceCode.provider()->url().string();
```

> Source/WebKit/UIProcess/API/Cocoa/WKUserScript.mm:-39
> -    API::Object::constructInWrapper<API::UserScript>(self, WebCore::UserScript { WTF::String(source), API::UserScript::generateUniqueURL(), { }, { }, API::toWebCoreUserScriptInjectionTime(injectionTime), forMainFrameOnly ? WebCore::UserContentInjectedFrames::InjectInTopFrameOnly : WebCore::UserContentInjectedFrames::InjectInAllFrames, WebCore::WaitForNotificationBeforeInjecting::No }, API::ContentWorld::pageContentWorld());

I do like these changes, but are they strictly speaking necessary?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:909
> +    URL sourceURL = url;
> +    if (!sourceURL.isValid())
> +        sourceURL = API::UserScript::generateUniqueURL();

Should we only do this if `url` is actually provided?  Otherwise, we'd end up generating a `sourceURL` (which is really only used for Web Inspector) for _every_ evaluation 🤔
Comment 6 Timothy Hatcher 2020-05-14 12:16:57 PDT
Comment on attachment 399388 [details]
Patch

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

>> Source/WebCore/bindings/js/RunJavaScriptParameters.h:50
>> +    RunJavaScriptParameters(const String& source, const URL& sourceURL, bool runAsAsyncFunction, Optional<ArgumentWireBytesMap>&& arguments, bool forceUserGesture)
> 
> I feel like we should just always use `URL&&`.

Okay.

>> Source/WebCore/bindings/js/ScriptController.cpp:594
>> +    URL sourceURL = parameters.sourceURL;
> 
> Do we need to pull this out into a local or can we just inline it?
> 
> NIT: `auto`?

Will fix.

>> Source/WebCore/bindings/js/ScriptController.cpp:-645
>> -    String sourceURL = jsSourceCode.provider()->url().string();
> 
> I think we can/should keep this logic so that we get the resolved `sourceURL` from the script itself, rather than what was passed in (just in case we add some logic later that mucks with it).
> ```
>     String sourceURLString = jsSourceCode.provider()->url().string();
> ```

Okay

>> Source/WebKit/UIProcess/API/Cocoa/WKUserScript.mm:-39
>> -    API::Object::constructInWrapper<API::UserScript>(self, WebCore::UserScript { WTF::String(source), API::UserScript::generateUniqueURL(), { }, { }, API::toWebCoreUserScriptInjectionTime(injectionTime), forMainFrameOnly ? WebCore::UserContentInjectedFrames::InjectInTopFrameOnly : WebCore::UserContentInjectedFrames::InjectInAllFrames, WebCore::WaitForNotificationBeforeInjecting::No }, API::ContentWorld::pageContentWorld());
> 
> I do like these changes, but are they strictly speaking necessary?

I wanted to clean it up, was bothering me.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:909
>> +        sourceURL = API::UserScript::generateUniqueURL();
> 
> Should we only do this if `url` is actually provided?  Otherwise, we'd end up generating a `sourceURL` (which is really only used for Web Inspector) for _every_ evaluation 🤔

Yeah, I'll drop this. Good call.
Comment 7 Timothy Hatcher 2020-05-14 12:27:39 PDT Comment hidden (obsolete)
Comment 8 Timothy Hatcher 2020-05-14 13:11:14 PDT Comment hidden (obsolete)
Comment 9 Timothy Hatcher 2020-05-14 13:14:09 PDT Comment hidden (obsolete)
Comment 10 Devin Rousso 2020-05-14 13:25:27 PDT
Comment on attachment 399397 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptController.cpp:644
> +    auto sourceURL = parameters.sourceURL;
> +    if (!sourceURL.isValid()) {
> +        // FIXME: This is gross, but when setTimeout() and setInterval() are passed JS strings, the thrown errors should use the frame document URL (according to WPT).
> +        sourceURL = m_frame.document()->url();
> +    }

I think this needs to be in `ScriptController::executeScriptInWorld` right above the `evaluateInWorld` call, _not_ in `ScriptController::callInWorld` (which was added for `-[WKWebView callAsyncJavaScript:arguments:inContentWorld:completionHandler]`).
Comment 11 Timothy Hatcher 2020-05-14 13:30:37 PDT
Created attachment 399400 [details]
Patch
Comment 12 EWS 2020-05-14 16:11:57 PDT
Committed r261718: <https://trac.webkit.org/changeset/261718>

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