Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1821723004:
Ensure CreateDataProperty works correctly on TypedArrays (Closed)
|
Created:
4 years, 9 months ago by Dan Ehrenberg Modified:
4 years, 8 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionEnsure CreateDataProperty works correctly on TypedArrays
Previously, CreateDataProperty would fail a DCHECK when used to create
an integer indexed property on a TypedArray. This patch makes it throw
a TypeError instead. The issue came up when Array.prototype.concat
was repaired to use CreateDataProperty rather than SetElement; concat
can be tricked into making a new TypedArray if it is given an Array
whose prototype is a TypedArray. This patch prevents the issue.
R=adamk
LOG=Y
BUG=chromium:596394
Committed: https://crrev.com/7a38462e8b0fb25543ce8975045bc1e13d11cd34
Cr-Commit-Position: refs/heads/master@{#35271}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use appropriate internal API #
Total comments: 2
Patch Set 3 : reverse polarity of test #
Messages
Total messages: 28 (9 generated)
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/patch-status/1821723004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821723004/1
https://codereview.chromium.org/1821723004/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1821723004/diff/1/src/objects.cc#newcode6691 src/objects.cc:6691: if (it->state() == LookupIterator::INTEGER_INDEXED_EXOTIC || Why is this needed? DefineOwnPropertyIgnoreAttributes looks like it already handles this state, and calls RedefineIncompatibleProperty(), which throws the same TypeError. https://codereview.chromium.org/1821723004/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-crbug-596394.js (right): https://codereview.chromium.org/1821723004/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-crbug-596394.js:6: // When concat makes a new integer-indexed exotic object, the resulting properties Nit: please wrap to 80 cols.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
littledan@chromium.org changed reviewers: + verwaest@chromium.org
Toon, how would you suggest structuring this code? https://codereview.chromium.org/1821723004/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1821723004/diff/1/src/objects.cc#newcode6691 src/objects.cc:6691: if (it->state() == LookupIterator::INTEGER_INDEXED_EXOTIC || On 2016/03/21 at 21:44:01, adamk wrote: > Why is this needed? DefineOwnPropertyIgnoreAttributes looks like it already handles this state, and calls RedefineIncompatibleProperty(), which throws the same TypeError. If there isn't a check here, then IsConfigurable() throws because INTEGER_INDEXED_EXOTIC doesn't have property_details_.
In particular, it seems to me like it's something of a footgun in the LookupIterator API that IsFound() followed by IsConfigurable() can cause a DCHECK failure, and I'm interested what Toon would suggest with respect to this API.
On 2016/03/21 at 22:16:30, adamk wrote: > In particular, it seems to me like it's something of a footgun in the LookupIterator API that IsFound() followed by IsConfigurable() can cause a DCHECK failure, and I'm interested what Toon would suggest with respect to this API. Ping. Could someone take a look?
https://codereview.chromium.org/1821723004/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1821723004/diff/1/src/objects.cc#newcode6691 src/objects.cc:6691: if (it->state() == LookupIterator::INTEGER_INDEXED_EXOTIC || On 2016/03/21 22:08:43, Dan Ehrenberg wrote: > On 2016/03/21 at 21:44:01, adamk wrote: > > Why is this needed? DefineOwnPropertyIgnoreAttributes looks like it already > handles this state, and calls RedefineIncompatibleProperty(), which throws the > same TypeError. > > If there isn't a check here, then IsConfigurable() throws because > INTEGER_INDEXED_EXOTIC doesn't have property_details_. IsConfigurable should not be exposed at all. You need to call GetPropertyAttributes(it) in case of IsFound() to get to the actual attributes. The cached property_details_ are only valid for data properties on the object; not for INTERCEPTOR, ACCESS_CHECK, INTEGER_INDEXED_EXOTIC. For INTEGER_INDEXED this will claim ABSENT, but that's fine since DefineOwnPropertyIgnoreAttributes throws the correct error as Adam mentioned.
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/patch-status/1821723004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821723004/20001
https://codereview.chromium.org/1821723004/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1821723004/diff/1/src/objects.cc#newcode6691 src/objects.cc:6691: if (it->state() == LookupIterator::INTEGER_INDEXED_EXOTIC || On 2016/03/31 at 15:08:03, Toon Verwaest wrote: > On 2016/03/21 22:08:43, Dan Ehrenberg wrote: > > On 2016/03/21 at 21:44:01, adamk wrote: > > > Why is this needed? DefineOwnPropertyIgnoreAttributes looks like it already > > handles this state, and calls RedefineIncompatibleProperty(), which throws the > > same TypeError. > > > > If there isn't a check here, then IsConfigurable() throws because > > INTEGER_INDEXED_EXOTIC doesn't have property_details_. > > IsConfigurable should not be exposed at all. You need to call GetPropertyAttributes(it) in case of IsFound() to get to the actual attributes. The cached property_details_ are only valid for data properties on the object; not for INTERCEPTOR, ACCESS_CHECK, INTEGER_INDEXED_EXOTIC. For INTEGER_INDEXED this will claim ABSENT, but that's fine since DefineOwnPropertyIgnoreAttributes throws the correct error as Adam mentioned. I guess all of those cases could actually come up in concat, as a species constructor can return anything. What do you think of the new patch?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...) v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/17899)
https://codereview.chromium.org/1821723004/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1821723004/diff/20001/src/objects.cc#newcode6690 src/objects.cc:6690: if (!(attributes.FromJust() & DONT_DELETE)) { Sadly, because V8 predates ES5, DONT_DELETE is the opposite of IsConfigurable, so you need to remove the leading '!'. This is usually spelled: if ((attributes.FromJust() & DONT_DELETE) != 0) { ... }
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/patch-status/1821723004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821723004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
https://codereview.chromium.org/1821723004/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/1821723004/diff/20001/src/objects.cc#newcode6690 src/objects.cc:6690: if (!(attributes.FromJust() & DONT_DELETE)) { On 2016/04/05 at 00:04:32, adamk wrote: > Sadly, because V8 predates ES5, DONT_DELETE is the opposite of IsConfigurable, so you need to remove the leading '!'. This is usually spelled: > > if ((attributes.FromJust() & DONT_DELETE) != 0) { ... } Fixed
lgtm
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821723004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821723004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Ensure CreateDataProperty works correctly on TypedArrays Previously, CreateDataProperty would fail a DCHECK when used to create an integer indexed property on a TypedArray. This patch makes it throw a TypeError instead. The issue came up when Array.prototype.concat was repaired to use CreateDataProperty rather than SetElement; concat can be tricked into making a new TypedArray if it is given an Array whose prototype is a TypedArray. This patch prevents the issue. R=adamk LOG=Y BUG=chromium:596394 ========== to ========== Ensure CreateDataProperty works correctly on TypedArrays Previously, CreateDataProperty would fail a DCHECK when used to create an integer indexed property on a TypedArray. This patch makes it throw a TypeError instead. The issue came up when Array.prototype.concat was repaired to use CreateDataProperty rather than SetElement; concat can be tricked into making a new TypedArray if it is given an Array whose prototype is a TypedArray. This patch prevents the issue. R=adamk LOG=Y BUG=chromium:596394 Committed: https://crrev.com/7a38462e8b0fb25543ce8975045bc1e13d11cd34 Cr-Commit-Position: refs/heads/master@{#35271} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7a38462e8b0fb25543ce8975045bc1e13d11cd34 Cr-Commit-Position: refs/heads/master@{#35271} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
