Bug 205294 - import.meta.url: baseURL for a module script should be response URL, not request URL
Summary: import.meta.url: baseURL for a module script should be response URL, not requ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari 13
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 210490
  Show dependency treegraph
 
Reported: 2019-12-16 12:30 PST by Justin Fagnani
Modified: 2020-04-15 08:48 PDT (History)
8 users (show)

See Also:


Attachments
Patch (12.30 KB, patch)
2020-04-13 17:02 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.27 KB, patch)
2020-04-13 17:05 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.73 KB, patch)
2020-04-13 17:44 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.56 KB, patch)
2020-04-13 18:18 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.87 KB, patch)
2020-04-13 18:52 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (14.54 KB, patch)
2020-04-13 19:12 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (20.06 KB, patch)
2020-04-14 05:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (22.20 KB, patch)
2020-04-14 05:57 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.46 KB, patch)
2020-04-14 07:29 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.45 KB, patch)
2020-04-14 07:32 PDT, Yusuke Suzuki
youennf: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fagnani 2019-12-16 12:30:09 PST
According to: https://html.spec.whatwg.org/multipage/webappapis.html#fetching-scripts:creating-a-module-script-2

The base URL for a module script should be the response URL, post any redirects. Currently Webkit is using the request URL.

This causes significant problems (and incompatibility with Firefox and Chrome) when there are redirects in the response for a module.

Imagine you are using something like `new URL('./foo.jpg', import.meta.url).href` to get the URL for an image relative to the main module of an NPM package. A CDN like unpkg.com will perform a redirect from `unpkg.com/foo` to the main module, ie: `unpkg.com/foo/index.js`. This causes there to be a difference in the image's URL depending on whether `import.meta.url` uses response or request URL. Response URL will give `unpkg.com/foo/foo.jpg` while request URL will give `unpkg.com/foo.jpg`.
Comment 1 Radar WebKit Bug Importer 2019-12-16 13:24:53 PST
<rdar://problem/57982755>
Comment 2 Yusuke Suzuki 2020-04-13 17:02:37 PDT
Created attachment 396359 [details]
Patch
Comment 3 Yusuke Suzuki 2020-04-13 17:05:01 PDT
Created attachment 396360 [details]
Patch
Comment 4 Yusuke Suzuki 2020-04-13 17:44:08 PDT
Created attachment 396364 [details]
Patch
Comment 5 Yusuke Suzuki 2020-04-13 18:18:19 PDT
Created attachment 396367 [details]
Patch
Comment 6 Yusuke Suzuki 2020-04-13 18:52:54 PDT
Created attachment 396370 [details]
Patch
Comment 7 Yusuke Suzuki 2020-04-13 19:12:54 PDT
Created attachment 396371 [details]
Patch
Comment 8 youenn fablet 2020-04-14 00:55:07 PDT
Comment on attachment 396371 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:276
> +        URL moduleRequestURL(URL(), asString(moduleKeyValue)->value(jsGlobalObject));

We parse the URL each time we query the map.
Does it make sense to have m_requestURLToResponseURLMap be a HashMap<String, URL>?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:334
> +    // FIXME: We should track fragments though redirections.

s/though/through/

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:338
> +            responseURL.setFragmentIdentifier(sourceURL.fragmentIdentifier());

In case of a response coming from a service worker, we should probably leave responseURL untouched.
Might be good to add a test.

> LayoutTests/ChangeLog:14
> +        * http/tests/misc/resources/import-meta-url-expose.js: Added.

Should they be WPT tests?
Comment 9 Yusuke Suzuki 2020-04-14 04:04:07 PDT
Comment on attachment 396371 [details]
Patch

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

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:276
>> +        URL moduleRequestURL(URL(), asString(moduleKeyValue)->value(jsGlobalObject));
> 
> We parse the URL each time we query the map.
> Does it make sense to have m_requestURLToResponseURLMap be a HashMap<String, URL>?

I think it makes sense. Nice, fixed.

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:334
>> +    // FIXME: We should track fragments though redirections.
> 
> s/though/through/

Fixed, thanks!

>> LayoutTests/ChangeLog:14
>> +        * http/tests/misc/resources/import-meta-url-expose.js: Added.
> 
> Should they be WPT tests?

Sounds nice!
Comment 10 Yusuke Suzuki 2020-04-14 04:05:23 PDT
Looks like, AppleWin networking implementation has different level of support of redirections, and causing failure, I'll check the test.
Comment 11 Yusuke Suzuki 2020-04-14 04:18:15 PDT
Comment on attachment 396371 [details]
Patch

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

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:338
>> +            responseURL.setFragmentIdentifier(sourceURL.fragmentIdentifier());
> 
> In case of a response coming from a service worker, we should probably leave responseURL untouched.
> Might be good to add a test.

Currently, module-loader is not supported in worker environment. (e.g. ES6 modules cannot be usable in workers.), so I think we do not take this case.
It would be nice to have FIXME here about service worker. Added :)
Comment 12 Yusuke Suzuki 2020-04-14 04:18:44 PDT
Comment on attachment 396371 [details]
Patch

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

>>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:338
>>> +            responseURL.setFragmentIdentifier(sourceURL.fragmentIdentifier());
>> 
>> In case of a response coming from a service worker, we should probably leave responseURL untouched.
>> Might be good to add a test.
> 
> Currently, module-loader is not supported in worker environment. (e.g. ES6 modules cannot be usable in workers.), so I think we do not take this case.
> It would be nice to have FIXME here about service worker. Added :)

Ah, no. Yeah, service worker can interrupt it.
Comment 13 Yusuke Suzuki 2020-04-14 05:17:29 PDT
Created attachment 396402 [details]
Patch
Comment 14 Yusuke Suzuki 2020-04-14 05:57:09 PDT
Created attachment 396405 [details]
Patch
Comment 15 Yusuke Suzuki 2020-04-14 06:03:19 PDT
Comment on attachment 396405 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:342
> +    if (!cachedScript.hasRedirections() && cachedScript.response().source() != ResourceResponse::Source::ServiceWorker) {

@Youenn For service-worker thing, can you show me a pointer to an existing WPT SW test which is using SW results? I'm new to SW, and I would like to create a test based on that.
Comment 16 youenn fablet 2020-04-14 06:08:22 PDT
(In reply to Yusuke Suzuki from comment #15)
> Comment on attachment 396405 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396405&action=review
> 
> > Source/WebCore/bindings/js/ScriptModuleLoader.cpp:342
> > +    if (!cachedScript.hasRedirections() && cachedScript.response().source() != ResourceResponse::Source::ServiceWorker) {
> 
> @Youenn For service-worker thing, can you show me a pointer to an existing
> WPT SW test which is using SW results? I'm new to SW, and I would like to
> create a test based on that.

Sure, you can look at existing LayoutTests like LayoutTests/http/wpt/service-workers/navigation-redirect.https.html.
Typically, a test will:
- register a service worker
- wait for it to be activated
- load an iframe that matches the scope of the service worker so that all loads of the iframe go through the service worker
- trigger a module script load that the service worker will intercept based on the fetch event handler.

WPT tests are also available at imported/w3c/web-platform-tests/service-workers.
Ping me and I can help writing the test if needed.
Comment 17 youenn fablet 2020-04-14 06:10:15 PDT
Comment on attachment 396405 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:289
> +    ASSERT(responseURL.isValid());

If we would move the if check in responseURLFromRequestURL, we could do an early return.
That might improve readability.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:340
> +    // FIXME: We should track fragments through redirections.

Let's file a bug.
Comment 18 Yusuke Suzuki 2020-04-14 06:27:36 PDT
Comment on attachment 396405 [details]
Patch

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

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:289
>> +    ASSERT(responseURL.isValid());
> 
> If we would move the if check in responseURLFromRequestURL, we could do an early return.
> That might improve readability.

Because of the difference between `m_document.url()` / `m_document.baseURL()`, I've left `if` in the caller side.
But seems like always using `baseURL` is correct: https://html.spec.whatwg.org/multipage/webappapis.html#hostgetimportmetaproperties
Will fix, thanks :)

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:340
>> +    // FIXME: We should track fragments through redirections.
> 
> Let's file a bug.

While this issue is tracked by https://bugs.webkit.org/show_bug.cgi?id=158420, I've filed another bug to ensure this behavior particularly in module-loader in https://bugs.webkit.org/show_bug.cgi?id=210490

>>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:342
>>> +    if (!cachedScript.hasRedirections() && cachedScript.response().source() != ResourceResponse::Source::ServiceWorker) {
>> 
>> @Youenn For service-worker thing, can you show me a pointer to an existing WPT SW test which is using SW results? I'm new to SW, and I would like to create a test based on that.
> 
> Sure, you can look at existing LayoutTests like LayoutTests/http/wpt/service-workers/navigation-redirect.https.html.
> Typically, a test will:
> - register a service worker
> - wait for it to be activated
> - load an iframe that matches the scope of the service worker so that all loads of the iframe go through the service worker
> - trigger a module script load that the service worker will intercept based on the fetch event handler.
> 
> WPT tests are also available at imported/w3c/web-platform-tests/service-workers.
> Ping me and I can help writing the test if needed.

Nice, I'll try.
Comment 19 Yusuke Suzuki 2020-04-14 07:29:05 PDT
Created attachment 396415 [details]
Patch
Comment 20 Yusuke Suzuki 2020-04-14 07:32:04 PDT
Created attachment 396416 [details]
Patch
Comment 21 youenn fablet 2020-04-15 07:12:46 PDT
Comment on attachment 396416 [details]
Patch

LGTM!
Some nits below.

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

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:109
> +    ASSERT(baseURL.isValid());

Maybe ASSERT that the url is valid in ScriptModuleLoader::responseURLFromRequestURL or in resolveModuleSpecifier

> LayoutTests/http/wpt/html/semantics/scripting-1/the-script-element/module/module-meta-url-redirect.html:11
> +let basename = `http://${document.location.host}/WebKit/html/semantics/scripting-1/the-script-element/module`;

Could use const here and elsewhere.

> LayoutTests/http/wpt/html/semantics/scripting-1/the-script-element/module/module-meta-url-redirect.html:13
> +    return new Promise(function (resolve, reject) {

You use (resolve, reject) => elsewhere.
It is apparently good style to limit the code executed in the function(resolve, reject) as well.
Comment 22 Yusuke Suzuki 2020-04-15 08:27:53 PDT
Comment on attachment 396416 [details]
Patch

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

Thanks!

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:109
>> +    ASSERT(baseURL.isValid());
> 
> Maybe ASSERT that the url is valid in ScriptModuleLoader::responseURLFromRequestURL or in resolveModuleSpecifier

Fixed.

>> LayoutTests/http/wpt/html/semantics/scripting-1/the-script-element/module/module-meta-url-redirect.html:11
>> +let basename = `http://${document.location.host}/WebKit/html/semantics/scripting-1/the-script-element/module`;
> 
> Could use const here and elsewhere.

Fixed.

>> LayoutTests/http/wpt/html/semantics/scripting-1/the-script-element/module/module-meta-url-redirect.html:13
>> +    return new Promise(function (resolve, reject) {
> 
> You use (resolve, reject) => elsewhere.
> It is apparently good style to limit the code executed in the function(resolve, reject) as well.

Fixed.
Comment 23 Yusuke Suzuki 2020-04-15 08:48:20 PDT
Committed r260131: <https://trac.webkit.org/changeset/260131>