Closed Bug 1689499 Opened 4 years ago Closed 4 years ago

Fix async module import cycle detection

Categories

(Core :: JavaScript Engine, task, P1)

Product:
Core
Component:
JavaScript Engine
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
firefox88 --- fixed

People

(Reporter: jorendorff, Assigned: yulia)

References

Details

Attachments

(3 files)

Bug 1689499 - Add test to show cycle issue with multiple parents;r=jorendorff
48 bytes, text/x-phabricator-request
Details | Review
Bug 1689499 - fix throwing check in asyncModuleExecutionRejected when reaching an errored module the second time;r=jorendorff
48 bytes, text/x-phabricator-request
Details | Review
Bug 1689499 - Replace GetAsyncCycleRoot with [[CycleRoot]] field;r=jorendorff
48 bytes, text/x-phabricator-request
Details | Review

The test case below first tries dynamically loading A, then X. Evaluating the body of module B succeeds, but module C fails.

Expected: Since A, B, and C are in a strongly connected component, per spec the same [[EvaluationError]] should be stored in all of them. Therefore loading X should fail, because B failed.

Observed: B is marked as "evaluated" with no [[EvaluationError]]. Loading X succeeds.

A.js:

// |reftest| skip -- support file

import B from "./B.js";
import C from "./C.js";
export default "A";

B.js:

// |reftest| skip -- support file

import A from "./A.js";
export default "B";
if (false) await 0;

C.js:

// |reftest| skip -- support file

import A from "./A.js";
export default "C";
if (false) await 0;
throw "FAIL";

X.js:

// |reftest| skip -- support file

import B from "./B.js";
export default "X";

test.js:

// |reftest| module shell-option(--enable-top-level-await)

try {
  await import("./A.js");
  throw new Error("import should fail");
} catch (exc) {
  assertEq(exc, "FAIL");
}

print("so far so good");

try {
  await import("./X.js");
  throw new Error("import should fail");
} catch (exc) {
  assertEq(exc, "FAIL");
}

print("pass");

Note that tc39/proposal-top-level-await#156 contains some discussion about this.

Errors that happen synchronously and errors that happen asynchronously go through different spec steps, but I think Guy is saying the test should behave the same even if you insert some nontrivial await expressions in all of the modules.

Assignee: nobody → ystartsev
Status: NEW → ASSIGNED
Severity: -- → N/A
Priority: -- → P1

This patch queue, when fully applied (including D101465 which was the start of all this!), addresses the test case.

Blocks: 1693261
Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d6efdab9e842 Add test to show cycle issue with multiple parents;r=jorendorff https://hg.mozilla.org/integration/autoland/rev/d284779b08b4 fix throwing check in asyncModuleExecutionRejected when reaching an errored module the second time;r=jorendorff https://hg.mozilla.org/integration/autoland/rev/6ae5b3c665fd Replace GetAsyncCycleRoot with [[CycleRoot]] field;r=jorendorff

Backed out for assertion failures on Value.h

backout: https://hg.mozilla.org/integration/autoland/rev/bfe0b7fdfc027afbff4e2d9002a771d92d3bee41

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&searchStr=jsreftest&revision=59fe9cc6ef8d816eaed9ffd74250c1e2182625fb

failure log: https://treeherder.mozilla.org/logviewer?job_id=330706645&repo=autoland&lineNumber=9600

[task 2021-02-22T09:45:53.502Z] 09:45:53     INFO -  Initializing stack-fixing for the first stack frame, this may take a while...
[task 2021-02-22T09:46:01.967Z] 09:46:01     INFO - #01: OnResolvedDynamicModule(JSContext*, unsigned int, JS::Value*) [js/src/builtin/ModuleObject.cpp:2235]
[task 2021-02-22T09:46:01.968Z] 09:46:01     INFO - #02: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) [js/src/vm/Interpreter.cpp:435]
[task 2021-02-22T09:46:01.968Z] 09:46:01     INFO - #03: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:520]
[task 2021-02-22T09:46:01.969Z] 09:46:01     INFO - #04: InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) [js/src/vm/Interpreter.cpp:580]
[task 2021-02-22T09:46:01.970Z] 09:46:01     INFO - #05: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) [js/src/vm/Interpreter.cpp:597]
[task 2021-02-22T09:46:01.970Z] 09:46:01     INFO - #06: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.h:107]
[task 2021-02-22T09:46:01.971Z] 09:46:01     INFO - #07: PromiseReactionJob(JSContext*, unsigned int, JS::Value*) [js/src/builtin/Promise.cpp:1905]
[task 2021-02-22T09:46:01.971Z] 09:46:01     INFO - #08: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) [js/src/vm/Interpreter.cpp:435]
[task 2021-02-22T09:46:01.971Z] 09:46:01     INFO - #09: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:520]
[task 2021-02-22T09:46:01.972Z] 09:46:01     INFO - #10: InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) [js/src/vm/Interpreter.cpp:580]
[task 2021-02-22T09:46:01.972Z] 09:46:01     INFO - #11: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) [js/src/vm/Interpreter.cpp:597]
[task 2021-02-22T09:46:01.972Z] 09:46:01     INFO - #12: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:2861]
[task 2021-02-22T09:46:01.973Z] 09:46:01     INFO - #13: mozilla::dom::PromiseJobCallback::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::ErrorResult&) [s3:gecko-generated-sources:cac1e8a859bf007c4bb469353b11f3d630a177e3fbc0d9735897aa03c6234a3abe6cd36ec6927b54b0636bc8fb052f005caaaac7035e6ba6d96902977f3658df/dom/bindings/PromiseBinding.cpp::31]
[task 2021-02-22T09:46:01.973Z] 09:46:01     INFO - #14: mozilla::dom::PromiseJobCallback::Call(mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) [s3:gecko-generated-sources:8c57d3ccabff4472fa99a26fe36cd82bc81894b551ae7a003e1669a2499168c04192a339fe03f3af8718b555ffdd12f20cb82068072555eefe653f7dc644898f/dist/include/mozilla/dom/PromiseBinding.h::90]
[task 2021-02-22T09:46:01.973Z] 09:46:01     INFO - #15: mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation&) [xpcom/base/CycleCollectedJSContext.cpp:212]
[task 2021-02-22T09:46:01.974Z] 09:46:01     INFO - #16: mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool) [xpcom/base/CycleCollectedJSContext.cpp:648]
[task 2021-02-22T09:46:01.974Z] 09:46:01     INFO - #17: mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) [dom/script/ScriptLoader.cpp:3227]
[task 2021-02-22T09:46:01.974Z] 09:46:01     INFO - #18: mozilla::dom::ScriptLoader::ProcessDynamicImport(mozilla::dom::ModuleLoadRequest*) [dom/script/ScriptLoader.cpp:2710]
[task 2021-02-22T09:46:01.974Z] 09:46:01     INFO - #19: mozilla::dom::ScriptRequestProcessor::Run() [dom/script/ScriptLoader.cpp:1231]
[task 2021-02-22T09:46:01.975Z] 09:46:01     INFO - #20: nsContentUtils::AddScriptRunner(already_AddRefed<nsIRunnable>) [dom/base/nsContentUtils.cpp:5632]
[task 2021-02-22T09:46:01.976Z] 09:46:01     INFO - #21: nsContentUtils::AddScriptRunner(nsIRunnable*) [dom/base/nsContentUtils.cpp:5637]
[task 2021-02-22T09:46:01.977Z] 09:46:01     INFO - #22: mozilla::dom::ScriptLoader::ProcessLoadedModuleTree(mozilla::dom::ModuleLoadRequest*) [dom/script/ScriptLoader.cpp:1247]
[task 2021-02-22T09:46:01.977Z] 09:46:01     INFO - #23: mozilla::dom::ModuleLoadRequest::LoadFinished() [dom/script/ModuleLoadRequest.cpp:208]
[task 2021-02-22T09:46:01.978Z] 09:46:01     INFO - #24: mozilla::MozPromise<CopyableTArray<bool>, nsresult, true>::ThenValue<mozilla::dom::ModuleLoadRequest*, void (mozilla::dom::ModuleLoadRequest::*)(), void (mozilla::dom::ModuleLoadRequest::*)()>::DoResolveOrRejectInternal(mozilla::MozPromise<CopyableTArray<bool>, nsresult, true>::ResolveOrRejectValue&) [xpcom/threads/MozPromise.h:715]
[task 2021-02-22T09:46:01.978Z] 09:46:01     INFO - #25: mozilla::MozPromise<CopyableTArray<bool>, nsresult, true>::ThenValueBase::ResolveOrRejectRunnable::Run() [xpcom/threads/MozPromise.h:488]
[task 2021-02-22T09:46:01.979Z] 09:46:01     INFO - #26: mozilla::RunnableTask::Run() [xpcom/threads/TaskController.cpp:473]
[task 2021-02-22T09:46:01.979Z] 09:46:01     INFO - #27: mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) [xpcom/threads/TaskController.cpp:760]
[task 2021-02-22T09:46:01.980Z] 09:46:01     INFO - #28: mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) [xpcom/threads/TaskController.cpp:611]
[task 2021-02-22T09:46:01.981Z] 09:46:01     INFO - #29: mozilla::TaskController::ProcessPendingMTTask(bool) [xpcom/threads/TaskController.cpp:395]
[task 2021-02-22T09:46:01.981Z] 09:46:01     INFO - #30: mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_3>::Run() [xpcom/threads/nsThreadUtils.h:535]
[task 2021-02-22T09:46:01.982Z] 09:46:01     INFO - #31: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:1162]
[task 2021-02-22T09:46:01.982Z] 09:46:01     INFO - #32: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:548]
[task 2021-02-22T09:46:01.983Z] 09:46:01     INFO - #33: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:87]
[task 2021-02-22T09:46:01.983Z] 09:46:01     INFO - #34: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:335]
[task 2021-02-22T09:46:01.984Z] 09:46:01     INFO - #35: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:311]
[task 2021-02-22T09:46:01.984Z] 09:46:01     INFO - #36: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:139]
[task 2021-02-22T09:46:01.985Z] 09:46:01     INFO - #37: XRE_RunAppShell() [toolkit/xre/nsEmbedFunctions.cpp:902]
[task 2021-02-22T09:46:01.985Z] 09:46:01     INFO - #38: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:237]
[task 2021-02-22T09:46:01.986Z] 09:46:01     INFO - #39: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:335]
[task 2021-02-22T09:46:01.986Z] 09:46:01     INFO - #40: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:311]
[task 2021-02-22T09:46:01.987Z] 09:46:01     INFO - #41: XRE_InitChildProcess(int, char**, XREChildData const*) [toolkit/xre/nsEmbedFunctions.cpp:733]
[task 2021-02-22T09:46:02.027Z] 09:46:02     INFO - #42: content_process_main(mozilla::Bootstrap*, int, char**) [ipc/contentproc/plugin-container.cpp:58]
[task 2021-02-22T09:46:02.031Z] 09:46:02     INFO - #43: main [browser/app/nsBrowserApp.cpp:306]
[task 2021-02-22T09:46:02.032Z] 09:46:02     INFO - #44: __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6 + 0x21b97]
[task 2021-02-22T09:46:02.033Z] 09:46:02     INFO - #45: ??? [/builds/worker/workspace/build/application/firefox/firefox-bin + 0x409e9]
[task 2021-02-22T09:46:02.034Z] 09:46:02     INFO - #46: ??? (???:???)
[task 2021-02-22T09:46:02.035Z] 09:46:02     INFO - [Parent 1440, Breakpad Server] WARNING: Resource acquired is being released in non-LIFO order; why?
[task 2021-02-22T09:46:02.036Z] 09:46:02     INFO - : file /builds/worker/checkouts/gecko/xpcom/threads/BlockingResourceBase.cpp:292
[task 2021-02-22T09:46:02.036Z] 09:46:02     INFO - --- Mutex : dumpSafetyLock (currently acquired)
[task 2021-02-22T09:46:02.037Z] 09:46:02     INFO -  calling context
[task 2021-02-22T09:46:02.038Z] 09:46:02     INFO -   [stack trace unavailable]
Flags: needinfo?(ystartsev)

Also failing: js/src/tests/non262/module/bug1689499.js;module;async | load failed: timed out waiting for reftest-wait to be removed - https://treeherder.mozilla.org/logviewer?job_id=330705449&repo=autoland&lineNumber=35248

Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bf45ac283ab7 Add test to show cycle issue with multiple parents;r=jorendorff https://hg.mozilla.org/integration/autoland/rev/83dcd14e375a fix throwing check in asyncModuleExecutionRejected when reaching an errored module the second time;r=jorendorff https://hg.mozilla.org/integration/autoland/rev/b42024c79b4c Replace GetAsyncCycleRoot with [[CycleRoot]] field;r=jorendorff
Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/90e1590feb31 Add test to show cycle issue with multiple parents;r=jorendorff https://hg.mozilla.org/integration/autoland/rev/f9c985552c46 fix throwing check in asyncModuleExecutionRejected when reaching an errored module the second time;r=jorendorff https://hg.mozilla.org/integration/autoland/rev/a5323b1391bf Replace GetAsyncCycleRoot with [[CycleRoot]] field;r=jorendorff

https://hg.mozilla.org/mozilla-central/rev/90e1590feb31
https://hg.mozilla.org/mozilla-central/rev/f9c985552c46
https://hg.mozilla.org/mozilla-central/rev/a5323b1391bf

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox88: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Flags: needinfo?(ystartsev)
Blocks: 1725621
No longer blocks: 1725621
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: