Bug 211904 - Add sourceURL to _evaluateJavaScript: so the scripts appear in Web Inspector
Summary: Add sourceURL to _evaluateJavaScript: so the scripts appear in Web Inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-14 10:06 PDT by Timothy Hatcher
Modified: 2020-05-14 16:11 PDT (History)
8 users (show)

See Also:


Attachments
Patch (27.16 KB, patch)
2020-05-14 11:28 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (30.08 KB, patch)
2020-05-14 11:49 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (30.34 KB, patch)
2020-05-14 12:27 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (30.62 KB, patch)
2020-05-14 13:11 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (31.19 KB, patch)
2020-05-14 13:14 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (30.73 KB, patch)
2020-05-14 13:30 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].