Named properties object: allow symbols in [[DefineOwnProperty]] and [[Delete]] #963

shvaikalesh · 2021-03-11T00:33:38Z

Given that X is named properties object for Window, the following code:

Object.defineProperty(X, Symbol.toStringTag,
  Object.getOwnPropertyDescriptor(X, Symbol.toStringTag));

should throw TypeError per current spec. While being compliant with invariants of EIM, this behavior is very weird.

The same snippet for X that is module namespace object doesn't throw, even though the namespace is non-extensible and its Symbol.toStringTag is non-configurable.

While we could just special-case Symbol.toStringTag or own properties that are symbols, I suggest we treat symbols in [[DefineOwnProperty]] and [[Delete]] the same way as ordinary methods do. To prevent clashes with supported property names, it's critical for WindowProperties to prohibit expando properties that are strings, but not symbols.

Proposed change feels like an expected behavior, reducing divergence with ordinary methods, and aligns named properties object with legacy platforms objects & module namespace object. It is already implemented in Blink and also, with bug #222918, in WebKit.

Currently, no browser features spec-perfect WindowProperties object, so this change can be implemented along with fixing current spec incompatibilities.

Tests: web-platform-tests/wpt#27970.

cc @domenic @evilpie

…[Delete]]

shvaikalesh · 2021-03-11T00:34:43Z

index.bs


-    1.  Return <emu-val>false</emu-val>.
+    1.  If [$Type$](|P|) is String, then return <emu-val>false</emu-val>.
+    1.  Otherwise, return [=?=] [$OrdinaryDelete$](|O|, |P|).


Unlike in legacy platform objects, we can call OrdinaryDelete instead of reimplementing it, as it's reachable only for own symbol properties. I mildly prefer calling ordinary methods so the implementations can match the spec more closely.

evilpie · 2021-03-11T14:25:26Z

@annevk do we want this in Firefox? I think this would mean creating an expando object for WindowNamedProperties, which we currently don't have.

annevk · 2021-03-11T14:56:47Z

I would be fine keeping this as-is personally. Roughly what I care about for these legacy objects is:

Not breaking compat or security.
Interop.
Agreement with ECMAScript on the object model.

And although this is a bit of 2, that could also be achieved by Chromium changing.

@yuki3 was this intentional on the side of Chromium?

shvaikalesh · 2021-03-11T15:20:15Z

I think this would mean creating an expando object for WindowNamedProperties, which we currently don't have.

Creating an expando object for WindowNamedProperties can be avoided if we leave [[Delete]] and [[DefineOwnProperty]] as is, but change Symbol.toStringTag to be non-configurable like it's done for module namespace object.

Agreement with ECMAScript on the object model.

It feels a bit of 3 to me.

yuki3 · 2021-03-12T08:40:22Z

@yuki3 was this intentional on the side of Chromium?

About named properties and indexed properties, we're struggling to implement them correctly due to unique characteristics of V8 APIs (which sometimes do not have a clear mapping to ECMAScript spec). Clearly not all behaviors are implemented on purpose.

I don't fully understand the issue and cannot say what's the best. If necessary, I'm happy to change the Chromium's behaviors.

annevk · 2021-03-12T08:50:31Z

@shvaikalesh could you elaborate on why the specified semantics are weird with respect to the invariants?

evilpie · 2021-03-12T09:43:46Z

I like the idea of making Symbol.toStringTag non-configurable. Right now it's non-writable, but configurable in Firefox. Making it non-configurable would make it more consistent with the always throwing [[DefineOwnProperty]].

shvaikalesh · 2021-03-12T12:12:48Z

Given the following example:

<!DOCTYPE html>
<form name=foo></form>
<script>
const wp = Object.getPrototypeOf(Window.prototype);
wp.foo = 1;
</script>

@yuki3 Is it possible to make the [[Set]] ignored? Currently it is an interop issue: Firefox throws (it should't since no "use strict" there, but ok), while Chrome creates expando foo property that shadows <form name=foo>.

@annevk current spec does comply to invariants, but makes Object.defineProperty unnecessarily weird. Module namespace object's [[DefineOwnProperty]] used to return false unconditionally in ES6, but was later relaxed to be inline with ordinary counterpart.

In perfect world, it would be nice to do the same for WindowProperties. I don't really have hard opinion on whether symbols should be allowed or what descriptors should they have.

On implementation side, WebCore uses JavaScriptCore classes directly, without the API, so it's possible to implement anything that can be spec-ed.

yuki3 · 2021-03-15T10:54:45Z

@yuki3 Is it possible to make the [[Set]] ignored? Currently it is an interop issue: Firefox throws (it should't since no "use strict" there, but ok), while Chrome creates expando foo property that shadows <form name=foo>.

To my best understanding, this is simply a bug. We're forbidding Object.defineProperty(wp, 'foo', {value: 1}) on purpose, but somehow assignments are mis-implemented. I'm now fixing this issue.

IIUC, [[Set]] in this case will fallback to [[DefineOwnProperty]] and the property won't be defined. Correct me if I'm wrong.

V8 interceptor's setter doesn't fallback to definer automatically, so we have to define both of setter and definer interceptors if we'd like to forbid properties to be set/defined on a named properties object. whatwg/webidl#963 Change-Id: I5386ffaac06c2275fd847c7d47cec87054a138d6

V8 interceptor's setter doesn't fallback to definer automatically, so we have to define both of setter and definer interceptors if we'd like to forbid properties to be set/defined on a named properties object. whatwg/webidl#963 Change-Id: I5386ffaac06c2275fd847c7d47cec87054a138d6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2759574 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/master@{#862830}

petervanderbeken · 2021-03-15T19:39:11Z

Firefox throws (it should't since no "use strict" there, but ok)

The throwing without use strict is just a bug I think, we'll fix that.

In perfect world, it would be nice to do the same for WindowProperties. I don't really have hard opinion on whether symbols should be allowed or what descriptors should they have.

I don't feel great about fiddling with some of these legacy objects, but it doesn't seem too complicated for us to change this in Gecko I think (limited to symbols). Making @@toStringTag non-configurable is fine by me too.

V8 interceptor's setter doesn't fallback to definer automatically, so we have to define both of setter and definer interceptors if we'd like to forbid properties to be set/defined on a named properties object. whatwg/webidl#963 Change-Id: I5386ffaac06c2275fd847c7d47cec87054a138d6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2759574 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/master@{#862830}

yuki3 · 2021-03-16T06:26:56Z

@yuki3 Is it possible to make the [[Set]] ignored? Currently it is an interop issue: Firefox throws (it should't since no "use strict" there, but ok), while Chrome creates expando foo property that shadows <form name=foo>.

To my best understanding, this is simply a bug. We're forbidding Object.defineProperty(wp, 'foo', {value: 1}) on purpose, but somehow assignments are mis-implemented. I'm now fixing this issue.

I've landed a fix. Chromium (tip of tree) is fixed. Canary (nightly) version containing the fix will be shipped in a day or two.

…ed properties object, a=testonly Automatic update from web-platform-tests v8bindings: Fix indexed/named set on named properties object V8 interceptor's setter doesn't fallback to definer automatically, so we have to define both of setter and definer interceptors if we'd like to forbid properties to be set/defined on a named properties object. whatwg/webidl#963 Change-Id: I5386ffaac06c2275fd847c7d47cec87054a138d6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2759574 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/master@{#862830} -- wpt-commits: 398cdd520373bd27384419b6926be816637994e8 wpt-pr: 28078

shvaikalesh · 2021-05-20T18:36:10Z

Thank you @yuki3, it's a great fix that covers all [[Set]] edge cases (like setters upper in prototype chain).

IIUC, [[Set]] in this case will fallback to [[DefineOwnProperty]] and the property won't be defined.

That's right, per step 3 of OrdinarySet.

I've decided not to pursue this change: if we make @@toStringTag non-configurable, named properties' [[DefineOwnProperty]] would still need to be updated to accept descriptors like:

{}
{ value: "WindowProperties" }
{ configurable: false }

And even if we fix symbols, [[DefineOwnProperty]] would still remain weird (IMO) for existing string properties, while [[Delete]] would still fail for non-existing string properties (in perfect world, it shouldn't).

Instead, let's reach interop with the current spec. I've updated the tests and reported bugs regarding remaining minor issues:

V8 interceptor's setter doesn't fallback to definer automatically, so we have to define both of setter and definer interceptors if we'd like to forbid properties to be set/defined on a named properties object. whatwg/webidl#963 Change-Id: I5386ffaac06c2275fd847c7d47cec87054a138d6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2759574 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/master@{#862830} GitOrigin-RevId: f9ea840de1d561c3c95541c183a4f8affcf9b74e

Named properties object: allow symbols in [[DefineOwnProperty]] and […

464a1a5

…[Delete]]

shvaikalesh commented Mar 11, 2021

View reviewed changes

chromium-wpt-export-bot mentioned this pull request Mar 15, 2021

v8bindings: Fix indexed/named set on named properties object web-platform-tests/wpt#28078

Merged

shvaikalesh closed this May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Named properties object: allow symbols in [[DefineOwnProperty]] and [[Delete]] #963

Named properties object: allow symbols in [[DefineOwnProperty]] and [[Delete]] #963

shvaikalesh commented Mar 11, 2021

shvaikalesh Mar 11, 2021

evilpie commented Mar 11, 2021

annevk commented Mar 11, 2021

shvaikalesh commented Mar 11, 2021

yuki3 commented Mar 12, 2021

annevk commented Mar 12, 2021

evilpie commented Mar 12, 2021

shvaikalesh commented Mar 12, 2021 •

edited

Loading

yuki3 commented Mar 15, 2021

petervanderbeken commented Mar 15, 2021

yuki3 commented Mar 16, 2021

shvaikalesh commented May 20, 2021 •

edited

Loading

Named properties object: allow symbols in [[DefineOwnProperty]] and [[Delete]] #963

Named properties object: allow symbols in [[DefineOwnProperty]] and [[Delete]] #963

Conversation

shvaikalesh commented Mar 11, 2021

shvaikalesh Mar 11, 2021

Choose a reason for hiding this comment

evilpie commented Mar 11, 2021

annevk commented Mar 11, 2021

shvaikalesh commented Mar 11, 2021

yuki3 commented Mar 12, 2021

annevk commented Mar 12, 2021

evilpie commented Mar 12, 2021

shvaikalesh commented Mar 12, 2021 • edited Loading

yuki3 commented Mar 15, 2021

petervanderbeken commented Mar 15, 2021

yuki3 commented Mar 16, 2021

shvaikalesh commented May 20, 2021 • edited Loading

shvaikalesh commented Mar 12, 2021 •

edited

Loading

shvaikalesh commented May 20, 2021 •

edited

Loading