Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860993002:
[ES6 modules] ModuleMap::GetFetchedModuleScript to return nullptr when entry not found / "fetching" (Closed)
|
Created:
3 years, 7 months ago by kouhei (in TOK) Modified:
3 years, 7 months ago CC:
chromium-reviews, blink-reviews-w3ctests_chromium.org, tfarina, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ES6 modules] ModuleMap::GetFetchedModuleScript to return nullptr when entry not found / "fetching"
Before this CL, we asserted that ModuleMap::GetFetchedModuleScript always queried load completed module scripts, but it was not always the case.
This CL changes the member function to return nullptr if the entry doesn't exist or the entry is being fetched.
This CL fixes a crash that happens when we early-exit module script graph fetch by a sub-graph instantiation failed, but there are still partial subgraph in flight.
BUG=718442, 594639
Review-Url: https://codereview.chromium.org/2860993002
Cr-Commit-Position: refs/heads/master@{#472192}
Committed: https://chromium.googlesource.com/chromium/src/+/1f92cfbdedaecb97e8a87773deb32f98c6997351
Patch Set 1 #
Total comments: 5
Patch Set 2 : neis #
Total comments: 1
Patch Set 3 : add comment #Patch Set 4 : rm-wrong-test #Patch Set 5 : update Modulator.h comment too #
Messages
Total messages: 43 (26 generated)
kouhei@chromium.org changed reviewers: + domenic@chromium.org, module-dev@chromium.org
https://codereview.chromium.org/2860993002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/import-subgraph-404.html (right): https://codereview.chromium.org/2860993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/import-subgraph-404.html:1: <!DOCTYPE html> W/ this CL applied, this test should not crash, but... https://codereview.chromium.org/2860993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/import-subgraph-404.html:5: import { delayedLoaded } from "./resources/delayed-modulescript.py"; Wether delayed-modulescript.py would succeed to instantiate/not is non-deterministic. https://codereview.chromium.org/2860993002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/import-subgraph-404.html:8: </script> if we write <script type=module> import "./resources/delayed-modulescript.py"; here, the module may/may not instantiate.
Proper fix blocked on https://github.com/whatwg/html/issues/2630 , but we might want to land something to avoid the crash from this.
neis@chromium.org changed reviewers: + neis@chromium.org
https://codereview.chromium.org/2860993002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ModuleMap.cpp (right): https://codereview.chromium.org/2860993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleMap.cpp:148: return it->value->GetModuleScript(); This return statement makes no sense if the url wasn't found.
Description was changed from
==========
[ES6 modules] {D,}CHECKs in ModuleMap
BUG=718442,594639
==========
to
==========
[ES6 modules] ModuleMap::GetFetchedModuleScript to return nullptr when entry not
found / "fetching"
Before this CL, we asserted that ModuleMap::GetFetchedModuleScript always
queried load completed module scripts, but it was not always the case.
This CL changes the member function to return nullptr if the entry doesn't exist
or the entry is being fetched.
This CL fixes a crash that happens when we early-exit module script graph fetch
by a sub-graph instantiation failed, but there are still partial subgraph in
flight.
BUG=718442,594639
==========
I think the change makes sense to fix the crash, thus would like to proceed. https://codereview.chromium.org/2860993002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ModuleMap.cpp (right): https://codereview.chromium.org/2860993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ModuleMap.cpp:148: return it->value->GetModuleScript(); On 2017/05/09 08:56:37, neis wrote: > This return statement makes no sense if the url wasn't found. Done.
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with comment https://codereview.chromium.org/2860993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ModuleMap.cpp (right): https://codereview.chromium.org/2860993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModuleMap.cpp:151: } In Modulator.h, the comment on GetFetchedModuleScript says: // Synchronously retrieves a single module script from existing module map // entry. // Note: returns nullptr if the module map entry is still "fetching". The first sentence should be updated to reflect that the entry does not have to exist. (From what I understand, the second sentence wasn't true before removing the DCHECK, but is true now.)
kouhei@chromium.org changed reviewers: + hiroshige@chromium.org
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org Link to the patchset: https://codereview.chromium.org/2860993002/#ps40001 (title: "add comment")
The CQ bit was unchecked by kouhei@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/05/15 20:16:41, kouhei (travelling SFO) wrote: Your message was empty, did some text get lost? I see you didn't change Modulator.h, was I wrong?
The CQ bit was checked by hiroshige@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org Link to the patchset: https://codereview.chromium.org/2860993002/#ps60001 (title: "rm-wrong-test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/16 08:35:47, neis wrote: > On 2017/05/15 20:16:41, kouhei (travelling SFO) wrote: > > Your message was empty, did some text get lost? I see you didn't change > Modulator.h, was I wrong? Sorry, I missed change to Modulator.h. Addressed on PS5
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, neis@chromium.org Link to the patchset: https://codereview.chromium.org/2860993002/#ps80001 (title: "update Modulator.h comment too")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494954024096080,
"parent_rev": "883ea6f5b904ba890bf23ff0b76bd711f6f343f9", "commit_rev":
"1f92cfbdedaecb97e8a87773deb32f98c6997351"}
Message was sent while issue was closed.
Description was changed from ========== [ES6 modules] ModuleMap::GetFetchedModuleScript to return nullptr when entry not found / "fetching" Before this CL, we asserted that ModuleMap::GetFetchedModuleScript always queried load completed module scripts, but it was not always the case. This CL changes the member function to return nullptr if the entry doesn't exist or the entry is being fetched. This CL fixes a crash that happens when we early-exit module script graph fetch by a sub-graph instantiation failed, but there are still partial subgraph in flight. BUG=718442,594639 ========== to ========== [ES6 modules] ModuleMap::GetFetchedModuleScript to return nullptr when entry not found / "fetching" Before this CL, we asserted that ModuleMap::GetFetchedModuleScript always queried load completed module scripts, but it was not always the case. This CL changes the member function to return nullptr if the entry doesn't exist or the entry is being fetched. This CL fixes a crash that happens when we early-exit module script graph fetch by a sub-graph instantiation failed, but there are still partial subgraph in flight. BUG=718442,594639 Review-Url: https://codereview.chromium.org/2860993002 Cr-Commit-Position: refs/heads/master@{#472192} Committed: https://chromium.googlesource.com/chromium/src/+/1f92cfbdedaecb97e8a87773deb3... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1f92cfbdedaecb97e8a87773deb3...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2886123002/ by kolos@chromium.org. The reason for reverting is: This CL probably causes test failures external/wpt/html/semantics/scripting-1/the-script-element/module/import-subgraph-404.html The first failed build: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
