Issue 750024 in chromium: Blink calls Evaluate on module with errored dependencies

[n… via monorail]

Status: Available
Owner: ----
CC: modul...@chromium.org
Components: Blink>HTML>Script

New issue 750024 by ne...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024

When 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.

--
You received this message because:
1. You were specifically CC'd on the issue

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

Reply to this email to add a comment.

[n… via monorail]

Updates:
Cc: dom...@chromium.org

Comment #1 on issue 750024 by ne...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c1

This might very well be a specification issue.

[n… via monorail]

Comment #2 on issue 750024 by ne...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c2

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.

[kou… via monorail]

Comment #3 on issue 750024 by kou...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c3

I believe that Blink currently behaves as expected.

[n… via monorail]

Comment #4 on issue 750024 by ne...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c4

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.

[bugdro… via monorail]

Comment #5 on issue 750024 by bugd...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c5

The following revision refers to this bug:
https://chromium.googlesource.com/v8/v8.git/+/31173f92e5b7ff94a83736f5e753eaa5f2c45368

commit 31173f92e5b7ff94a83736f5e753eaa5f2c45368
Author: Georg Neis <ne...@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=ad...@chromium.org

Bug: v8:1569, chromium:750024
Change-Id: I44b6dde2d5fe5ca25ca2b8c44ede2683d1be944d
Reviewed-on: https://chromium-review.googlesource.com/596055
Reviewed-by: Adam Klein <ad...@chromium.org>
Commit-Queue: Georg Neis <ne...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47071}
[modify] https://crrev.com/31173f92e5b7ff94a83736f5e753eaa5f2c45368/src/objects.cc

[n… via monorail]

Comment #6 on issue 750024 by ne...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c6

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.

[kou… via monorail]

Comment #7 on issue 750024 by kou...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c7

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.

[dome… via monorail]

Comment #8 on issue 750024 by dom...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c8

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!

[ko… via monorail]

Comment #9 on issue 750024 by ko...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c9

This is marked P1 while it has been left untouched for a while.
Can someone check if this is still a P1 issue?

[kou… via monorail]

Updates:
Cc: ne...@chromium.org
Owner: hirosh...@chromium.org

Comment #10 on issue 750024 by kou...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c10

(No comment was entered for this change.)

[hirosh… via monorail]

Updates:
Mergedinto: 763597
Status: Duplicate

Comment #11 on issue 750024 by hirosh...@chromium.org: Blink calls Evaluate on module with errored dependencies
https://bugs.chromium.org/p/chromium/issues/detail?id=750024#c11

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.