Bug 177959 – Support integrity="" on module scripts

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Domenic Denicola]

This was recently added to the spec in https://github.com/whatwg/html/commit/9275d955dcd604e959cfcc672e0c234b1b8c00db . It applies to the top-level fetch only.

Spec: https://html.spec.whatwg.org/#prepare-a-script step 18 and step 22.6.

Test: http://w3c-test.org/html/semantics/scripting-1/the-script-element/module/integrity.html

[Radar WebKit Bug Importer]

<rdar://problem/34892850>

[Yusuke Suzuki]

Created attachment 323420 [details]
Patch

WIP: Works in GTK with WPT. Need to add files to Xcode project file.

[Yusuke Suzuki]

Created attachment 323439 [details]
Patch

[Sam Weinig]

Comment on attachment 323439 [details]
Patch

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

Thanks so much for tackling this!

The high level approach I was thinking of taking was to to pass integrity down from LoadableModuleScript -> LoadableScript -> ScriptElementCachedScriptFetcher -> CachedScriptFetcher (and modify LoadableClassicScript to also pass it down). Then in ScriptModuleLoader::notifyFinished, get the integrity off the CachedScriptFetcher which is already available from the CachedModuleScriptLoader.

What is the benefit of adding the new 'parameters' object in addition to the existing 'scriptFetcher'?

> Source/JavaScriptCore/ChangeLog:21
> +        can use thsi parameters to fetch modules.

thsi -> this.

[Yusuke Suzuki]

Comment on attachment 323439 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:18
> +        We separately pass this parameters to the pipeline along with the script fetcher.
> +        The script fetcher is only one for module graph since this is the initiator of
> +        this module graph loading. On the other hand, this parameters is for each
> +        module fetching.

Yeah, that's super good question!
We add `parameters` in addition to script fetcher. This is because script fetcher is a `per-module-graph`.
This script fetcher is shared among all the modules in the given module graph.
While it is sufficient to pass parameters to top-level-module's fetching, it is not enough for the future extension.

In the future, we will investigate a way to pass parameters to each non-top-level module.
At that time, this `parameters` should be per-module. This is because `integrity` value should be different for each module.
For example, we will accept some form of syntax to add parameters to `import`. Some proposed syntax is like https://discourse.wicg.io/t/specifying-nonce-or-integrity-when-importing-modules/1861

import "./xxx.js" integrity "xxxxxxx"

In this case, this `parameters` will be passed to "./xxx.js" module fetching. This `parameters` should be different from the one of top-level-module's one. That's why we need per-module `parameters` and why this patch adds `parameters` to the module pipeline.

On the other hand, we also want to keep script fetcher. This `per-module-graph` thing is important to offer module-graph-wide information.
For example, import.meta would have `import.meta.scriptElement`, which is the script element fetching the module graph including this.
That's why we keep both.
https://github.com/tc39/proposal-import-meta

>> Source/JavaScriptCore/ChangeLog:21
>> +        can use thsi parameters to fetch modules.
> 
> thsi -> this.

Oops, fixing.

[Yusuke Suzuki]

Comment on attachment 323439 [details]
Patch

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

> Source/JavaScriptCore/builtins/ModuleLoaderPrototype.js:250
> +                return this.requestSatisfy(depKey, @undefined, fetcher).then((entry) => {

And this is important point. When fetching dependent modules, currently we pass `@undefined` as `parameters`.
This should be changed to per-module parameters once the way to take per-module parameters are defined in the spec.

[Yusuke Suzuki]

Created attachment 323519 [details]
Patch

[Sam Weinig]

Comment on attachment 323519 [details]
Patch

Thanks for the detailed explanation! Looks great.

[Yusuke Suzuki]

Comment on attachment 323519 [details]
Patch

Thank you for your reviews ;)

[WebKit Commit Bot]

Comment on attachment 323519 [details]
Patch

Clearing flags on attachment: 323519

Committed r223237: <https://trac.webkit.org/changeset/223237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.