New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 740886 link

Starred by 16 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Feature



Sign in to add a comment

Link rel=modulepreload

Reported by guybedf...@gmail.com, Jul 11 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce the problem:
1. Clone https://github.com/guybedford/modules-link-preload
2. Run a local server
3. Open test.html

What is the expected behavior?
module999.js seems like it is preloaded ok, but the preloaded source doesn't seem to be used by the loading pipeline. In addition there is a warning that an unnecessary preload request was made.

What went wrong?
The preloaded <link rel="preload" as="script" src="lib/module999.js" /> should be used by the module pipeline.

Did this work before? N/A 

Chrome version: 61.0.3154.0  Channel: canary
OS Version: OS X 10.12.5
Flash Version:
 

Comment 1 by rtoy@chromium.org, Jul 11 2017

Components: -Blink Blink>Loader
Status: Untriaged (was: Unconfirmed)
I can duplicate this.

Setting component to Blink>Loader for further triage
Components: Blink>HTML>Script
Labels: -OS-Mac OS-All

Comment 3 by kinuko@chromium.org, Jul 24 2017

Cc: domenic@chromium.org yhirano@chromium.org
Owner: kouhei@chromium.org
Is there still a spec issue that needs to be resolved?

Comment 4 by neis@chromium.org, Jul 24 2017

Status: Assigned (was: Untriaged)
> Is there still a spec issue that needs to be resolved?

It depends. At least theoretically, I think we can do some work here without further spec work. That is, we should be able to hook up the preloaded source so that it is used by the module pipeline, even if as dumb text. Maybe that is a good target for this bug?

The extra spec work comes about in figuring out if we can also parse and compile the source text, and even store it in the the module map. Currently as="script" does not give enough information to do that. That is why we had a spec thread at https://github.com/whatwg/fetch/issues/486, with a summary at https://github.com/whatwg/fetch/issues/486#issuecomment-282044172 and a proposal at https://github.com/whatwg/html/pull/2383 .

That proposal currently does not have any implementer interest, so if Chrome is interested in it (or has opinions on how it should be changed---e.g. maybe crawling the graph is not worthwhile), please comment there.
> The extra spec work comes about in figuring out if we can also parse and compile the source text, and even store it in the the module map. 

You could actually instantiate as well, in theory.

Comment 7 by kouhei@chromium.org, Jul 24 2017

Short term solution/question:
Why are we not reusing ScriptResource preloaded? Check why it doesn't hit -> make it hit.
After this work, the module script doesn't benefit from ahead of time compile/instantiate, but should still benefit from having the raw bytes on blink MemoryCache.

Long term solution:

Step 1: Distinguish blink::{Module,Classic}ScriptResource
Step 2: if as="modulescript", LinkPreloader preloads ModuleScriptResource which CompileModule() on Resource::NotifyFinished()
Step 3: Also call InstantiateModule() somehow?
I'm aware this isn't necessarily the best place to discuss this, so feel free to ignore. But I worry a lot about module loading performance - for example in https://bugs.chromium.org/p/chromium/issues/detail?id=740874.

Now imagine I wanted to make the slow tree in that issue load without the latency delay in browsers. The fix I'd like to add for this is to tell the browser all the deep dependencies ahead of time, using a link preload. The goal being that it should be possible to parse and execute the modules then in a single latency (the modules here have very low bandwidth).

If my only option is to instantiate preload, then I'm having to deeply instantiate every module in the tree, which would lead to exponential traversal of the tree, in addition to breaking circular reference instantiations as they now become top-level instantiate calls.

This is why I'd simply like to see a simple fetch-based preloader without instantiation.

Yes, instantiation may still be a useful preloading technique, but as a separate use case to trying to speed up deep prefetching.
Correction to the above - circular references are unaffected by top-level instantiating every module in the tree. But the argument over performance stands. A tree of N nodes being traversed M ~ O(N) times just sounds like a perf issue waiting to happen.
>c#8
The bug was already fixed (and made it to M61 branch). Sorry I forgot to update the bug.
Amazing! Thanks so much for the update, that is great to hear.
re: partial instantiate, as long as the subgraph is complete, we can mark all the subgraph records as "instantiated" and not need to visit the nodes in the subgraph again, which would avoid exponential time.

There is still an open question about how we would know that the subgraph is complete w/o actually trying though. (If we always try, we will still have a worst case scn of exponential time)
Actually it sounds like the performance profile from https://bugs.chromium.org/p/chromium/issues/detail?id=740874 would be exactly what to expect with this preload mechanism though - ~20s load time for 170 modules preloaded.
If preloading is done upfront, subgraphs with circular references can easily overlap causing deadlock. The result being one still ends up traversing every subgraph for this scenario.

This is why I say an instantiation preload is a separate use case - it's not for dealing with the graph latency problem, its for dealing with preloading a dynamic load event to get new functionality of another section of the app, as opposed to boot-time loading.
>c#13
I don't think it is not necessarily the case. We have many ways that we can address c#12 w/o exponential worst case. One top of my head is "memorizing" the incomplete edges for each module graph node.

That said, as I made a note in c#7, we will start from caching raw bytes in Memory Cache, then caching parsed module script, and finally explore eager instantiating.
Thanks, as long as performance is upheld as the primary requirement, that is my only main concern. Let the numbers decide then certainly. And I will continue to test performance here too.
Cc: kouhei@chromium.org
Owner: ksakamoto@chromium.org
ksakamoto: Would you own this?
Did some initial investigation.

Re #c7:
> Why are we not reusing ScriptResource preloaded? Check why it doesn't hit -> make it hit.

To make it hit, requests' credentials mode must match.
In the current implementation, it hits memory cached ScriptResource if both <link rel=preload> and <script type=module> have the same crossorigin= attribute (anonymous or use-credentials).

If <script type=module> has no crossorigin= attribute, the module is fetched with "omit" credentials mode, but there's no way to use this mode in <link rel=preload>.

Cc: kinuko@chromium.org
Project Member

Comment 20 by bugdroid1@chromium.org, Sep 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73482ef199fa82d96601f4221dcfe1debd0d7ede

commit 73482ef199fa82d96601f4221dcfe1debd0d7ede
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Wed Sep 13 06:41:50 2017

Add a layout test for module script preloading

As a follow-up to https://chromium-review.googlesource.com/658016,
this adds a layout test that makes sure module scripts are preloaded.

Bug:  740886 
Change-Id: Iffa21862444d940750d03bf7cb744812cd87f4a1
Reviewed-on: https://chromium-review.googlesource.com/664498
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501557}
[add] https://crrev.com/73482ef199fa82d96601f4221dcfe1debd0d7ede/third_party/WebKit/LayoutTests/http/tests/preload/module-script.html
[add] https://crrev.com/73482ef199fa82d96601f4221dcfe1debd0d7ede/third_party/WebKit/LayoutTests/http/tests/preload/resources/empty.js

I tried the Canary release 63.0.3223.8 which includes the fix, but I am still seeing messages like: The resource xxxx was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it wasn't preloaded for nothing.

I've made sure to match the crossorigins and everything. I even tried the repo project in this original issue and get the same error.
Sorry for confusion, the above patch is for Preload Scanner, another kind of preload.
Link preload for modules still doesn't work, we're working on it.

Comment 23 by addyo@chromium.org, Sep 30 2017

Cc: addyo@chromium.org

Comment 24 by y...@yoav.ws, Sep 30 2017

Cc: y...@yoav.ws
Cc: ksakamoto@chromium.org
 Issue 770205  has been merged into this issue.
Cc: sgomes@chromium.org
Project Member

Comment 27 by bugdroid1@chromium.org, Nov 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80b8282d97f105ac5f59a50640d732e62374e4e2

commit 80b8282d97f105ac5f59a50640d732e62374e4e2
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Fri Nov 10 06:52:28 2017

Implement link rel=modulepreload

This implements link rel=modulepreload without submodules fetch,
behind the Experimental Web Platform feature flag.

LinkLoader fetches module script using Modulator::FetchSingle, so it
populates the module map but does not fetch descendant modules.
Unlike the preload fetches by link rel=preload, this doesn't set
FetchParameters::SetLinkPreload flag, so it works like a regular
script fetch.

HTMLPreloadScanner fetches module scripts in a similar way to
speculative preload of <script type=module>, i.e. just fetch a
ScriptResource and not populate the module map.

Spec PR: https://github.com/whatwg/html/pull/2383
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/ynkrM70KDD4

Bug:  740886 
Change-Id: I5f4420e305f4962b3fbe3f703ce95a5a43e4f7a9
Reviewed-on: https://chromium-review.googlesource.com/662697
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515490}
[add] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/LayoutTests/external/wpt/preload/modulepreload.html
[add] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/LayoutTests/external/wpt/preload/resources/module1.js
[add] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/LayoutTests/external/wpt/preload/resources/module2.js
[add] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/LayoutTests/external/wpt/preload/resources/syntax-error.js
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/dom/DynamicModuleResolver.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/dom/DynamicModuleResolverTest.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/dom/ModuleMap.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/dom/ModuleMapTest.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/dom/ScriptLoader.h
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/html/LinkRelAttribute.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/html/LinkRelAttribute.h
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/html/RelList.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/html/parser/PreloadRequest.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/loader/LinkLoader.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/loader/LinkLoader.h
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptFetchRequest.h
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp
[modify] https://crrev.com/80b8282d97f105ac5f59a50640d732e62374e4e2/third_party/WebKit/Source/platform/runtime_enabled_features.json5

Project Member

Comment 28 by bugdroid1@chromium.org, Nov 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/74c7dbf73733dbb4791e699851345f3789687498

commit 74c7dbf73733dbb4791e699851345f3789687498
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Thu Nov 16 03:47:04 2017

Link rel=modulepreload: Add comments that map code to the spec

This patch also fixes the followings:
- Reject modulepreloads with invalid as="" value
- ParserState should be "not parser inserted"

Bug:  740886 

Change-Id: I9f3c8e87da31a0b09bad208d681e200b6a06d69e
Reviewed-on: https://chromium-review.googlesource.com/768773
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516974}
[modify] https://crrev.com/74c7dbf73733dbb4791e699851345f3789687498/third_party/WebKit/LayoutTests/external/wpt/preload/modulepreload.html
[modify] https://crrev.com/74c7dbf73733dbb4791e699851345f3789687498/third_party/WebKit/Source/core/loader/LinkLoader.cpp
[modify] https://crrev.com/74c7dbf73733dbb4791e699851345f3789687498/third_party/WebKit/Source/core/loader/LinkLoader.h
[modify] https://crrev.com/74c7dbf73733dbb4791e699851345f3789687498/third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp

Project Member

Comment 29 by bugdroid1@chromium.org, Nov 16 2017

Project Member

Comment 30 by bugdroid1@chromium.org, Nov 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c7f97a60d5b54d507f9f4494f002934124b9d74

commit 0c7f97a60d5b54d507f9f4494f002934124b9d74
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Tue Nov 21 01:34:58 2017

Link rel=modulepreload: Support integrity attribute

This CL enables subresource integrity check for link rel=modulepreload, by
passing integrity value to ScriptFetchOptions.

Note that this is only for modulepreload; integrity is still not supported
for link rel=preload (crbug.com/677022).

Bug:  740886 
Change-Id: I09159ed802e49b402e71ff464dee56a380a73b07
Reviewed-on: https://chromium-review.googlesource.com/773719
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518037}
[modify] https://crrev.com/0c7f97a60d5b54d507f9f4494f002934124b9d74/third_party/WebKit/LayoutTests/external/wpt/preload/modulepreload.html
[modify] https://crrev.com/0c7f97a60d5b54d507f9f4494f002934124b9d74/third_party/WebKit/Source/core/html/HTMLLinkElement.cpp
[modify] https://crrev.com/0c7f97a60d5b54d507f9f4494f002934124b9d74/third_party/WebKit/Source/core/html/HTMLLinkElement.h
[modify] https://crrev.com/0c7f97a60d5b54d507f9f4494f002934124b9d74/third_party/WebKit/Source/core/html/LinkStyle.cpp
[modify] https://crrev.com/0c7f97a60d5b54d507f9f4494f002934124b9d74/third_party/WebKit/Source/core/loader/LinkLoader.cpp
[modify] https://crrev.com/0c7f97a60d5b54d507f9f4494f002934124b9d74/third_party/WebKit/Source/core/loader/LinkLoader.h
[modify] https://crrev.com/0c7f97a60d5b54d507f9f4494f002934124b9d74/third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp
[modify] https://crrev.com/0c7f97a60d5b54d507f9f4494f002934124b9d74/third_party/WebKit/Source/platform/loader/LinkHeader.cpp
[modify] https://crrev.com/0c7f97a60d5b54d507f9f4494f002934124b9d74/third_party/WebKit/Source/platform/loader/LinkHeader.h

Project Member

Comment 31 by bugdroid1@chromium.org, Dec 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/960460d86dcd0d20f9c382ec82924ce1a2e7940f

commit 960460d86dcd0d20f9c382ec82924ce1a2e7940f
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Tue Dec 05 17:13:11 2017

Fire load event for link modulepreload with non-fetch errors

According to the spec comment, load events (instead of error events)
should be fired for modules with non-fetch errors such as parse errors.

Bug:  740886 ,  763597 
Change-Id: I6074b36631e1299b90d52bc01e42c5ddeb957b31
Reviewed-on: https://chromium-review.googlesource.com/802251
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521726}
[modify] https://crrev.com/960460d86dcd0d20f9c382ec82924ce1a2e7940f/third_party/WebKit/LayoutTests/external/wpt/preload/modulepreload.html
[modify] https://crrev.com/960460d86dcd0d20f9c382ec82924ce1a2e7940f/third_party/WebKit/Source/core/loader/LinkLoader.cpp

Project Member

Comment 32 by bugdroid1@chromium.org, Dec 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b60066879b0a160ff9111dcf92d1c1aced5648aa

commit b60066879b0a160ff9111dcf92d1c1aced5648aa
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Mon Dec 18 03:18:16 2017

Update comment in modulepreload web platform test

Bug:  740886 
Change-Id: I7cdfd6099a24bd36ccb1f962317f83b5e5299f20
Reviewed-on: https://chromium-review.googlesource.com/828342
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524645}
[modify] https://crrev.com/b60066879b0a160ff9111dcf92d1c1aced5648aa/third_party/WebKit/LayoutTests/external/wpt/preload/modulepreload.html

Project Member

Comment 33 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c9b82a67c6d7b0560df77f0e29272ae3f34d2bc

commit 8c9b82a67c6d7b0560df77f0e29272ae3f34d2bc
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Fri Feb 16 04:01:12 2018

Ship link rel=modulepreload

Intent to Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/T-OkMDASYfk

Bug:  740886 
Change-Id: I494bc350b5a7eaa2d3f46e4fa948ef1c3b014d37
Reviewed-on: https://chromium-review.googlesource.com/923221
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537187}
[modify] https://crrev.com/8c9b82a67c6d7b0560df77f0e29272ae3f34d2bc/third_party/WebKit/Source/platform/runtime_enabled_features.json5

Cc: krajshree@chromium.org
Labels: Needs-Feedback
Tested the issue on Win-10 and mac 10.12.6 using latest chrome version #66.0.3350.0 as per the merged  issue #770205  in comment #25.

Attached a screen cast and screenshot for reference.

Following are the steps followed to reproduce the issue.
------------
1. Ran http server command and opened the html file in issue id: 770205
2. Opened dev tools->network tab
3. Observed that index.js file loaded twice as in the reporter version #61.0.3163.100

ksakamoto@ - Could you please check the attached screen cast and screenshot and please let us know the expected behaviour and please confirm the fix.

Thanks...!!
740886.mp4
1.4 MB View Download
740886@M61.png
443 KB View Download
Labels: -Type-Bug -Needs-Feedback M-66 Type-Feature
Status: Fixed (was: Assigned)
Summary: Link rel=modulepreload (was: Link preload not working with module scripts)
krajshree@: The processing model of <link rel="preload"> does not work well for module scripts, so we introduced a new primitive <link rel="modulepreload">.

Changing Summary and Type to reflect that.

Sign in to add a comment