1881359 - Eager evaluation shouldn't evaluate ReadableStream.values()

Status: VERIFIED FIXED

(DevTools :: Console, defect, P2)

Description

[Mattias Buelens]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:122.0) Gecko/20100101 Firefox/122.0

Steps to reproduce:

1. Open the Console tab of Firefox DevTools.
2. Type readable = new ReadableStream(), then press Enter.
3. Type readable.values() without pressing Enter.

Actual results:

DevTools shows the following autocomplete hint: "ReadableStream AsyncIterator { }"

When you then press Enter, it throws an error:

Uncaught TypeError: Asynchronous iterator initialization steps for ReadableStream failed: Cannot get a new reader for a readable stream already locked by another reader.

I assume this happens because the autocomplete hint already created a ReadableStream AsyncIterator, which has the side effect of locking the stream.

Expected results:

The debugger should recognize that calling readable.values() has side effects, and should not show an autocomplete hint.

After you press Enter, it should construct a valid ReadableStream AsyncIterator.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'DevTools::Debugger' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Thanks for the report Mattias, I can reproduce easily

Comment 3

[Julian Descottes [:jdescottes]]

Nicolas will have a look at this one.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

I'm not sure what's happening, but the eager function call gets "validated" because we return true from https://searchfox.org/mozilla-central/rev/9cd4ea81e27db6b767f1d9bbbcf47da238dd64fa/devtools/server/actors/webconsole/eval-with-debugger.js#584,609-611

function nativeIsEagerlyEvaluateable(fn) {
...
  const natives = gSideEffectFreeNatives.get(fn.name);
  return natives && natives.some(n => fn.isSameNative(n));
}

Here, fn.name is values, so I tried to remove values from https://searchfox.org/mozilla-central/rev/9cd4ea81e27db6b767f1d9bbbcf47da238dd64fa/devtools/server/actors/webconsole/webidl-pure-allowlist.js#50-55,84-86

FormData: {
  prototype: ["entries", "keys", "values"],
},
Headers: {
  prototype: ["entries", "keys", "values"],
},
...
URLSearchParams: {
  prototype: ["entries", "keys", "values"],
},

Without the values entries in those, the problem is gone.
It looks like this is an issue from Debugger.Object.isSameNative, or with the values function itself that is "shared" between those different classes?

arai, I could use some help here, do you understand what's going on?

Comment 5

[Tooru Fujisawa [:arai]]

We don't differentiate multiple methods with same native, and apparently most of DOM methods are using mozilla::dom::binding_detail::GenericMethod template function, which means many unrelated JS functions share single native function, which means the function name isn't sufficient as condition, especially for methods with generic name such as "values".

I think we need to apply the same rule as getters, that also compares JitInfo as well.

Comment 6

[Tooru Fujisawa [:arai]]

err, I misremembered about the situation for getters. we just allow all getters.

anyway, I'll look into adding JitInfo as condition.

Comment 7

[Tooru Fujisawa [:arai]]

Actually, we had the method once added for this, for getters: https://phabricator.services.mozilla.com/D168336 and then removed https://phabricator.services.mozilla.com/D175091 when optimizing getters.
and this is a kind of regression from the latter change, given it had also been handling methods with GenericMethod.
we should've kept using isSameNativeWithJitInfo to handle methods in allowlist.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Bug 1881359 - Part 1: Re-add DebuggerObject.prototype.isSameNativeWithJitInfo. r=jandem!

This just reverts bug 1831826 change.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Bug 1881359 - Part 2: Use DebuggerObject.prototype.isSameNativeWithJitInfo in eager evaluation to support methods with GenericMethod native function. r=nchevobbe!

This reverts a part of bug Bug 1815381 change.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b02314c7599e
Part 1: Re-add DebuggerObject.prototype.isSameNativeWithJitInfo. r=jandem
https://hg.mozilla.org/integration/autoland/rev/de9ecee6742f
Part 2: Use DebuggerObject.prototype.isSameNativeWithJitInfo in eager evaluation to support methods with GenericMethod native function. r=nchevobbe,devtools-reviewers

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b02314c7599e
https://hg.mozilla.org/mozilla-central/rev/de9ecee6742f

Comment 12

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2024-02-21 Nightly build on macOS 12.
Verified as fixed on Firefox Nightly 126.0a1 and Firefox 125.0 on macOS 12, Windows 10, Ubuntu 22.