1896390 - Ship (synchronous) Iterator Helpers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Dan Minor [:dminor]]

Chrome has been shipping this since 122, they encountered a web compat problem, but that has been worked around. Once we've split out the AsyncIterator stuff (which is Stage 2) and had this fuzzed, I think we can consider shipping our implementation.

Comment 1

[Dan Minor [:dminor]]

Roughly planning this for Firefox 130.

Comment 2

[Dan Minor [:dminor]]

Attachment: Bug 1896390 - Ship iterator helpers; r=mgaudet!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adbd3c6ad6c2
Ship iterator helpers; r=mgaudet,webidl,emilio

Comment 4

[Sandor Molnar[:smolnar]]

Backed out for causing dt failures @ browser_script_command_execute_basic.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/f705b097e3b3f549c827fd417d7accea8a817415

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | devtools/shared/commands/script/tests/browser_script_command_execute_basic.js

Comment 5

[Dan Minor [:dminor]]

I don't really understand what this test is trying to do or if the change here is significant. Nicholas, I was wondering if you could help, or know someone whom I could ask? Thanks!

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Dan Minor [:dminor] from comment #5)

I don't really understand what this test is trying to do or if the change here is significant. Nicholas, I was wondering if you could help, or know someone whom I could ask? Thanks!

This test does some console evaluation, and this specific function is checking for eager evaluation (e.g. the "live" evaluation that is done before you hit Enter) on some specific objects (FormData, Headers and URLSearchParams values() constructor). With your patch, it looks like the constructor changed from Object to Iterator, so the test should reflect that. We also need to indicate to eager evaluation that the Iterator constructor getter is side-effect free. With the following changes, the test is fine:

diff --git a/devtools/server/actors/webconsole/eager-ecma-allowlist.js b/devtools/server/actors/webconsole/eager-ecma-allowlist.js
--- a/devtools/server/actors/webconsole/eager-ecma-allowlist.js
+++ b/devtools/server/actors/webconsole/eager-ecma-allowlist.js
@@ -191,6 +191,7 @@ const getterAllowList = [
   getter(Intl.Locale.prototype, "language"),
   getter(Intl.Locale.prototype, "script"),
   getter(Intl.Locale.prototype, "region"),
+  getter(Iterator.prototype, "constructor"),
   getter(Map.prototype, "size"),
   getter(Map, Symbol.species),
   // NOTE: Object.prototype.__proto__ is not safe, because it can internally
diff --git a/devtools/shared/commands/script/tests/browser_script_command_execute_basic.js b/devtools/shared/commands/script/tests/browser_script_command_execute_basic.js
--- a/devtools/shared/commands/script/tests/browser_script_command_execute_basic.js
+++ b/devtools/shared/commands/script/tests/browser_script_command_execute_basic.js
@@ -1012,9 +1012,9 @@ async function doEagerEvalDOMMethods(com
   // JitInfo, while ReadableStream's "values" isn't side-effect free.
 
   const testDataAllowed = [
-    [`testFormData.values().constructor.name`, "Object"],
-    [`testHeaders.values().constructor.name`, "Object"],
-    [`testURLSearchParams.values().constructor.name`, "Object"],
+    [`testFormData.values().constructor.name`, "Iterator"],
+    [`testHeaders.values().constructor.name`, "Iterator"],
+    [`testURLSearchParams.values().constructor.name`, "Iterator"],
   ];
 
   for (const [code, expectedResult] of testDataAllowed) {

But I'm not sure if this still covers the original intent of the test : The following "values" methods share single native function with different JitInfo
Is this still the case with your patch?

arai might have some input on this as well

Comment 7

[Tooru Fujisawa [:arai]]

The intent of the test is to verify whether the values() call is blocked or not, and the .constructor.name property access is just to make the comparison easier, so it doesn't actually have to be "Object" string, but receiving "Iterator" is also okay.

But the problem is that, Iterator global variable exists only if the Iterator Helpers are enabled, which is still controllable via pref, which means the Iterator.prototype expression throws ReferenceError if the pref is turned off.

So, unconditionally putting getter(Iterator.prototype, "constructor"), in eager-ecma-allowlist.js doesn't work.
(the console will stop working if the pref is turned off)

If the constructor getter is really side-effect-free, possible workaround is something like the following, in eager-ecma-allowlist.js:

if (typeof Iterator === "function") {
  getterAllowList.push(
    getter(Iterator.prototype, "constructor")
  );
}

If the function is not side-effect-free, then the test needs to be rewritten to use other native function (it doesn't have to be values).
In that case, feel free to disable the test for now with a followup bug filed. I'll address it.

---

Then, the comment about the shared native is still valid.
Here's the details about the natives, comparison, and the behavior difference:

In eager evaluation, we check if given function is side-effect-free, by comparing it with functions in allowlist. There are 2 separate allowlist, one is for Web API, defined in webidl-pure-allowlist.js (generated by GenerateDataFromWebIdls.py), and the other is for ECMAScript built-ins, defined in eager-ecma-allowlist.js.

The check for the allowlist is done by comparing the JSNative and the JSJitInfo against the function retrieved in unmodified realm.
(We cannot compare JSFunction itself because it differs for each realm)

https://searchfox.org/mozilla-central/rev/e637cd67f98ed4272d13a96db9a7674689122dcb/devtools/server/actors/webconsole/eval-with-debugger.js#620

return natives && natives.some(n => fn.isSameNativeWithJitInfo(n));

The reason for using JSJitInfo is that, binding functions for Web API share single JSNative, with the actual implementation stored inside JSJitInfo (JSJitMethodOp).

Those APIs get the following bindings, generated by bindgen:

https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/FormDataBinding.cpp#1159-1161,1174,1176,1298

namespace FormData_Binding {
...
static const JSTypedMethodJitInfo values_methodinfo = {
  {
    { (JSJitGetterOp)values },
...
  },
...
};
...
JS_FNSPEC("values", (GenericMethod<NormalThisPolicy, ThrowExceptions>), reinterpret_cast<const JSJitInfo*>(&values_methodinfo), 0, JSPROP_ENUMERATE, nullptr),

https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/ReadableStreamBinding.cpp#950-952,965,967,1039

namespace ReadableStream_Binding {
...
static const JSTypedMethodJitInfo values_methodinfo = {
  {
    { (JSJitGetterOp)values },
...
  },
...
};
...
JS_FNSPEC("values", (GenericMethod<NormalThisPolicy, ThrowExceptions>), reinterpret_cast<const JSJitInfo*>(&values_methodinfo), 0, JSPROP_ENUMERATE, nullptr),

There, GenericMethod<NormalThisPolicy, ThrowExceptions> is the shared JSNative for them, and JSJitInfo is different for each.

Then, the test starts failing because of the values() behavior changes for FormData etc.
The FormData case (side-effect-free case) creates a FormDataIterator instance.

https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/FormDataBinding.cpp#1137-1139,1145-1148,1151

MOZ_CAN_RUN_SCRIPT static bool
values(JSContext* cx, JS::Handle<JSObject*> obj, void* void_self, const JSJitMethodCallArgs& args)
{
...
  auto* self = static_cast<mozilla::dom::FormData*>(void_self);
  typedef mozilla::dom::binding_detail::WrappableIterableIterator<mozilla::dom::FormData, &FormDataIterator_Binding::Wrap> itrType;
  RefPtr<itrType> result(new itrType(self,
                                     itrType::IteratorType::Values));
...
  if (!WrapNewBindingNonWrapperCachedObject(cx, obj, result, args.rval())) {

The FormDataIterator has JS::GetRealmIteratorPrototype(aCx) as prototype,
where it now returns Iterator prototype.

https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/FormDataBinding.cpp#1657,1843-1845,1849,1854

namespace FormDataIterator_Binding {
...
static void
CreateInterfaceObjects(JSContext* aCx, JS::Handle<JSObject*> aGlobal, ProtoAndIfaceCache& aProtoAndIfaceCache, bool aDefineOnGlobal)
{
...
  JS::Rooted<JSObject*> parentProto(aCx, JS::GetRealmIteratorPrototype(aCx));
...
  dom::CreateInterfaceObjects(aCx, aGlobal, parentProto,

https://searchfox.org/mozilla-central/rev/e637cd67f98ed4272d13a96db9a7674689122dcb/js/src/vm/GlobalObject.cpp#1024-1037

JSObject* GlobalObject::createIteratorPrototype(JSContext* cx,
                                                Handle<GlobalObject*> global) {
  if (!IsIteratorHelpersEnabled()) {
    return getOrCreateBuiltinProto(cx, global, ProtoKind::IteratorProto,
                                   initIteratorProto);
  }

  if (!ensureConstructor(cx, global, JSProto_Iterator)) {
    return nullptr;
  }
  JSObject* proto = &global->getPrototype(JSProto_Iterator);
  global->initBuiltinProto(ProtoKind::IteratorProto, proto);
  return proto;
}

The ReadableStream case (side-effectful case) creates ReadableStreamAsyncIterator, which has JS::GetRealmAsyncIteratorPrototype(aCx) as prototype, thus this remains plain object.

https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/ReadableStreamBinding.cpp#916-918,925,930-932,942

MOZ_CAN_RUN_SCRIPT static bool
values(JSContext* cx_, JS::Handle<JSObject*> obj, void* void_self, const JSJitMethodCallArgs& args)
{
...
  auto* self = static_cast<mozilla::dom::ReadableStream*>(void_self);
...
  typedef mozilla::dom::binding_detail::WrappableAsyncIterableIterator<mozilla::dom::ReadableStream, true, &ReadableStreamAsyncIterator_Binding::Wrap> itrType;
  RefPtr<itrType> result(new itrType(self,
                                     itrType::IteratorType::Values));
...
  if (!WrapNewBindingNonWrapperCachedObject(cx, obj, result, args.rval())) {

https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/ReadableStreamBinding.cpp#1646-1648,1652

static void
CreateInterfaceObjects(JSContext* aCx, JS::Handle<JSObject*> aGlobal, ProtoAndIfaceCache& aProtoAndIfaceCache, bool aDefineOnGlobal)
{
...
  JS::Rooted<JSObject*> parentProto(aCx, JS::GetRealmAsyncIteratorPrototype(aCx));

Comment 8

[Dan Minor [:dminor]]

Attachment: Bug 1896390 - Add getter for Iterator.constructor to eager-ecma-allowlist.js; r=nchevobbe!

Comment 9

[Dan Minor [:dminor]]

Since it's now the day before soft-freeze, I'm going to hold off on this until Firefox 131.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f85ed29d06c7
Ship iterator helpers; r=mgaudet,webidl,emilio
https://hg.mozilla.org/integration/autoland/rev/d775409d7c6a
Add getter for Iterator.constructor to eager-ecma-allowlist.js; r=nchevobbe,devtools-reviewers,arai

Comment 11

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/f85ed29d06c7
https://hg.mozilla.org/mozilla-central/rev/d775409d7c6a