Bug 216533 - [WebIDL] %Interface%.prototype.constructor should be defined on [[Set]] receiver
Summary: [WebIDL] %Interface%.prototype.constructor should be defined on [[Set]] receiver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-15 02:21 PDT by bashorov
Modified: 2021-03-09 15:03 PST (History)
18 users (show)

See Also:


Attachments
WIP (385.11 KB, patch)
2020-09-17 09:05 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
WIP (9.20 KB, patch)
2020-09-17 09:31 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (244.99 KB, patch)
2020-10-11 11:30 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (245.15 KB, patch)
2020-10-19 14:12 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description bashorov 2020-09-15 02:21:45 PDT
```
var p = Object.create(HTMLElement.prototype);
p.constructor = "foo";
console.log("!!!" + HTMLElement.prototype.constructor)
```

For Safari it prints:
```
!!!foo
```

For Chrome it prints:
```
!!!function HTMLElement() { [native code] }
```

For non HTML elements it works as expected.
Comment 1 Radar WebKit Bug Importer 2020-09-16 16:41:33 PDT
<rdar://problem/69023868>
Comment 2 Alexey Shvayka 2020-09-17 09:05:50 PDT Comment hidden (obsolete)
Comment 3 Alexey Shvayka 2020-09-17 09:31:31 PDT Comment hidden (obsolete)
Comment 4 Alexey Shvayka 2020-10-11 11:30:41 PDT
Created attachment 411057 [details]
Patch
Comment 5 EWS Watchlist 2020-10-11 11:32:00 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 6 Darin Adler 2020-10-11 11:50:25 PDT
Comment on attachment 411057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411057&action=review

> Source/JavaScriptCore/runtime/CustomGetterSetter.h:79
> +JS_EXPORT_PRIVATE Optional<bool> callCustomSetter(JSGlobalObject*, CustomGetterSetter::CustomSetter, bool isAccessor, JSObject* slotBase, JSValue thisValue, JSValue);

Sam Weinig has argued that Optional<bool> is too confusing a type to ever be used for anything. In this case, I must admit it’s not clear to me what the three different return values are and what they mean. Could just return a custom enumeration to make that clearer?
Comment 7 Alexey Shvayka 2020-10-19 14:12:28 PDT
Created attachment 411796 [details]
Patch

Use TriState instead of Optional<bool>.
Comment 8 Alexey Shvayka 2020-10-19 14:13:18 PDT
(In reply to Darin Adler from comment #6)

Thank you for review, Darin!

> Sam Weinig has argued that Optional<bool> is too confusing a type to ever be
> used for anything. In this case, I must admit it’s not clear to me what the
> three different return values are and what they mean. Could just return a
> custom enumeration to make that clearer?

Does TriState make it clearer?
It's nice that, unlike custom enumeration, it leaves the knowledge on how to handle missing CustomValue setter out of callCustomSetter().
Comment 9 Alexey Shvayka 2020-10-19 14:15:42 PDT
(In reply to Alexey Shvayka from comment #8)
> It's nice that, unlike custom enumeration, it leaves the knowledge on how to
> handle missing CustomValue setter out of callCustomSetter().

This can also be achieved by passing something like CustomGetterSetter::Result::MissingCustomValueSetter.
Comment 10 Darin Adler 2020-10-19 14:21:48 PDT
Comment on attachment 411796 [details]
Patch

Yes, I think the use of TriState makes this clearer. Not as elegant as Optional<bool> but clearer.
Comment 11 EWS 2020-10-19 19:49:12 PDT
Committed r268710: <https://trac.webkit.org/changeset/268710>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411796 [details].
Comment 12 Alexey Shvayka 2021-02-04 05:17:34 PST
(In reply to EWS from comment #11)
> Committed r268710: <https://trac.webkit.org/changeset/268710>

This change reduced WebCore --release binary size by 0.36% (296 KB), and also fixed DontEnum to be preserved when reassigning "constructor":

    HTMLElement.constructor = function() {};
    HTMLElement.propertyIsEnumerable("constructor"); // now: false (correct), was: true

https://bugs.webkit.org/show_bug.cgi?id=217916 adds a test case for this, and also fixes 2 super minor regressions introduced in r268710:

    1. When CustomValue structure property is reassigned, PropertyAttribute::CustomValue is not unset.
       This is unobservable since JSC relies on value being a CustomGetterSetter rather than checking attributes.
    2. When non-reified static setter-less CustomValue of a non-extensible structure is reassigned, TypeError is erroneously thrown.
       There are no such cases in the wild: "constructor" properties are always reified.