Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2431223005:
Implement DefineOwnProperty for TypedArrays (Closed)
|
Created:
4 years, 2 months ago by Henrique Ferreiro Modified:
4 years ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionImplement DefineOwnProperty for TypedArrays
TypedArrays need specific checks before calling OrdinaryDefineOwnProperty.
BUG=v8:5328
Committed: https://crrev.com/bc1a3820c27ea869fcc7033ce4f72d9f2105992d
Cr-Commit-Position: refs/heads/master@{#41333}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Implement DefineOwnProperty for TypedArrays #
Total comments: 17
Patch Set 3 : address comments and throw when needed #
Total comments: 1
Patch Set 4 : set correct descriptor #
Total comments: 8
Patch Set 5 : Run git cl format #Patch Set 6 : Fix tests and a type conversion mistake #
Total comments: 8
Patch Set 7 : Refactor CanonicalNumericIndexString and fix tests #
Total comments: 6
Patch Set 8 : Correct use of assertThrows #
Total comments: 4
Patch Set 9 : Use function arrow for assertThrows #
Total comments: 5
Patch Set 10 : fix punctuation, variable name and formatting #Patch Set 11 : Fix GCMole warning #
Total comments: 1
Patch Set 12 : move assignment out of comparison #Patch Set 13 : formatting fixes #
Total comments: 3
Messages
Total messages: 74 (20 generated)
Description was changed from ========== Implement DefineOwnProperty for TypedArrays TypedArrays need specific checks before calling OrdinaryDefineOwnProperty. BUG=v8:5328 ========== to ========== Implement DefineOwnProperty for TypedArrays TypedArrays need specific checks before calling OrdinaryDefineOwnProperty. BUG=v8:5328 ==========
henrique.ferreiro@gmail.com changed reviewers: + caitp@igalia.com
https://codereview.chromium.org/2431223005/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/1/src/objects.cc#newcode17445 src/objects.cc:17445: if (std::floor(std::abs(d)) != std::abs(d)) return false; Why the std::floor() and std::abs() and stuff? I think we can get away with something like this: ``` if (!number->IsNumber()) return false; double index = number->Value(); return std::isfinite(index) && FastD2I(index) == index; ``` And, you could also get the `-0` and negative number checks for free by adding a `&& !std::signbit(index)` to that line at the end. https://codereview.chromium.org/2431223005/diff/1/src/objects.cc#newcode17454 src/objects.cc:17454: if (!Object::ToString(isolate, index).ToHandleChecked()->SameValue(*s)) I would leave a comment saying that we need this to avoid treating strings like "2E1" and "20" as the same key, so that it doesn't look like we're just implementing the spec verbatim "just because". I wonder if there's a faster way to implement this, though. If the string contains an non-digit characters, ToString() should always return a different value AFAIK (except in the case of NaN, which would return false anyways?) I'm not sure of all of the situations we'd have to account for, so I'm fine with leaving it as is (with the comment). https://codereview.chromium.org/2431223005/diff/1/src/objects.cc#newcode17509 src/objects.cc:17509: } It looks like we're missing the Integer IntegerIndexedElementSet case. Some of the validation steps in that algorithm can be skipped here because they're already checked above (eg index < length, index != -0, IsInteger(index), etc). Do you need help with this part? https://codereview.chromium.org/2431223005/diff/1/src/objects.cc#newcode17513 src/objects.cc:17513: return OrdinaryDefineOwnProperty(isolate, o, key, desc, should_throw); The ToPropertyKey implementation in v8 can return Smi numbers, but per spec those should be converted to strings. You should add a path which handles the Smi case (which you only need to validate by testing if it's negative or within the range of the typed array).
I think I got all comments sorted through. https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17503: if (!desc->has_writable()) desc->set_writable(true); I got the default values from GetOwnProperty, is this ok? is there a function to get these instead of having them hardcoded? https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17520: // TODO(jkummerow): Setting an indexed accessor on a typed array should I think this comments may be obsolete, the check above returns false instead of throwing
https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17454: return (n->IsSmi() || (std::isfinite(d) && FastD2I(d) == d)) && There are situations where this doesn't work. FastD2I(2147483647.0 + 1.0) != 2147483648, but this is a valid positive integer which fits in the 32bit range. Maybe a simpler thing to do is this: ``` // Sign check, etc double fp_index = nueric_index->Number(); if (!std::isfinite(fp_index) || std::signbit(fp_index)) return Just(false); // Convert to uint32, return false if not an integer uint32_t index; if (!numeric_index->ToUint32(&index)) return Just(false); ``` I _think_ that does everything that's needed for this. (We can't really allocate typed arrays with lengths like this in Chrome yet, so you could probably mark this as a FIXME to be dealt with later) https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17477: if (numeric_index->IsSmi() || !numeric_index->IsUndefined(isolate)) { You don't need the `numeric_index->IsSmi()` condition here, the `!numeric_index->IsUndefined(isolate)` branch covers that https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17503: if (!desc->has_writable()) desc->set_writable(true); On 2016/10/24 11:56:11, hferreiro wrote: > I got the default values from GetOwnProperty, is this ok? is there a function to > get these instead of having them hardcoded? You don't need to update the descriptors, the stuff above checks that the descriptor is compatible, so leaving it alone is fine https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17520: // TODO(jkummerow): Setting an indexed accessor on a typed array should On 2016/10/24 11:56:11, hferreiro wrote: > I think this comments may be obsolete, the check above returns false instead of > throwing Notice that this function is passed the `should_throw` flag. If that flag == THROW_ON_ERROR, throw an exception and return `Nothing<bool>()`; The caller won't throw for you (at least, Object.defineProperty() won't). This gives you the freedom to provide a more detailed error message, which is kind of nice for users (but it's fine to just throw the same thing thrown elsewhere, too).
https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17454: return (n->IsSmi() || (std::isfinite(d) && FastD2I(d) == d)) && On 2016/10/24 13:44:01, caitp wrote: > There are situations where this doesn't work. > > FastD2I(2147483647.0 + 1.0) != 2147483648, but this is a valid positive integer > which fits in the 32bit range. > > Maybe a simpler thing to do is this: > > ``` > // Sign check, etc > double fp_index = nueric_index->Number(); > if (!std::isfinite(fp_index) || std::signbit(fp_index)) return Just(false); > > // Convert to uint32, return false if not an integer > uint32_t index; > if (!numeric_index->ToUint32(&index)) return Just(false); > ``` > > I _think_ that does everything that's needed for this. > > (We can't really allocate typed arrays with lengths like this in Chrome yet, so > you could probably mark this as a FIXME to be dealt with later) I've just found IsUint32Double(double), which seems equivalent to IsPositiveInteger for our case, right? https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17477: if (numeric_index->IsSmi() || !numeric_index->IsUndefined(isolate)) { On 2016/10/24 13:44:02, caitp wrote: > You don't need the `numeric_index->IsSmi()` condition here, the > `!numeric_index->IsUndefined(isolate)` branch covers that Acknowledged. https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17503: if (!desc->has_writable()) desc->set_writable(true); On 2016/10/24 13:44:02, caitp wrote: > On 2016/10/24 11:56:11, hferreiro wrote: > > I got the default values from GetOwnProperty, is this ok? is there a function > to > > get these instead of having them hardcoded? > > You don't need to update the descriptors, the stuff above checks that the > descriptor is compatible, so leaving it alone is fine Sure, I got confused because many tests where failing without that. I am updating them to pass the correct descriptor values. https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17520: // TODO(jkummerow): Setting an indexed accessor on a typed array should On 2016/10/24 13:44:01, caitp wrote: > On 2016/10/24 11:56:11, hferreiro wrote: > > I think this comments may be obsolete, the check above returns false instead > of > > throwing > > Notice that this function is passed the `should_throw` flag. If that flag == > THROW_ON_ERROR, throw an exception and return `Nothing<bool>()`; The caller > won't throw for you (at least, Object.defineProperty() won't). > > This gives you the freedom to provide a more detailed error message, which is > kind of nice for users (but it's fine to just throw the same thing thrown > elsewhere, too). I meant that setting an indexed accessor shouldn't throw, in general. Though I missed that I should check the 'should_throw' flag.
https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17454: return (n->IsSmi() || (std::isfinite(d) && FastD2I(d) == d)) && On 2016/10/25 12:02:20, hferreiro wrote: > On 2016/10/24 13:44:01, caitp wrote: > > There are situations where this doesn't work. > > > > FastD2I(tel:2147483647.0 + 1.0) != tel:2147483648, but this is a valid positive > integer > > which fits in the 32bit range. > > > > Maybe a simpler thing to do is this: > > > > ``` > > // Sign check, etc > > double fp_index = nueric_index->Number(); > > if (!std::isfinite(fp_index) || std::signbit(fp_index)) return Just(false); > > > > // Convert to uint32, return false if not an integer > > uint32_t index; > > if (!numeric_index->ToUint32(&index)) return Just(false); > > ``` > > > > I _think_ that does everything that's needed for this. > > > > (We can't really allocate typed arrays with lengths like this in Chrome yet, > so > > you could probably mark this as a FIXME to be dealt with later) > > I've just found IsUint32Double(double), which seems equivalent to > IsPositiveInteger for our case, right? Sure https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17503: if (!desc->has_writable()) desc->set_writable(true); On 2016/10/25 12:02:20, hferreiro wrote: > On 2016/10/24 13:44:02, caitp wrote: > > On 2016/10/24 11:56:11, hferreiro wrote: > > > I got the default values from GetOwnProperty, is this ok? is there a > function > > to > > > get these instead of having them hardcoded? > > > > You don't need to update the descriptors, the stuff above checks that the > > descriptor is compatible, so leaving it alone is fine > > Sure, I got confused because many tests where failing without that. I am > updating them to pass the correct descriptor values. You can use PropertyDescriptor::CompletePropertyDescriptor() instead, to avoid hard coding anything here https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17520: // TODO(jkummerow): Setting an indexed accessor on a typed array should On 2016/10/25 12:02:20, hferreiro wrote: > On 2016/10/24 13:44:01, caitp wrote: > > On 2016/10/24 11:56:11, hferreiro wrote: > > > I think this comments may be obsolete, the check above returns false instead > > of > > > throwing > > > > Notice that this function is passed the `should_throw` flag. If that flag == > > THROW_ON_ERROR, throw an exception and return `Nothing<bool>()`; The caller > > won't throw for you (at least, Object.defineProperty() won't). > > > > This gives you the freedom to provide a more detailed error message, which is > > kind of nice for users (but it's fine to just throw the same thing thrown > > elsewhere, too). > > I meant that setting an indexed accessor shouldn't throw, in general. Though I > missed that I should check the 'should_throw' Yes it should throw, particularly if called via DefineOwnPropertyOrThrow() in the spec, which is why you have the should_throw flag here
https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17503: if (!desc->has_writable()) desc->set_writable(true); On 2016/10/25 12:53:38, caitp wrote: > On 2016/10/25 12:02:20, hferreiro wrote: > > On 2016/10/24 13:44:02, caitp wrote: > > > On 2016/10/24 11:56:11, hferreiro wrote: > > > > I got the default values from GetOwnProperty, is this ok? is there a > > function > > > to > > > > get these instead of having them hardcoded? > > > > > > You don't need to update the descriptors, the stuff above checks that the > > > descriptor is compatible, so leaving it alone is fine > > > > Sure, I got confused because many tests where failing without that. I am > > updating them to pass the correct descriptor values. > > You can use PropertyDescriptor::CompletePropertyDescriptor() instead, to avoid > hard coding anything here Good to know, anyway, the problem is with the tests in test262 that provide the wrong descriptor https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17520: // TODO(jkummerow): Setting an indexed accessor on a typed array should On 2016/10/25 12:53:38, caitp wrote: > On 2016/10/25 12:02:20, hferreiro wrote: > > On 2016/10/24 13:44:01, caitp wrote: > > > On 2016/10/24 11:56:11, hferreiro wrote: > > > > I think this comments may be obsolete, the check above returns false > instead > > > of > > > > throwing > > > > > > Notice that this function is passed the `should_throw` flag. If that flag == > > > THROW_ON_ERROR, throw an exception and return `Nothing<bool>()`; The caller > > > won't throw for you (at least, Object.defineProperty() won't). > > > > > > This gives you the freedom to provide a more detailed error message, which > is > > > kind of nice for users (but it's fine to just throw the same thing thrown > > > elsewhere, too). > > > > I meant that setting an indexed accessor shouldn't throw, in general. Though I > > missed that I should check the 'should_throw' > > Yes it should throw, particularly if called via DefineOwnPropertyOrThrow() in > the spec, which is why you have the should_throw flag here Acknowledged.
https://codereview.chromium.org/2431223005/diff/40001/src/messages.h File src/messages.h (left): https://codereview.chromium.org/2431223005/diff/40001/src/messages.h#oldcode507 src/messages.h:507: T(InvalidTypedArrayOffset, "Start offset is too large:") \ it isn't used anywhere
https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17503: if (!desc->has_writable()) desc->set_writable(true); On 2016/10/25 12:53:38, caitp wrote: > On 2016/10/25 12:02:20, hferreiro wrote: > > On 2016/10/24 13:44:02, caitp wrote: > > > On 2016/10/24 11:56:11, hferreiro wrote: > > > > I got the default values from GetOwnProperty, is this ok? is there a > > function > > > to > > > > get these instead of having them hardcoded? > > > > > > You don't need to update the descriptors, the stuff above checks that the > > > descriptor is compatible, so leaving it alone is fine > > > > Sure, I got confused because many tests where failing without that. I am > > updating them to pass the correct descriptor values. > > You can use PropertyDescriptor::CompletePropertyDescriptor() instead, to avoid > hard coding anything here CompletePropertyDescriptor() returns false for all the data descriptor fields. I need writable and enumerable to be true. Should I add PropertyDescriptor::CompleteIntegerIndexedPropertyDescriptor()?
https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... src/objects.cc:17503: if (!desc->has_writable()) desc->set_writable(true); On 2016/11/08 23:29:00, hferreiro wrote: > On 2016/10/25 12:53:38, caitp wrote: > > On 2016/10/25 12:02:20, hferreiro wrote: > > > On 2016/10/24 13:44:02, caitp wrote: > > > > On 2016/10/24 11:56:11, hferreiro wrote: > > > > > I got the default values from GetOwnProperty, is this ok? is there a > > > function > > > > to > > > > > get these instead of having them hardcoded? > > > > > > > > You don't need to update the descriptors, the stuff above checks that the > > > > descriptor is compatible, so leaving it alone is fine > > > > > > Sure, I got confused because many tests where failing without that. I am > > > updating them to pass the correct descriptor values. > > > > You can use PropertyDescriptor::CompletePropertyDescriptor() instead, to avoid > > hard coding anything here > > CompletePropertyDescriptor() returns false for all the data descriptor fields. I > need writable and enumerable to be true. Should I add > PropertyDescriptor::CompleteIntegerIndexedPropertyDescriptor()? ah, good point. I would just leave it as-is then
On 2016/11/08 at 23:55:33, caitp wrote: > https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc > File src/objects.cc (right): > > https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... > src/objects.cc:17503: if (!desc->has_writable()) desc->set_writable(true); > On 2016/11/08 23:29:00, hferreiro wrote: > > On 2016/10/25 12:53:38, caitp wrote: > > > On 2016/10/25 12:02:20, hferreiro wrote: > > > > On 2016/10/24 13:44:02, caitp wrote: > > > > > On 2016/10/24 11:56:11, hferreiro wrote: > > > > > > I got the default values from GetOwnProperty, is this ok? is there a > > > > function > > > > > to > > > > > > get these instead of having them hardcoded? > > > > > > > > > > You don't need to update the descriptors, the stuff above checks that the > > > > > descriptor is compatible, so leaving it alone is fine > > > > > > > > Sure, I got confused because many tests where failing without that. I am > > > > updating them to pass the correct descriptor values. > > > > > > You can use PropertyDescriptor::CompletePropertyDescriptor() instead, to avoid > > > hard coding anything here > > > > CompletePropertyDescriptor() returns false for all the data descriptor fields. I > > need writable and enumerable to be true. Should I add > > PropertyDescriptor::CompleteIntegerIndexedPropertyDescriptor()? > > ah, good point. I would just leave it as-is then As it currently stands, passing a descriptor without { configurable: false } fails with 'cannot redefine property'.
caitp@igalia.com changed reviewers: + cbruni@chromium.org, littledan@chromium.org
On 2016/11/09 00:06:30, hferreiro wrote: > On 2016/11/08 at 23:55:33, caitp wrote: > > https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc > > File src/objects.cc (right): > > > > > https://codereview.chromium.org/2431223005/diff/20001/src/objects.cc#newcode1... > > src/objects.cc:17503: if (!desc->has_writable()) desc->set_writable(true); > > On 2016/11/08 23:29:00, hferreiro wrote: > > > On 2016/10/25 12:53:38, caitp wrote: > > > > On 2016/10/25 12:02:20, hferreiro wrote: > > > > > On 2016/10/24 13:44:02, caitp wrote: > > > > > > On 2016/10/24 11:56:11, hferreiro wrote: > > > > > > > I got the default values from GetOwnProperty, is this ok? is there a > > > > > function > > > > > > to > > > > > > > get these instead of having them hardcoded? > > > > > > > > > > > > You don't need to update the descriptors, the stuff above checks that > the > > > > > > descriptor is compatible, so leaving it alone is fine > > > > > > > > > > Sure, I got confused because many tests where failing without that. I am > > > > > updating them to pass the correct descriptor values. > > > > > > > > You can use PropertyDescriptor::CompletePropertyDescriptor() instead, to > avoid > > > > hard coding anything here > > > > > > CompletePropertyDescriptor() returns false for all the data descriptor > fields. I > > > need writable and enumerable to be true. Should I add > > > PropertyDescriptor::CompleteIntegerIndexedPropertyDescriptor()? > > > > ah, good point. I would just leave it as-is then > > As it currently stands, passing a descriptor without { configurable: false } > fails with 'cannot redefine property'. I may not have been clear, I just meant leave your descriptor settup code the way it is, not "leave the descriptor flags alone". /CC some other folks to take a look
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28529)
Looks like a clean, correct implementation to me. A couple small things: - You need to update test expectations, both in test262 and mjsunit tests, as you can see from the try job results that I triggered. - Since this takes us from not-throwing to throwing in a particular case, it would be nice to at least add a UseCounter which tells us how often we hit this case, so we know whether we're breaking the web (as has happened in the past when implementing the spec's semantics). Here, I'd be really surprised if it's common, but it would be good to have this data so we can back off if it's harmful. If it looked like a greater risk, I'd say first ship the counter, and then ship the change later. If the counter seems prohibitively difficult to implement, it's not strictly necessary, though. https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... src/objects.cc:17451: // ES6 9.4.5.3 Nit: New references to the spec should be identified by the id, e.g. here ES#sec-integer-indexed-exotic-objects-defineownproperty-p-desc so that it's stable across versions.
https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... src/objects.cc:17440: static Handle<Object> CanonicalNumericIndexString(Isolate* isolate, nit: use an anonymous namespace for this helper You probably want to convert this into something similar to PropertyKeyToArrayIndex, directly returning a converted value and a boolean for success. This would avoid accessing undefined and the IsUndefined check further down. Looking at the spec, I see that all the users of CanonicalNumericIndexString do an integer conversion later-on, so you could directly fold this into here. And return false if the input cannot be converted to a valid index. https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... src/objects.cc:17447: return isolate->factory()->undefined_value(); nit: always use braces for if-bodies on separate lines. https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... src/objects.cc:17476: if (!IsUint32Double(index)) You may want to shuffle around the steps a bit here to make the code a bit more streamlined (see comment above for CanonicalNumericIndexString). https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... src/objects.cc:17480: int length = o->length()->Number(); I'd feel safer by using a uint32_t here. https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... src/objects.cc:17484: NewTypeError(MessageTemplate::kInvalidTypedArrayIndex)); nit: missing braces https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... src/objects.cc:17488: NewTypeError(MessageTemplate::kRedefineDisallowed, key)); nit: missing braces https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... src/objects.cc:17499: NewTypeError(MessageTemplate::kRedefineDisallowed, key)); nit: missing braces
Fixed tests and a type conversion mistake that was hidden by not updating test262 expectations. I haven't yet looked at using a counter. https://codereview.chromium.org/2431223005/diff/100001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/100001/src/objects.cc#newcode... src/objects.cc:17472: double index_dbl = numeric_index->Number(); This was uncovered when fixing test262 expectations. I used DoubleToUint32 instead of FastD2UI because it looks faster in the case the two values match (two static type conversions). https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:32: var o = new Int32Array(1); I assumed that we are checking particularly for accessors, not an out-of-bounds index
https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:32: var o = new Int32Array(1); On 2016/11/15 13:07:55, hferreiro wrote: > I assumed that we are checking particularly for accessors, not an out-of-bounds > index Why not test both? https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:34: assertEquals(undefined, Object.getOwnPropertyDescriptor(o, "0")); isn't this assertion going to fail with this change?
https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:32: var o = new Int32Array(1); On 2016/11/15 at 15:10:57, caitp wrote: > On 2016/11/15 13:07:55, hferreiro wrote: > > I assumed that we are checking particularly for accessors, not an out-of-bounds > > index > > Why not test both? Well, this test is called element-accessor.js, there are already tests for that in mjsunit/es6/indexed-integer-exotics.js. https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:34: assertEquals(undefined, Object.getOwnPropertyDescriptor(o, "0")); On 2016/11/15 at 15:10:57, caitp wrote: > isn't this assertion going to fail with this change? It returns 'undefined', as it used to do. I don't understand why that would change.
On 2016/11/15 at 11:28:16, littledan wrote: > - Since this takes us from not-throwing to throwing in a particular case, it would be nice to at least add a UseCounter which tells us how often we hit this case, so we know whether we're breaking the web (as has happened in the past when implementing the spec's semantics). Here, I'd be really surprised if it's common, but it would be good to have this data so we can back off if it's harmful. If it looked like a greater risk, I'd say first ship the counter, and then ship the change later. If the counter seems prohibitively difficult to implement, it's not strictly necessary, though. As far as I know, the previous version would always throw. What particular case are you talking about?
https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:34: assertEquals(undefined, Object.getOwnPropertyDescriptor(o, "0")); On 2016/11/15 22:03:13, hferreiro wrote: > On 2016/11/15 at 15:10:57, caitp wrote: > > isn't this assertion going to fail with this change? > > It returns 'undefined', as it used to do. I don't understand why that would > change. It shouldn't return undefined, because it _should_ be `{ value: 0, writable: true, enumerable: true, configurable: false }`, based on the logic in https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-getownprop...
https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:34: assertEquals(undefined, Object.getOwnPropertyDescriptor(o, "0")); On 2016/11/15 22:13:50, caitp wrote: > On 2016/11/15 22:03:13, hferreiro wrote: > > On 2016/11/15 at 15:10:57, caitp wrote: > > > isn't this assertion going to fail with this change? > > > > It returns 'undefined', as it used to do. I don't understand why that would > > change. > > It shouldn't return undefined, because it _should_ be `{ value: 0, writable: > true, enumerable: true, configurable: false }`, based on the logic in > https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-getownprop... in detail, the typed array is created via https://tc39.github.io/ecma262/#sec-typedarray-length, so it has a length of 1, so "0"'s canonical numeric index is less than length, so it returns this predefined descriptor. If it's returning undefined, then something is wrong in this code
On 2016/11/15 at 22:17:01, caitp wrote: > https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... > File test/mjsunit/element-accessor.js (right): > > https://codereview.chromium.org/2431223005/diff/100001/test/mjsunit/element-a... > test/mjsunit/element-accessor.js:34: assertEquals(undefined, Object.getOwnPropertyDescriptor(o, "0")); > On 2016/11/15 22:13:50, caitp wrote: > > On 2016/11/15 22:03:13, hferreiro wrote: > > > On 2016/11/15 at 15:10:57, caitp wrote: > > > > isn't this assertion going to fail with this change? > > > > > > It returns 'undefined', as it used to do. I don't understand why that would > > > change. > > > > It shouldn't return undefined, because it _should_ be `{ value: 0, writable: > > true, enumerable: true, configurable: false }`, based on the logic in > > https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-getownprop... > > in detail, the typed array is created via https://tc39.github.io/ecma262/#sec-typedarray-length, so it has a length of 1, so "0"'s canonical numeric index is less than length, so it returns this predefined descriptor. > > If it's returning undefined, then something is wrong in this code Yes, sorry, somehow I missed that, I'll update the test.
On 2016/11/15 at 13:07:23, cbruni wrote: > https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc > File src/objects.cc (right): > > https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... > src/objects.cc:17440: static Handle<Object> CanonicalNumericIndexString(Isolate* isolate, > nit: use an anonymous namespace for this helper > > You probably want to convert this into something similar to PropertyKeyToArrayIndex, directly returning a converted value and a boolean for success. > This would avoid accessing undefined and the IsUndefined check further down. > > Looking at the spec, I see that all the users of CanonicalNumericIndexString do an integer conversion later-on, so you could directly fold this into here. And return false if the input cannot be converted to a valid index. > > https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... > src/objects.cc:17476: if (!IsUint32Double(index)) > You may want to shuffle around the steps a bit here to make the code a bit more streamlined (see comment above for CanonicalNumericIndexString). The problem is that there is a distinction between a non-numeric string and a floating point value. In the first case, we fall back on OrdinaryDefineOwnProperty checking for undefined. In the latter, an error is thrown. I can return the numeric value in the function though, and avoid the undefined check.
On 2016/11/16 at 10:13:20, henrique.ferreiro wrote: > On 2016/11/15 at 13:07:23, cbruni wrote: > > https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc > > File src/objects.cc (right): > > > > https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... > > src/objects.cc:17440: static Handle<Object> CanonicalNumericIndexString(Isolate* isolate, > > nit: use an anonymous namespace for this helper > > > > You probably want to convert this into something similar to PropertyKeyToArrayIndex, directly returning a converted value and a boolean for success. > > This would avoid accessing undefined and the IsUndefined check further down. > > > > Looking at the spec, I see that all the users of CanonicalNumericIndexString do an integer conversion later-on, so you could directly fold this into here. And return false if the input cannot be converted to a valid index. > > > > https://codereview.chromium.org/2431223005/diff/60001/src/objects.cc#newcode1... > > src/objects.cc:17476: if (!IsUint32Double(index)) > > You may want to shuffle around the steps a bit here to make the code a bit more streamlined (see comment above for CanonicalNumericIndexString). > > The problem is that there is a distinction between a non-numeric string and a floating point value. In the first case, we fall back on OrdinaryDefineOwnProperty checking for undefined. In the latter, an error is thrown. I can return the numeric value in the function though, and avoid the undefined check. Right, that's probably good enough.
https://codereview.chromium.org/2431223005/diff/120001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/120001/src/objects.cc#newcode... src/objects.cc:17484: if (double_index->IsMinusZero() || !double_index->ToUint32(&index)) { ToUint32 doesn't check for -0. This seems like a bug.
https://codereview.chromium.org/2431223005/diff/120001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/120001/src/objects.cc#newcode... src/objects.cc:17484: if (double_index->IsMinusZero() || !double_index->ToUint32(&index)) { On 2016/11/17 12:11:02, hferreiro wrote: > ToUint32 doesn't check for -0. This seems like a bug. Why would it? -0 can't be represented in uint32, and most uses of this function treat -0 and +0 the same
https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (left): https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:34: assertEquals(undefined, Object.getOwnPropertyDescriptor(o, "0")); this old test was wrong per spec, does it still pass? This line should have thrown instead of silently failing
https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (left): https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:34: assertEquals(undefined, Object.getOwnPropertyDescriptor(o, "0")); On 2016/11/17 at 15:24:42, caitp wrote: > this old test was wrong per spec, does it still pass? This line should have thrown instead of silently failing IntegerIndexedElementGet: Step 8. If index < 0 or index ≥ length, return undefined. GetOwnProperty: Step 3b ii. If value is undefined, return undefined
https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (left): https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:34: assertEquals(undefined, Object.getOwnPropertyDescriptor(o, "0")); On 2016/11/18 09:27:07, hferreiro wrote: > On 2016/11/17 at 15:24:42, caitp wrote: > > this old test was wrong per spec, does it still pass? This line should have > thrown instead of silently failing > > IntegerIndexedElementGet: Step 8. If index < 0 or index ≥ length, return > undefined. > GetOwnProperty: Step 3b ii. If value is undefined, return undefined No, Object.defineProperty should throw here, because the index is out of range
On 2016/11/18 at 14:47:51, caitp wrote: > https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... > File test/mjsunit/element-accessor.js (left): > > https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... > test/mjsunit/element-accessor.js:34: assertEquals(undefined, Object.getOwnPropertyDescriptor(o, "0")); > On 2016/11/18 09:27:07, hferreiro wrote: > > On 2016/11/17 at 15:24:42, caitp wrote: > > > this old test was wrong per spec, does it still pass? This line should have > > thrown instead of silently failing > > > > IntegerIndexedElementGet: Step 8. If index < 0 or index ≥ length, return > > undefined. > > GetOwnProperty: Step 3b ii. If value is undefined, return undefined > > No, Object.defineProperty should throw here, because the index is out of range Ah, yes. It failed silently.
https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:33: assertThrows('Object.defineProperty(o, "0", {get: function(){}})'); Nit: For new tests, pass two arguments: the code to run (as a function, rather than a string) and the class of the exception (here, TypeError).
On 2016/11/18 at 16:03:52, littledan wrote: > https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... > File test/mjsunit/element-accessor.js (right): > > https://codereview.chromium.org/2431223005/diff/120001/test/mjsunit/element-a... > test/mjsunit/element-accessor.js:33: assertThrows('Object.defineProperty(o, "0", {get: function(){}})'); > Nit: For new tests, pass two arguments: the code to run (as a function, rather than a string) and the class of the exception (here, TypeError). Fixed.
caitp@igalia.com changed reviewers: + jkummerow@chromium.org
lgtm /cc jkummerow about the fixed TODO https://codereview.chromium.org/2431223005/diff/140001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/140001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:35: }, TypeError); nit: you can use arrow function to make this more compact and readable, but its fine https://codereview.chromium.org/2431223005/diff/140001/test/test262/test262.s... File test/test262/test262.status (left): https://codereview.chromium.org/2431223005/diff/140001/test/test262/test262.s... test/test262/test262.status:446: 'built-ins/TypedArrays/internals/DefineOwnProperty/key-is-minus-zero': [FAIL], Are there any tests about the OOB accessor error that needs to be flipped?
https://codereview.chromium.org/2431223005/diff/140001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/140001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:35: }, TypeError); On 2016/11/21 at 22:02:36, caitp wrote: > nit: you can use arrow function to make this more compact and readable, but its fine Done. https://codereview.chromium.org/2431223005/diff/140001/test/test262/test262.s... File test/test262/test262.status (left): https://codereview.chromium.org/2431223005/diff/140001/test/test262/test262.s... test/test262/test262.status:446: 'built-ins/TypedArrays/internals/DefineOwnProperty/key-is-minus-zero': [FAIL], On 2016/11/21 at 22:02:36, caitp wrote: > Are there any tests about the OOB accessor error that needs to be flipped? key-is-greater-than-last-index was apparently working.
ACK. https://codereview.chromium.org/2431223005/diff/160001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/160001/src/objects.cc#newcode... src/objects.cc:6552: // DefineOwnPropertyIgnoreAttributes, can handle arguments (ES6 9.4.4.2) nit: Comments should have proper punctuation, in this case a trailing full stop. https://codereview.chromium.org/2431223005/diff/160001/src/objects.cc#newcode... src/objects.cc:17452: // Avoid treating strings like "2E1" and "20" as the same key nit: Comments should have proper punctuation, in this case a trailing full stop. https://codereview.chromium.org/2431223005/diff/160001/src/objects.cc#newcode... src/objects.cc:17473: // ToPropertyKey returns Smi's too ToPropertyKey isn't called here? Just drop this comment. https://codereview.chromium.org/2431223005/diff/160001/src/objects.cc#newcode... src/objects.cc:17482: // FIXME: the standard allows up to 2^53 elements nit: Comments should have proper punctuation, in this case a trailing full stop.
LGTM minus var name https://codereview.chromium.org/2431223005/diff/160001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/160001/src/objects.cc#newcode... src/objects.cc:17477: Handle<Object> double_index; You call this double_index but in CanonicalNumericIndexString you might assign a Smi to it, that's a bit confusing.
> https://codereview.chromium.org/2431223005/diff/160001/src/objects.cc#newcode... > src/objects.cc:17473: // ToPropertyKey returns Smi's too > ToPropertyKey isn't called here? Just drop this comment. This was to explain why we are checking for an Smi too, in opposition to what the comment above the test (copied from the spec) says.
The CQ bit was checked by henrique.ferreiro@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29388)
I think I have addressed every issue. If you think it is ok, I'll commit the code.
https://codereview.chromium.org/2431223005/diff/200001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2431223005/diff/200001/src/objects.cc#newcode... src/objects.cc:17454: (str = Object::ToString(isolate, value).ToHandleChecked()) Please put the handle assignment on its own statement, separate from the SameValue call. Otherwise LGTM
On 2016/11/27 23:01:00, caitp wrote: > https://codereview.chromium.org/2431223005/diff/200001/src/objects.cc > File src/objects.cc (right): > > https://codereview.chromium.org/2431223005/diff/200001/src/objects.cc#newcode... > src/objects.cc:17454: (str = Object::ToString(isolate, value).ToHandleChecked()) > Please put the handle assignment on its own statement, separate from the > SameValue call. Otherwise LGTM Done. I have reworked the code a bit to avoid code repetition. If this looks ok, I'll commit.
lgtm
The CQ bit was checked by henrique.ferreiro@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org, caitp@igalia.com Link to the patchset: https://codereview.chromium.org/2431223005/#ps220001 (title: "move assignment out of comparison")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29512)
On 2016/11/28 21:48:52, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > v8_presubmit on master.tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29512) sorry about that. can you run `git cl format .`, recommit, and try again? Thanks
On 2016/11/28 22:49:33, caitp wrote: > On 2016/11/28 21:48:52, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > v8_presubmit on master.tryserver.v8 (JOB_FAILED, > > http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29512) > > sorry about that. can you run `git cl format .`, recommit, and try again? Thanks My fault, I stubbornly tried to preserve the existing spacing between functions.
It doesn't look like this patch does anything to collect data on how often the new exceptions are thrown. Are we confident we'll find out if there's a problem breaking real sites via bug reports before this hits stable? Otherwise, maybe we should add a UseCounter.
On 2016/11/28 22:54:36, Dan Ehrenberg wrote: > It doesn't look like this patch does anything to collect data on how often the > new exceptions are thrown. Are we confident we'll find out if there's a problem > breaking real sites via bug reports before this hits stable? Otherwise, maybe we > should add a UseCounter. It is actually the opposite case. The bug report was about the code always throwing, even when called using the Reflect API.
https://codereview.chromium.org/2431223005/diff/240001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/240001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:34: () => Object.defineProperty(o, '0', {get: function() {}}), TypeError); Looks like this case started to throw when it previously didn't.
https://codereview.chromium.org/2431223005/diff/240001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/240001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:34: () => Object.defineProperty(o, '0', {get: function() {}}), TypeError); On 2016/11/28 23:01:40, Dan Ehrenberg wrote: > Looks like this case started to throw when it previously didn't. Defining accessors on typed arrays used to fail silently, but this was not spec behaviour. all other impls ive tried match the spec here.
On 2016/11/28 23:01:40, Dan Ehrenberg wrote: > https://codereview.chromium.org/2431223005/diff/240001/test/mjsunit/element-a... > File test/mjsunit/element-accessor.js (right): > > https://codereview.chromium.org/2431223005/diff/240001/test/mjsunit/element-a... > test/mjsunit/element-accessor.js:34: () => Object.defineProperty(o, '0', {get: > function() {}}), TypeError); > Looks like this case started to throw when it previously didn't. Yes, I missed that. That case appears to happen only when defining an out-of-bounds accessor. I will take a look at using UseCounters anyway, if that is the standard approach.
https://codereview.chromium.org/2431223005/diff/240001/test/mjsunit/element-a... File test/mjsunit/element-accessor.js (right): https://codereview.chromium.org/2431223005/diff/240001/test/mjsunit/element-a... test/mjsunit/element-accessor.js:34: () => Object.defineProperty(o, '0', {get: function() {}}), TypeError); On 2016/11/28 23:10:10, caitp wrote: > On 2016/11/28 23:01:40, Dan Ehrenberg wrote: > > Looks like this case started to throw when it previously didn't. > > Defining accessors on typed arrays used to fail silently, but this was not spec > behaviour. all other impls ive tried match the spec here. OK, if we're not the first to ship this fix, I'm fine without the UseCounter.
lgtm
The CQ bit was checked by henrique.ferreiro@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com, cbruni@chromium.org, jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2431223005/#ps240001 (title: "formatting fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1480375844067770,
"parent_rev": "b2f9d36d447082da445b052b156468c97b72ea23", "commit_rev":
"50ef4a4033bb8fc60774e33407ff0a80e0f6ef89"}
Message was sent while issue was closed.
Description was changed from ========== Implement DefineOwnProperty for TypedArrays TypedArrays need specific checks before calling OrdinaryDefineOwnProperty. BUG=v8:5328 ========== to ========== Implement DefineOwnProperty for TypedArrays TypedArrays need specific checks before calling OrdinaryDefineOwnProperty. BUG=v8:5328 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Implement DefineOwnProperty for TypedArrays TypedArrays need specific checks before calling OrdinaryDefineOwnProperty. BUG=v8:5328 ========== to ========== Implement DefineOwnProperty for TypedArrays TypedArrays need specific checks before calling OrdinaryDefineOwnProperty. BUG=v8:5328 Committed: https://crrev.com/bc1a3820c27ea869fcc7033ce4f72d9f2105992d Cr-Commit-Position: refs/heads/master@{#41333} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/bc1a3820c27ea869fcc7033ce4f72d9f2105992d Cr-Commit-Position: refs/heads/master@{#41333} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
