Summary: | Support integrity="" on module scripts | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Domenic Denicola <d> | ||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, dbates, esprehn+autocc, kangil.han, keith_miller, mark.lam, msaboff, saam, sam, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Safari Technology Preview | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 178162 | ||||||||||
Bug Blocks: | 147340 | ||||||||||
Attachments: |
|
Description
Domenic Denicola
2017-10-05 12:29:34 PDT
Created attachment 323420 [details]
Patch
WIP: Works in GTK with WPT. Need to add files to Xcode project file.
Created attachment 323439 [details]
Patch
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. 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. 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. Created attachment 323519 [details]
Patch
Comment on attachment 323519 [details]
Patch
Thanks for the detailed explanation! Looks great.
Comment on attachment 323519 [details]
Patch
Thank you for your reviews ;)
Comment on attachment 323519 [details] Patch Clearing flags on attachment: 323519 Committed r223237: <https://trac.webkit.org/changeset/223237> All reviewed patches have been landed. Closing bug. |