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:

1. Launch Firefox
2. Open up the Browser Console
3. Type in MigrationUtils and 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?

Yulia, are you or arai investigating this bug?

It looks like arai is currently looking into this.

So far, what I confirmed is the following:

• the promise for the module evaluation is rejected
• the reason value of the rejection is undefined

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/js/xpconnect/loader/mozJSModuleLoader.cpp#1777,1851

nsresult mozJSModuleLoader::ImportESModule(
...
  rv = mModuleLoader->EvaluateModuleInContext(aCx, request,

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/js/loader/ModuleLoaderBase.cpp#1160,1239-1240

nsresult ModuleLoaderBase::EvaluateModuleInContext(
...
    if (!JS::ThrowOnModuleEvaluationFailure(aCx, evaluationPromise,
                                            errorBehaviour)) {

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/js/src/vm/Modules.cpp#165,172

JS_PUBLIC_API bool JS::ThrowOnModuleEvaluationFailure(
...
  return OnModuleEvaluationFailure(cx, evaluationPromise, errorBehaviour);

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/js/src/builtin/ModuleObject.cpp#2325,2345-2346

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.

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)

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/devtools/server/actors/webconsole/eval-with-debugger.js#364-388

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.

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 hadEvaluationError even with unhandlable exception

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

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/devtools/server/actors/webconsole/eval-with-debugger.js#399

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

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/dom/base/ChromeUtils.cpp#790-792

JS::Rooted<JSObject*> getter(
    aCx, JS_GetFunctionObject(js::NewFunctionByIdWithReserved(
             aCx, ESModuleGetter, 0, 0, aId)));

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/dom/base/ChromeUtils.cpp#652-653,695

static bool ModuleGetterImpl(JSContext* aCx, unsigned aArgc, JS::Value* aVp,
                             ModuleType aType) {
...
    nsresult rv = moduleloader->ImportESModule(aCx, uri, &moduleNamespace);

: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?

:nchevobbe, can I have your opinion about the "Getters are never considered effectful" assumption below and this issue?

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/devtools/server/actors/webconsole/eval-with-debugger.js#399-403

// 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, 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?

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

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

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

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.

(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 :)

Confirmed the expected behavior after bug 1806598 fix.