Closed Bug 1881359 Opened 2 months ago Closed 2 months ago

Eager evaluation shouldn't evaluate ReadableStream.values()

Categories

(DevTools :: Console, defect, P2)

Firefox 122
defect

Tracking

(firefox125 verified, firefox126 verified)

VERIFIED FIXED
125 Branch
Tracking Status
firefox125 --- verified
firefox126 --- verified

People

(Reporter: mattias, Assigned: arai)

References

Details

Attachments

(2 files)

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.

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.

Component: Untriaged → Debugger
Product: Firefox → DevTools

Thanks for the report Mattias, I can reproduce easily

Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: DevTools autocomplete evaluates ReadableStream.values() too soon → Eager evaluation shouldn't evaluate ReadableStream.values()
Component: Debugger → Console

Nicolas will have a look at this one.

Severity: -- → S3
Flags: needinfo?(nchevobbe)
Priority: -- → P2

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?

Flags: needinfo?(nchevobbe) → needinfo?(arai.unmht)

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.

Flags: needinfo?(arai.unmht)

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

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

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.

Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Regressions: 1815381
Pushed by arai_a@mac.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 125 Branch
QA Whiteboard: [qa-125b-p2]

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.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-125b-p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: