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

Typed array instance used as prototype incorrect perform [[Get]]/[[Set]] #1541

Closed
allenwb opened this issue May 15, 2019 · 15 comments · Fixed by #1556
Closed

Typed array instance used as prototype incorrect perform [[Get]]/[[Set]] #1541

allenwb opened this issue May 15, 2019 · 15 comments · Fixed by #1556

Comments

@allenwb
Copy link
Member

allenwb commented May 15, 2019

Using a Typed Array (or most any exotic object) as as a prototype is weird. But their specification
of [[Get]] and [[Set]] does not correctly deal with the case where the Rceiver argument is not the same object as the exotic array. In this case, a [[Put]] can actually mutate the prototype type object. Consider:

let o = new Int8Array(1)
Object.setPrototypeOf(Array.prototype, o);
const a = [];  //a inherits indirectly from o
a[0] = 4;  //according to current spec. this will set o[0] to 4.

The problem is that 9.4.5.5 does not check if O is different from Receiver and calls IntegerIndexElementSet to mutate O rather than Receiver (which would also be wrong, but for other reasons.

The fix is to change step 2 to:

if SameValue(O,Reciever) and Type(P) is String, then

The same change should also be made to 9.4.5.4

@ljharb
Copy link
Member

ljharb commented May 15, 2019

With this code:

let o = new Int8Array(1)
Object.setPrototypeOf(Array.prototype, o);
const a = [];  //a inherits indirectly from o
a[0] = 4;  //according to current spec. this will set o[0] to 4.
print(a[0], o[0]);

I get these results from eshost file.js:

#### Chakra
4 0

#### JavaScriptCore
4 0

#### SpiderMonkey
4 0

#### V8
4 0

#### V8 --harmony
4 0

This suggests that engines are already doing the correct thing, so it's just a bug in the spec.

@anba
Copy link
Contributor

anba commented Jul 19, 2019

That means reverting #347, so we'd again get what was originally specified in ES6.

@ljharb
Copy link
Member

ljharb commented Jul 19, 2019

Does that imply that engines implemented it after #347 landed but with the prior behavior?

@anba
Copy link
Contributor

anba commented Jul 19, 2019

I guess SpiderMonkey follows what the current spec (including #347) has for 9.4.5.4 [[Get]], but SpiderMonkey's implementation for 9.4.5.5 [[Set]] doesn't seem to comply to the current spec.

LookupOwnPropertyInline returns done = true when a TypedArray object is encountered, so we always stop in NativeGetPropertyInline even for out-of-bounds accesses. But for [[Set]], an out-of-bounds access results in adding a new property in NativeSetProperty and also in SetExistingProperty for in-bounds, but wrong receiver.

So basically the primary reason for any differences in SpiderMonkey's implementation, is that we inspect the [[ArrayLength]] internal slot of the TypedArrray object earlier than specified in the spec.

@ljharb
Copy link
Member

ljharb commented Jul 19, 2019

OK, so I guess the real questions here imo are in this order:

  1. what behavior do we want?
  2. are engines willing to change to the desired behavior?
  3. if so, yay, let's merge that! if not, what's the most desirable behavior that engines are willing to implement, and how does that differ from current reality?

@evilpie
Copy link
Contributor

evilpie commented Jul 20, 2019

I think I tried to implement the spec behavior for [[Set]] and it's not web compatible: https://bugzilla.mozilla.org/show_bug.cgi?id=1502889

@ljharb
Copy link
Member

ljharb commented Jul 20, 2019

aha, and that points to #1339.

Looks like we may need to revert all or part of #347?

@anba
Copy link
Contributor

anba commented Jul 22, 2019

I've totally forgotten about the web-compatibility issue encountered in Firefox.

Looks like we may need to revert all or part of #347?

I guess the [[Get]] part is still okay, but [[Set]] will need to be changed.

And then we need to decide if consistency between [[Get]] and [[Set]] (*) or between [[Get]] and [[HasProperty]] (**) is more important, because we can't get both.

(*) That means consistent receiver checks.
(**) Per the example in #347

@verwaest
Copy link

verwaest commented Sep 3, 2019

What about just adding a branch for into 2 (if SameValue(O,Receiver)) so we don't fall to 3, but still don't set the value onto the prototype?

@evilpie
Copy link
Contributor

evilpie commented Feb 25, 2021

@rkirsling this might be relevant to you, considering that you worked on #2164.

@shvaikalesh
Copy link
Member

shvaikalesh commented Feb 25, 2021

@evilpie I've updated #1556 to align the spec with V8, fixing the web-compatibility issue that is still present. I will update the summary and submit thorough test262 coverage within few days.

@rkirsling would you please consider presenting it at the next TC39 meeting?

@syg
Copy link
Contributor

syg commented Feb 25, 2021

I got consensus for this back in Dec 2019, but embarrassingly forgot to ever write up a normative PR for it. Is this the same issue or a different one?

@evilpie
Copy link
Contributor

evilpie commented Feb 25, 2021

I got consensus for this back in Dec 2019, but embarrassingly forgot to ever write up a normative PR for it. Is this the same issue or a different one?

Oh nice! I am pretty sure the [[Set]] issue is the same. You are even showing the Vector example that I found.

@akamsmrhill

This comment has been minimized.

@shvaikalesh
Copy link
Member

@syg It is definitely the same issue, yet with a bit different solution for [[Set]]. Slide 10 claims:

Inside IntegerIndexed when the receiver is not the holder inside TypedArrays’ [[Set]], do an OrdinarySet.

Taking into account Slide 12, it translates to normative change like:

          1. Assert: IsPropertyKey(_P_) is *true*.
-         1. If Type(_P_) is String, then
+         1. If SameValue(_O_, _Receiver_) is *true* and Type(_P_) is String, then
            1. Let _numericIndex_ be ! CanonicalNumericIndexString(_P_).
            1. If _numericIndex_ is not *undefined*, then
              1. Perform ? IntegerIndexedElementSet(_O_, _numericIndex_, _V_).
              1. Return *true*.
          1. Return ? OrdinarySet(_O_, _P_, _V_, _Receiver_).

Which would break consistency with TypedArray's [[HasProperty]] and [[Get]] methods regarding invalid indices:

Object.defineProperty(Int8Array.prototype, -1, {
  get() { throw new Error("-1 getter should be unreachable!"); },
  set(-v) { throw new Error("-1 setter should be unreachable!"); },
});

const tA = new Int8Array(10);
-1 in tA; // => false
tA[-1]; // => undefined
tA[-1] = 1; // => throws Error

#1556 aligns [[Set]] with other methods regarding canonical numeric indices that are invalid, and is what V8 currently do.

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

Successfully merging a pull request may close this issue.

8 participants