Attempting to load MigrationUtils from the Browser Console before the wizard is opened breaks MigrationUtils in that window's scope
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
People
(Reporter: mconley, Assigned: arai)
References
(Blocks 1 open bug)
Details
I'm not 100% certain this is the right place to file this, but seemed like a reasonable place to start. Obviously, feel free to move it somewhere more appropriate.
STR:
- Launch Firefox
- Open up the Browser Console
- Type in
MigrationUtilsand press Enter
ER:
MigrationUtils, a lazily-loaded ESM that's prepared here, should load and then we should be able to inspect and manipulate the exported object (the MigrationUtils singleton instance that is exported as MigrationUtils).
AR:
I get an "Uncaught undefined" error in the Browser Console, and then opening the Migration Wizard (via File > Import From Another Browser) ends up being broken in that browser window.
Interestingly, if I open the Migration Wizard first, then MigrationUtils loads just fine. That's strange, because effectively, it's also de-lazifying MigrationUtils via a XUL command here. If I run this command from the Browser Console manually:
MigrationUtils.showMigrationWizard(window, [MigrationUtils.MIGRATION_ENTRYPOINTS.FILE_MENU]);
after a restart, then I get the failure... so it seems that the XUL command can cause MigrationUtils to properly import if it's the first one to try it.
Any idea what's going on here?
|
Reporter | |
Updated•2 years ago
|
|
||
Comment 2•2 years ago
|
||
It looks like arai is currently looking into this.
|
Assignee | |
Comment 3•2 years ago
|
||
So far, what I confirmed is the following:
- the promise for the module evaluation is rejected
- the reason value of the rejection is
undefined
nsresult mozJSModuleLoader::ImportESModule(
...
rv = mModuleLoader->EvaluateModuleInContext(aCx, request,
nsresult ModuleLoaderBase::EvaluateModuleInContext(
...
if (!JS::ThrowOnModuleEvaluationFailure(aCx, evaluationPromise,
errorBehaviour)) {
JS_PUBLIC_API bool JS::ThrowOnModuleEvaluationFailure(
...
return OnModuleEvaluationFailure(cx, evaluationPromise, errorBehaviour);
bool js::OnModuleEvaluationFailure(JSContext* cx,
...
RootedValue error(cx, JS::GetPromiseResult(evaluationPromise));
JS_SetPendingException(cx, error);
I'll check why the promise is rejected with undefined.
|
|
Assignee | |
Updated•2 years ago
|
|
|
Assignee | |
Comment 4•2 years ago
|
||
This is caused by a bad interaction between:
- caching the module evaluation result
- eager evaluation in the console
setting devtools.webconsole.input.eagerEvaluation to false makes the issue disappear.
the details are the following:
eager evaluation installs hooks into the each JS code that's invoked during the evaluation.
the hook catches each effectful opcodes, and it aborts the evaluation with unhandlable exception.
(which becomes undefined in the thrown value)
const handler = {
hit: () => null,
};
dbg.onEnterFrame = frame => {
if (shouldCancel()) {
return null;
}
frame.onStep = () => {
if (shouldCancel()) {
return null;
}
return undefined;
};
const script = frame.script;
if (executedScripts.has(script)) {
return undefined;
}
executedScripts.add(script);
const offsets = script.getEffectfulOffsets();
for (const offset of offsets) {
script.setBreakpoint(offset, handler);
}
Then, when MigrationUtils is typed into the console, eager evaluation happens before Enter key gets hit.
the eager evaluation calls the lazy getter, and the module import happens, and the evaluation gets aborted during evaluating MigrationUtils.sys.mjs's top-level code, and the module is marked as hadEvaluationError.
when Enter key is hit, the lazy getter is called again but the module evaluation is skipped because of hadEvaluationError,
and the result from the previous error undefined is thrown.
|
|
Assignee | |
Comment 5•2 years ago
|
||
there are multiple issues here:
- the lazy getter or the module import is not considered "effectful" in the eager evaluation and the function is called
- the module is marked
hadEvaluationErroreven with unhandlable exception
|
|
Assignee | |
Comment 6•2 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #5)
there are multiple issues here:
- the lazy getter or the module import is not considered "effectful" in the eager evaluation and the function is called
about the lazy getter part, the eager evaluation doesn't consider any getter effectful.
// Getters are never considered effectful, and setters are always effectful.
then, here the implementation of the getter is native function that calls ImportESModule, and there's no other place to abort the execution until the target module's top-level script.
JS::Rooted<JSObject*> getter(
aCx, JS_GetFunctionObject(js::NewFunctionByIdWithReserved(
aCx, ESModuleGetter, 0, 0, aId)));
static bool ModuleGetterImpl(JSContext* aCx, unsigned aArgc, JS::Value* aVp,
ModuleType aType) {
...
nsresult rv = moduleloader->ImportESModule(aCx, uri, &moduleNamespace);
|
|
Assignee | |
Comment 7•2 years ago
|
||
:jonco, is it possible to handle the case where "the module's evaluation gets aborted by debugger" and avoid marking the module hadEvaluationError, and make it possible to re-evaluate the module on the next import?
|
|
Assignee | |
Comment 8•2 years ago
|
||
:nchevobbe, can I have your opinion about the "Getters are never considered effectful" assumption below and this issue?
// Getters are never considered effectful, and setters are always effectful.
// Natives called normally are handled with an allowlist.
if (
reason == "get" ||
(reason == "call" && nativeHasNoSideEffects(callee))
to my understanding, the assumption is wrong, and reason == "get" case should be removed, or allowlist-based filtering should be applied.
also, do you know how may cases takes the reason == "get" path in practice?
is it plausible to generate allowlist, or is it hard to achieve?
(In reply to Tooru Fujisawa [:arai] from comment #8)
:nchevobbe, can I have your opinion about the "Getters are never considered effectful" assumption below and this issue?
to my understanding, the assumption is wrong, andreason == "get"case should be removed, or allowlist-based filtering should be applied.also, do you know how may cases takes the
reason == "get"path in practice?
I wasn't sure from the top of my head, but with some logging, I can see that we do have cases where this is triggered, like globalThis.document
is it plausible to generate allowlist, or is it hard to achieve?
My guess is that it would not be super easy but still doable ?
We have a couple manually-written allow list (https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole/eager-ecma-allowlist.js and https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole/eager-function-allowlist.js), as well as a dynamic one, https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole/webidl-pure-allowlist.js, generated via https://searchfox.org/mozilla-central/source/devtools/shared/webconsole/GenerateDataFromWebIdls.py which gather data from the webidl files.
If we take the globalThis.document case again, it looks like this is tagged as Pure in the window webidl (https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/dom/webidl/Window.webidl#213) so this could work for this specific case (I checked a few other ones too which seem okay).
I hope this answers your question, let me know if I missed something
|
||
Updated•2 years ago
|
|
||
Comment 10•2 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #7)
:jonco, is it possible to handle the case where "the module's evaluation gets aborted by debugger" and avoid marking the module
hadEvaluationError, and make it possible to re-evaluate the module on the next import?
Sorry for the slow reply.
I can see two problems with this: working out when it's the debugger that has aborted execution and not another uncatchable exception (e.g. OOM), and resetting evaluation state when this happens. For modules we transition strongly connected module cycles between states at the same time so this is not trivial, especially considering evaluation has side effects. Hopefully we don't have to do this.
|
|
Assignee | |
Comment 11•2 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
We have a couple manually-written allow list (https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole/eager-ecma-allowlist.js and https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole/eager-function-allowlist.js), as well as a dynamic one, https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole/webidl-pure-allowlist.js, generated via https://searchfox.org/mozilla-central/source/devtools/shared/webconsole/GenerateDataFromWebIdls.py which gather data from the webidl files.
If we take the
globalThis.documentcase again, it looks like this is tagged asPurein the window webidl (https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/dom/webidl/Window.webidl#213) so this could work for this specific case (I checked a few other ones too which seem okay).I hope this answers your question, let me know if I missed something
Great :)
I'll look into generating allowlist for native getters.
(In reply to Jon Coppeard (:jonco) (PTO until 3rd Jan) from comment #10)
I can see two problems with this: working out when it's the debugger that has aborted execution and not another uncatchable exception (e.g. OOM), and resetting evaluation state when this happens. For modules we transition strongly connected module cycles between states at the same time so this is not trivial, especially considering evaluation has side effects. Hopefully we don't have to do this.
Good point. if the execution goes beyond side-effect before debugger aborts the execution, resetting the evaluation won't work.
I was thinking only the eager evaluation case where any side-effects inside JS code are prevented by debugger itself, but this is not safe assumption, and I agree that this is hard to achieve.
|
||
Comment 12•2 years ago
•
|
||
(In reply to Tooru Fujisawa [:arai] from comment #11)
Great :)
I'll look into generating allowlist for native getters.
Great, I filed Bug 1806598 for this work, in the end it might help us for other areas in DevTools :)
|
|
Assignee | |
Comment 13•2 years ago
|
||
Confirmed the expected behavior after bug 1806598 fix.
Description
•