|
|
Created:
3 years, 8 months ago by kouhei (in TOK) Modified:
3 years, 8 months ago CC:
chromium-reviews, blink-reviews, blink-reviews-bindings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ES6 modules] ScriptModule::instantiate with resolveModule cb and error handling
This CL replaces ScriptModule::instantiate implementation to support
HostResolveModule callback routed to Modulator and proper error handling
BUG=594639
Review-Url: https://codereview.chromium.org/2785983002
Cr-Commit-Position: refs/heads/master@{#461675}
Committed: https://chromium.googlesource.com/chromium/src/+/be24886a5bda4deb8a19e2026533d62246101707
Patch Set 1 #
Total comments: 2
Patch Set 2 : neis review #
Total comments: 11
Patch Set 3 : haraken/yuki #Patch Set 4 : add_tests #
Depends on Patchset: Messages
Total messages: 27 (12 generated)
kouhei@chromium.org changed reviewers: + adamk@chromium.org, haraken@chromium.org, neis@chromium.org, yhirano@chromium.org
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...
lgtm https://codereview.chromium.org/2785983002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2785983002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:29: ScriptValue instantiate(ScriptState*); Please add a comment saying that this returns the exception, if any.
bindings/OWNERS: ptal Thanks! https://codereview.chromium.org/2785983002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2785983002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:29: ScriptValue instantiate(ScriptState*); On 2017/03/30 06:38:40, neis wrote: > Please add a comment saying that this returns the exception, if any. Done.
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right): https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:38: v8::MaybeLocal<v8::Module> ScriptModule::resolveModuleCallback( Please place implementation code in the same order as declarations in the header. https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:59: ScriptValue ScriptModule::instantiate(ScriptState* scriptState) { Is it possible to take an ExceptionState instead of returning ScriptValue? https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:68: DCHECK(tryCatch.HasCaught()); We'd also like to guarantee that there is no exception if success. DCHECK_EQ(!success, tryCatch.HasCaught()); or something like that?
https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right): https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:32: v8::MaybeLocal<v8::Module> dummyCallback(v8::Local<v8::Context> context, Can this be removed? https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:51: if (resolved.isNull()) { In Module::PrepareInstantiate: // TODO(adamk): Revisit these failure cases once d8 knows how to // persist a module_map across multiple top-level module loads, as // the current module is left in a "half-instantiated" state. if (!callback(context, v8::Utils::ToLocal(specifier), v8::Utils::ToLocal(module)) .ToLocal(&api_requested_module)) { // TODO(adamk): Give this a better error message. But this is a // misuse of the API anyway. isolate->ThrowIllegalOperation(); return false; } I'm not sure what "misuse of the API" means. Is it valid to return an empty here?
https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right): https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:51: if (resolved.isNull()) { On 2017/03/30 08:05:17, yhirano wrote: > In Module::PrepareInstantiate: > > // TODO(adamk): Revisit these failure cases once d8 knows how to > // persist a module_map across multiple top-level module loads, as > // the current module is left in a "half-instantiated" state. > if (!callback(context, v8::Utils::ToLocal(specifier), > v8::Utils::ToLocal(module)) > .ToLocal(&api_requested_module)) { > // TODO(adamk): Give this a better error message. But this is a > // misuse of the API anyway. > isolate->ThrowIllegalOperation(); > return false; > } > > I'm not sure what "misuse of the API" means. Is it valid to return an empty > here? Excellent question! V8 must be able to deal with the situation where the callback returns nothing. This will be the case when instantiating a root module that depends on a module whose compilation or instantiation has already failed earlier. I will make the necessary changes in V8 and exercise such situations in the WPT tests that I'm working on (I believe we cannot test this situation in d8). Kouhei, please double check that the resolve call above returns null for modules that failed to compile or instantiate. (This CL doesn't include the resolve implementation.)
https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right): https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:59: ScriptValue ScriptModule::instantiate(ScriptState* scriptState) { On 2017/03/30 07:34:45, Yuki wrote: > Is it possible to take an ExceptionState instead of returning ScriptValue? I want to see a call site of ScriptModule::instantiate, but would it be an option to let the call site use TryCatch and handle the thrown exception (instead of making ScriptModule::instantiate return an exception)?
https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right): https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:32: v8::MaybeLocal<v8::Module> dummyCallback(v8::Local<v8::Context> context, On 2017/03/30 08:05:17, yhirano wrote: > Can this be removed? Done. https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:38: v8::MaybeLocal<v8::Module> ScriptModule::resolveModuleCallback( On 2017/03/30 07:34:45, Yuki wrote: > Please place implementation code in the same order as declarations in the > header. Done. https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:59: ScriptValue ScriptModule::instantiate(ScriptState* scriptState) { On 2017/03/30 13:26:00, haraken wrote: > On 2017/03/30 07:34:45, Yuki wrote: > > Is it possible to take an ExceptionState instead of returning ScriptValue? > > I want to see a call site of ScriptModule::instantiate, but would it be an > option to let the call site use TryCatch and handle the thrown exception > (instead of making ScriptModule::instantiate return an exception)? re: ExceptionState, I think ScriptValue or v8::Local<v8::Value> makes more sense here, as we don't report the exception back to V8 synchronously. My understanding of STACK_ALLOCATED ExcState is it is used when the callstack originates from V8 bindings entering Blink in someway, and used to trigger V8 exc in the end. In modules, we store the exception caught during instantiate, and store it inside ModuleScript blink heap obj, then rethrow the exception to V8 whenever we try to execute the ModuleScript. The planned callsite is: ModuleTreeLinker::instantiate() https://codereview.chromium.org/2555653002/diff/740001/third_party/WebKit/Sou... which later calls into ModulatorImpl::instantiateModule() which fills in ScriptState https://codereview.chromium.org/2555653002/diff/740001/third_party/WebKit/Sou... I'd prefer to abstract away the TryCatch here as currently written, primarily because I'm not comfortable w/ keeping TryCatch code in core/loader/modulescript https://codereview.chromium.org/2785983002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:68: DCHECK(tryCatch.HasCaught()); On 2017/03/30 07:34:45, Yuki wrote: > We'd also like to guarantee that there is no exception if success. > > DCHECK_EQ(!success, tryCatch.HasCaught()); > or something like that? Done.
LGTM. Thanks for the explanation.
lgtm
Can we add a unit test for resolveModuleCallback?
On 2017/04/04 02:10:27, haraken wrote: > Can we add a unit test for resolveModuleCallback? 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...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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, yhirano@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2785983002/#ps60001 (title: "add_tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491296005135500, "parent_rev": "4856780bb75144b6e7df21dcaead690da8e4d08a", "commit_rev": "be24886a5bda4deb8a19e2026533d62246101707"}
Message was sent while issue was closed.
Description was changed from ========== [ES6 modules] ScriptModule::instantiate with resolveModule cb and error handling This CL replaces ScriptModule::instantiate implementation to support HostResolveModule callback routed to Modulator and proper error handling BUG=594639 ========== to ========== [ES6 modules] ScriptModule::instantiate with resolveModule cb and error handling This CL replaces ScriptModule::instantiate implementation to support HostResolveModule callback routed to Modulator and proper error handling BUG=594639 Review-Url: https://codereview.chromium.org/2785983002 Cr-Commit-Position: refs/heads/master@{#461675} Committed: https://chromium.googlesource.com/chromium/src/+/be24886a5bda4deb8a19e2026533... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/be24886a5bda4deb8a19e2026533... |