Bug 205294 – import.meta.url: baseURL for a module script should be response URL, not request URL

• Component: New Bugs
• Status: RESOLVED FIXED

[Justin Fagnani]

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`.

[Radar WebKit Bug Importer]

<rdar://problem/57982755>

[Yusuke Suzuki]

Created attachment 396359 [details]
Patch

[Yusuke Suzuki]

Created attachment 396360 [details]
Patch

[Yusuke Suzuki]

Created attachment 396364 [details]
Patch

[Yusuke Suzuki]

Created attachment 396367 [details]
Patch

[Yusuke Suzuki]

Created attachment 396370 [details]
Patch

[Yusuke Suzuki]

Created attachment 396371 [details]
Patch

[youenn fablet]

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?

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Looks like, AppleWin networking implementation has different level of support of redirections, and causing failure, I'll check the test.

[Yusuke Suzuki]

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 :)

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 396402 [details]
Patch

[Yusuke Suzuki]

Created attachment 396405 [details]
Patch

[Yusuke Suzuki]

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.

[youenn fablet]

(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.

[youenn fablet]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 396415 [details]
Patch

[Yusuke Suzuki]

Created attachment 396416 [details]
Patch

[youenn fablet]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r260131: <https://trac.webkit.org/changeset/260131>