Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider removing call to named property setter in legacy platform object [[Set]] #630

Open
Ms2ger opened this issue Jan 23, 2019 · 5 comments · May be fixed by #715
Open

Consider removing call to named property setter in legacy platform object [[Set]] #630

Ms2ger opened this issue Jan 23, 2019 · 5 comments · May be fixed by #715

Comments

@Ms2ger
Copy link
Member

Ms2ger commented Jan 23, 2019

Test:

<!DOCTYPE html>
<script>
sessionStorage.clear()
var a = new Proxy(Object.getPrototypeOf(sessionStorage), {
  set(...args) { w(args); return false; },
});
Object.setPrototypeOf(sessionStorage, a);
sessionStorage.x = "foo";
w(sessionStorage.x)
</script>

Gecko and Chrome call proto.[[Set]]() rather than taking the early return, as shown by the log:

log: object "[object Storage],x,foo,[object Storage]" (4 props: 0="[object Storage]", 1="x", 2="foo", 3="[object Storage]")
log: undefined

but WebKit may be following the spec:

log: foo
@bzbarsky
Copy link
Collaborator

Hmm. It's possible this [[Set]] algorithm was written or changed after the Gecko implementation. I don't remember the exact history here. It's worth looking up the history of this part of the spec to see what the reasoning for it was, exactly.

@Ms2ger
Copy link
Member Author

Ms2ger commented Jan 23, 2019

I think the last major change to the Gecko implementation was https://bugzilla.mozilla.org/show_bug.cgi?id=987007 (April 2014). The spec was written in #45 (March 2015).

@bzbarsky
Copy link
Collaborator

Ah, so the key question is how named creators (aka setters nowadays) should work. I think in Gecko if you do the thing on an [OverrideBuiltins] object (e.g. DOMStringMap) the behavior would be "foo", not "undefined"...

@Ms2ger
Copy link
Member Author

Ms2ger commented Jan 30, 2019

Hmm, indeed. The test below doesn't call proto.[[Set]], and just calls the setter. Should I just add a check for [OverrideBuiltins]?

<!DOCTYPE html>
<script>
var ds = document.documentElement.dataset;
var a = new Proxy(Object.getPrototypeOf(ds), {
  set(...args) { w(args); return false; },
});
Object.setPrototypeOf(ds, a);
ds.x = "foo";
w(ds.x)
</script>

It also turns out WebKit isn't following the spec for the non-[OverrideBuiltins] case; it was just messing with my __proto__ set. Updated the result in #630 (comment).

@Ms2ger
Copy link
Member Author

Ms2ger commented Jan 30, 2019

I confused myself again; it looks like WebKit is following the spec in all cases I tested. Results:

Test Spec Gecko Chrome WebKit
no [OB], O = Receiver named setter proto.[[Set]]() proto.[[Set]]() named setter
no [OB], OReceiver proto.[[Set]]() proto.[[Set]]() proto.[[Set]]() proto.[[Set]]()
[OB], O = Receiver named setter named setter named setter named setter
[OB], OReceiver proto.[[Set]]() named setter proto.[[Set]]() proto.[[Set]]()
Tests

no [OB], O = Receiver

<!DOCTYPE html>
<script>
sessionStorage.clear()
var a = new Proxy(Object.getPrototypeOf(sessionStorage), {
  set(...args) { w(args); return false; },
});
Object.setPrototypeOf(sessionStorage, a);
sessionStorage.x = "foo";
w(sessionStorage.x)
</script>
C
log: object "[object Storage],x,foo,[object Storage]" (4 props: 0="[object Storage]", 1="x", 2="foo", 3="[object Storage]")
log: undefined
G
log: object "[object StoragePrototype],x,foo,[object Storage]" (4 props: 0="[object StoragePrototype]", 1="x", 2="foo", 3="[object Storage]")
log: undefined
W
log: foo

no [OB], OReceiver

<!DOCTYPE html>
<script>
sessionStorage.clear()
var a = new Proxy(Object.getPrototypeOf(sessionStorage), {
  set(...args) { w(args); return false; },
});
Object.setPrototypeOf(sessionStorage, a);
Object.create(sessionStorage).x = "foo";
w(sessionStorage.x)
</script>
C
log: object "[object Storage],x,foo,[object Storage]" (4 props: 0="[object Storage]", 1="x", 2="foo", 3="[object Storage]")
log: undefined

G
log: object "[object StoragePrototype],x,foo,[object Object]" (4 props: 0="[object StoragePrototype]", 1="x", 2="foo", 3="[object Object]")
log: undefined

W
log: object "[object Object],x,foo,[object Object]" (4 props: 0="[object Object]", 1="x", 2="foo", 3="[object Object]")
log: undefined

[OB], O = Receiver

<!DOCTYPE html>
<script>
var ds = document.documentElement.dataset;
var a = new Proxy(Object.getPrototypeOf(ds), {
  set(...args) { w(args); return false; },
});
Object.setPrototypeOf(ds, a);
ds.x = "foo";
w(ds.x)
</script>
C
log: foo

G
log: foo

W
log: foo

[OB], OReceiver

<!DOCTYPE html>
<script>
var ds = document.documentElement.dataset;
var a = new Proxy(Object.getPrototypeOf(ds), {
  set(...args) { w(args); return false; },
});
Object.setPrototypeOf(ds, a);
Object.create(ds).x = "foo";
w(ds.x)
</script>
C
log: object "[object DOMStringMap],x,foo,[object DOMStringMap]" (4 props: 0="[object DOMStringMap]", 1="x", 2="foo", 3="[object DOMStringMap]")
log: undefined

G
log: foo

W
log: object "[object DOMStringMapPrototype],x,foo,[object Object]" (4 props: 0="[object DOMStringMapPrototype]", 1="x", 2="foo", 3="[object Object]")
log: undefined

Ms2ger added a commit that referenced this issue Apr 15, 2019
… interfaces

This is the case where it makes most sense, since the prototype chain can't
shadow any properties.

This change does not affect normal usage; it is only detectable by messing with
the prototype of the object.

The majority of implementations (Gecko and Chrome) already follow the proposed
change; WebKit follows the existing spec.

Fixes #630.
Ms2ger added a commit that referenced this issue Apr 16, 2019
… interfaces

This is the case where it makes most sense, since the prototype chain can't
shadow any properties.

This change does not affect normal usage; it is only detectable by messing with
the prototype of the object.

The majority of implementations (Gecko and Chrome) already follow the proposed
change; WebKit follows the existing spec.

Fixes #630.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants