1815152 - Eager evaluation triggers side effect in async method when awaiting non promises

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Kaiido]

Attachment: test-case.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:109.0) Gecko/20100101 Firefox/109.0

Steps to reproduce:

Define an async function that does await on a non Promise value:

async function fn() {
  await 0;
  alert("that's bad");
}

Then in the console type fn().

Actual results:

The alert got triggered.

Expected results:

I guess the evaluation should have stopped at the await keyword, and certainly it shouldn't have bypassed the "side-effects" white list.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

It looks like this is now fixed in Firefox Nightly (111)
Would you mind to check?

Comment 2

[Kaiido]

Attachment: BUG1815152-screencast.webm

I attach a screencast from today's Nightly build on a macOS 13.2 (22D49) where this still does reproduce.

Comment 3

[Julian Descottes [:jdescottes]]

Thanks for filing! We can reproduce on latest Nightly.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

mozregression gives me this range https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=720d5125a9b4aa6750806ed9b51fbd0811da10c4&tochange=315e476ecf385fa0c9fd395aa1423a26b6a1e1b1

It's not clear which bug would have caused this though

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Note that this only happens when we do await in the function, for example with

async function func() {
  alert("that's bad");
}

doing an eager evaluation with func() does bail (we do get the call to alert in onNativeCall)

But when we await for something we might introduce a slight delay, and we get the pending promise as the result of the eager evaluation, then dismantle our side-effect-free debugger https://searchfox.org/mozilla-central/rev/e7dd9d9ba128478e1ff399778e851365ced3c806/devtools/server/actors/webconsole/eval-with-debugger.js#170-188

let result;
try {
  result = getEvalResult(
    dbg,
    evalString,
    evalOptions,
    bindings,
    frame,
    dbgGlobal,
    noSideEffectDebugger
  );
} finally {
  // We need to be absolutely sure that the sideeffect-free debugger's
  // debuggees are removed because otherwise we risk them terminating
  // execution of later code in the case of unexpected exceptions.
  if (noSideEffectDebugger) {
    noSideEffectDebugger.removeAllDebuggees();
  }
}

I'm not sure what a good solution would be for this. We might await until the promise settles before removing all debuggees, but we might get into cases where promise never settle ?
Note that we have a mechanism to bail if the evaluation takes too long (eval-with-debugger.js), so it might be safe?
Otherwise we may directly bail if the eager evaluation returns a Promise, which could be the simplest fix.

Also, I wonder how Bug 1776376 might have changed the behavior of this, would you have any idea arai?

Comment 6

[Tooru Fujisawa [:arai]]

Yes, bug 1776376 changed the behavior, and that made async function eagerly evaluateable, given JSOp::SetAliasedVar was the last opcode that blocked the eager evaluation of async function.

Then, I guess it was wrong decision to allow async function there.

We treat Promise.prototype.then side-effectful, and in that case we should treat async function and async generator also side-effectful,
because they do the same thing as Promise.prototype.then internally.

I'll look into treating JSOp::Await and JSOp::CanSkipAwait opcodes effectful.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Bug 1815152 - Treat await inside async function effectful in eager evaluation. r?jandem!,nchevobbe!

Comment 8

[Tooru Fujisawa [:arai]]

the patch keeps treating async function eagerly evaluate-able as long as it doesn't execute await inside it.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Based on comment #4, this bug contains a bisection range found by mozregression. However, the Regressed by field is still not filled.

:arai, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit auto_nag documentation.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

This bug has been marked as a regression. Setting status flag for Nightly to affected.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cb216818bb6
Treat await inside async function effectful in eager evaluation. r=jandem,nchevobbe

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6cb216818bb6