Issue metadata
Sign in to add a comment
|
Blink calls Evaluate on module with errored dependencies |
||||||||||||||||||||||||
Issue descriptionWhen running the wpt tests in an infinite loop, every now and then evaluation-error-3.html fails: (The OUT lines are debugging output that I added. Status 3 is Instantiated, 4 is Evaluating.) IN: 'http://web-platform.test:8001/html/semantics/scripting-1/the-script-element/module/evaluation-error-3.html\n OUT: INSTANTIATE throw-nested.js OUT: Changing status from 0 to 1 on throw-nested.js OUT: Changing status from 0 to 1 on throw.js OUT: Changing status from 1 to 2 on throw-nested.js OUT: Changing status from 1 to 2 on throw.js OUT: Changing status from 2 to 3 on throw.js OUT: Changing status from 2 to 3 on throw-nested.js OUT: EVALUATE throw.js OUT: Changing status from 3 to 4 on throw.js OUT: Changing status from 4 to errored on throw.js ERR: CONSOLE ERROR: line 2: Uncaught [object Object] OUT: EVALUATE throw-nested.js OUT: Changing status from 3 to 4 on throw-nested.js ERR: ERR: # Fatal error in ../../v8/src/objects.cc, line 20081\n ERR: # Debug check failed: module->status() != kErrored (6 vs. 6).\n ERR: #\n ERR: CONSOLE ERROR: line 1: Uncaught [object Object]\n What seems to happen is: - Non-deterministically, blink chooses to instantiate throw-nested.js (instead of throw.js). - This results in both throw.js and throw-nested.js becoming Instantiated. - Then blink decides to evaluate throw.js. This is a bit strange to me but okay, throw.js got Instantiated so evaluating it now is okay. - This results in throw.js becoming Errored. - Now blink evaluates throw-nested.js. V8 is not prepared for that because at this point throw-nested.js has errored dependencies (namely throw.js). Stacktrace: ERR: #0 0x7f8951660e87 ERR: base::debug::StackTrace::StackTrace()\n ERR: #1 0x7f894f6ac5c5 gin::(anonymous namespace)::PrintStackTrace()\n ERR: #2 0x7f8945f90c9d V8_Fatal()\n ERR: #3 0x7f894f1213d1 ERR: v8::internal::Module::Evaluate()\n ERR: #4 0x7f894f120e4e ERR: v8::internal::Module::Evaluate()\n ERR: #5 0x7f894f12082f ERR: v8::internal::Module::Evaluate()\n ERR: #6 0x7f894eb6fbd1 ERR: v8::Module::Evaluate()\n ERR: #7 0x7f894d41b2c5 ERR: blink::V8ScriptRunner::EvaluateModule()\n ERR: #8 0x7f894d3f6389 ERR: blink::ScriptModule::Evaluate()\n ERR: #9 0x7f894d79be6a ERR: blink::ModulatorImpl::ExecuteModule()\n ERR: #10 0x7f894d7a38df ERR: blink::ModuleScript::RunScript()\n ERR: #11 0x7f894d7e4e15 ERR: blink::ScriptLoader::DoExecuteScript()\n ERR: #12 0x7f894d7e4aa2 ERR: blink::ScriptLoader::ExecuteScriptBlock()\n ERR: #13 0x7f894dc08b3b ERR: blink::(anonymous namespace)::DoExecuteScript()\n ERR: #14 0x7f894dc089e0 ERR: blink::HTMLParserScriptRunner::ExecutePendingScriptAndDispatchEvent()\n ERR: #15 0x7f894dc0a915 ERR: blink::HTMLParserScriptRunner::ExecuteScriptsWaitingForParsing()\n ERR: #16 0x7f894dbeffa4 ERR: blink::HTMLDocumentParser::AttemptToRunDeferredScriptsAndEnd()\n ERR: #17 0x7f894dbf784d ERR: blink::HTMLDocumentParser::NotifyScriptLoaded()\n ERR: #18 0x7f894dc09320 ERR: blink::HTMLParserScriptRunner::PendingScriptFinished()\n ERR: #19 0x7f894d7a1f57 ERR: blink::ModulePendingScriptTreeClient::NotifyModuleTreeLoadFinished()\n ERR: #20 0x7f894dfa38a6 ERR: blink::ModuleTreeLinker::AdvanceState()\n ERR: #21 0x7f894dfa4852 ERR: blink::ModuleTreeLinker::NotifyModuleLoadFinished()\n ERR: #22 0x7f89516616cb ERR: base::debug::TaskAnnotator::RunTask()\n ERR: #23 0x7f894c91e615 ERR: blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue()\n I could change V8 such that it can deal with the situation but I'm wondering if there's perhaps a deeper problem here.
,
Aug 1 2017
Please let me know if blink behaves as expected so that I can avoid the problem on the V8 side if necessary. This issue can cause crashes for users so we should fix it ASAP.
,
Aug 1 2017
I believe that Blink currently behaves as expected.
,
Aug 1 2017
In that case I believe the following V8 change should be sufficient:
@@ -20104,7 +20122,10 @@ MaybeHandle<Object> Module::Evaluate(Handle<Module> module,
ZoneForwardList<Handle<Module>>* stack,
unsigned* dfs_index) {
Isolate* isolate = module->GetIsolate();
- DCHECK_NE(module->status(), kErrored);
+ if (module->status() == kErrored) {
+ isolate->Throw(module->GetException());
+ return MaybeHandle<Object>();
+ }
if (module->status() >= kEvaluating) {
return isolate->factory()->undefined_value();
}
However, with this change the test still fails every now and then. Instead of
crashing in the CHECK, now the assert_array_equals fails because the log
contains only 4 exceptions (it should contain 5).
BTW, reproducing the error becomes much faster and more reliable when my
machine is under heavy load.
,
Aug 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/31173f92e5b7ff94a83736f5e753eaa5f2c45368 commit 31173f92e5b7ff94a83736f5e753eaa5f2c45368 Author: Georg Neis <neis@chromium.org> Date: Wed Aug 02 09:26:08 2017 [modules] Make Evaluate deal with errored dependencies. Apparently it can happen that Blink calls Evaluate on a module that has errored dependencies. R=adamk@chromium.org Bug: v8:1569 , chromium:750024 Change-Id: I44b6dde2d5fe5ca25ca2b8c44ede2683d1be944d Reviewed-on: https://chromium-review.googlesource.com/596055 Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Georg Neis <neis@chromium.org> Cr-Commit-Position: refs/heads/master@{#47071} [modify] https://crrev.com/31173f92e5b7ff94a83736f5e753eaa5f2c45368/src/objects.cc
,
Aug 2 2017
So, now we need to find out if it's also expected behavior that Chrome only reports four exceptions although there are five script elements. Here's the trace that I get when that happens (not sure if it's always the same): Instantiating module throw-nested.js> Changing module status from 0 to 1 for throw-nested.js> Changing module status from 0 to 1 for throw.js> Changing module status from 1 to 2 for throw-nested.js> Changing module status from 1 to 2 for throw.js> Changing module status from 2 to 3 for throw.js> Changing module status from 2 to 3 for throw-nested.js> Instantiating module throw-nested.js> Evaluating module throw.js> Changing module status from 3 to 4 for throw.js> Changing module status from 4 to 6 for throw.js> Evaluating module throw-nested.js> Changing module status from 3 to 4 for throw-nested.js> Changing module status from 4 to 6 for throw-nested.js> This is a testharness.js-based test. FAIL Test that exceptions during evaluation lead to error events on window, and that exceptions are remembered. assert_array_equals: lengths differ, expected 6 got 5 Harness: the test ran to completion. In addition to the Instantiate/Evaluate mismatch that I mentioned earlier, here there are two calls to Instantiate on throw-nested. This looks strange to me but maybe that's just because I'm not very familiar with the fetching spec. In any case, I think reporting only four errors is a bug either in the specification or in blink.
,
Aug 4 2017
There are some known spec issues around error reporting in current Blink impl. I think we should land hiroshige's MTL rewrite https://chromium-review.googlesource.com/c/583552/ asap as it should get us to better state.
,
Aug 8 2017
I have analyzed this test and I think the spec is sound. The problems are likely in the Blink implementation. Detailed analysis at https://docs.google.com/document/d/1LPYmsFa5A41dhctgNuqHbUd9LfE-4HXWtNMf6DqYxIA/edit?usp=sharing ; please review and let me know if I missed anything!
,
Nov 9 2017
This is marked P1 while it has been left untouched for a while. Can someone check if this is still a P1 issue?
,
Nov 9 2017
,
Nov 30 2017
After https://github.com/tc39/ecma262/pull/1006 and https://github.com/whatwg/html/pull/2991, the spec allows calling Evaluate on module with errored dependencies. Merging to Issue 763597 , and I'll handle remaining issues there, if still any. |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by neis@chromium.org
, Jul 28 2017