Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1814933002:
Throw the right exceptions from setting elements in Array.prototype.concat (Closed)
|
Created:
4 years, 9 months ago by Dan Ehrenberg Modified:
4 years, 9 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. |
DescriptionThrow the right exceptions from setting elements in Array.prototype.concat
This patch fixes two bugs in Array.prototype.concat in conjunction with
subclassing Arrays:
- Create a new property rather than calling Set when adding elements to
the output array. This means setters are not called.
- If there is an exception thrown from DefineProperty, propagate it
outwards properly, rather than swallowing it. This can occur, e.g., with
a Proxy as the new output array.
R=adamk
LOG=Y
BUG=chromium:595319
Committed: https://crrev.com/7acee1ef61ce2afd6dd8688499fad7475bcd82cd
Cr-Commit-Position: refs/heads/master@{#34876}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Ensure this works for frozen properties #Patch Set 3 : Depend on CreateDataProperty to throw #
Total comments: 7
Patch Set 4 : cleanups from review, with fewer DCHECKs #Patch Set 5 : format #Messages
Total messages: 26 (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/1814933002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814933002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
adamk@chromium.org changed reviewers: + cbruni@chromium.org
https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc File src/builtins.cc (right): https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc#newcode723 src/builtins.cc:723: &it, elm, AbstractCode::ShouldThrow::THROW_ON_ERROR); "Object::THROW_ON_ERROR" is the usual way of naming this enum. https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc#newcode725 src/builtins.cc:725: DCHECK(success.FromJust()); I agree that this ought to be the case, but I'm not sure our CreateDataProperty function agrees. See this code from JSObject::CreateDataProperty: if (it->IsFound()) { if (!it->IsConfigurable()) return Just(false); } else { if (!JSObject::IsExtensible(Handle<JSObject>::cast(it->GetReceiver()))) return Just(false); } Can you add tests cases for those two cases (one where '0' is present but non-configurable and one where the returned object doesn't have '0' and is non-extensible)? From my reading of the code those should throw but will not (and your DCHECK will fire). It seems like we may need some fixes in objects.cc; hope no existing code depends on this non-throwing behavior... https://codereview.chromium.org/1814933002/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-595319.js (right): https://codereview.chromium.org/1814933002/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-595319.js:7: // an element, and that elements are added to array subclasses appropraitely Typo: s/appropraitely/appropriately/
https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc File src/builtins.cc (right): https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc#newcode723 src/builtins.cc:723: &it, elm, AbstractCode::ShouldThrow::THROW_ON_ERROR); On 2016/03/17 at 19:22:21, adamk wrote: > "Object::THROW_ON_ERROR" is the usual way of naming this enum. Done https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc#newcode725 src/builtins.cc:725: DCHECK(success.FromJust()); On 2016/03/17 at 19:22:21, adamk wrote: > I agree that this ought to be the case, but I'm not sure our CreateDataProperty function agrees. See this code from JSObject::CreateDataProperty: > > if (it->IsFound()) { > if (!it->IsConfigurable()) return Just(false); > } else { > if (!JSObject::IsExtensible(Handle<JSObject>::cast(it->GetReceiver()))) > return Just(false); > } > > Can you add tests cases for those two cases (one where '0' is present but non-configurable and one where the returned object doesn't have '0' and is non-extensible)? > > From my reading of the code those should throw but will not (and your DCHECK will fire). > > It seems like we may need some fixes in objects.cc; hope no existing code depends on this non-throwing behavior... For this patch, I just fixed it on this side, WDYT? https://codereview.chromium.org/1814933002/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-595319.js (right): https://codereview.chromium.org/1814933002/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-595319.js:7: // an element, and that elements are added to array subclasses appropraitely On 2016/03/17 at 19:22:21, adamk wrote: > Typo: s/appropraitely/appropriately/ Done
https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc File src/builtins.cc (right): https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc#newcode725 src/builtins.cc:725: DCHECK(success.FromJust()); On 2016/03/17 20:26:33, Dan Ehrenberg wrote: > On 2016/03/17 at 19:22:21, adamk wrote: > > I agree that this ought to be the case, but I'm not sure our > CreateDataProperty function agrees. See this code from > JSObject::CreateDataProperty: > > > > if (it->IsFound()) { > > if (!it->IsConfigurable()) return Just(false); > > } else { > > if (!JSObject::IsExtensible(Handle<JSObject>::cast(it->GetReceiver()))) > > return Just(false); > > } > > > > Can you add tests cases for those two cases (one where '0' is present but > non-configurable and one where the returned object doesn't have '0' and is > non-extensible)? > > > > From my reading of the code those should throw but will not (and your DCHECK > will fire). > > > > It seems like we may need some fixes in objects.cc; hope no existing code > depends on this non-throwing behavior... > > For this patch, I just fixed it on this side, WDYT? I'd prefer it be fixed properly, the CreateDataProperty API is just wrong at the moment. I don't think the bug fix is urgent enough to want to do this out-of-order.
https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc File src/builtins.cc (right): https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc#newcode725 src/builtins.cc:725: DCHECK(success.FromJust()); On 2016/03/17 20:33:59, adamk wrote: > On 2016/03/17 20:26:33, Dan Ehrenberg wrote: > > On 2016/03/17 at 19:22:21, adamk wrote: > > > I agree that this ought to be the case, but I'm not sure our > > CreateDataProperty function agrees. See this code from > > JSObject::CreateDataProperty: > > > > > > if (it->IsFound()) { > > > if (!it->IsConfigurable()) return Just(false); > > > } else { > > > if (!JSObject::IsExtensible(Handle<JSObject>::cast(it->GetReceiver()))) > > > return Just(false); > > > } > > > > > > Can you add tests cases for those two cases (one where '0' is present but > > non-configurable and one where the returned object doesn't have '0' and is > > non-extensible)? > > > > > > From my reading of the code those should throw but will not (and your DCHECK > > will fire). > > > > > > It seems like we may need some fixes in objects.cc; hope no existing code > > depends on this non-throwing behavior... > > > > For this patch, I just fixed it on this side, WDYT? > > I'd prefer it be fixed properly, the CreateDataProperty API is just wrong at the > moment. I don't think the bug fix is urgent enough to want to do this > out-of-order. I do think it would make sense to do the CreateDataProperty fix in a separate patch, though; I just think it should land before this one.
https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc File src/builtins.cc (right): https://codereview.chromium.org/1814933002/diff/1/src/builtins.cc#newcode725 src/builtins.cc:725: DCHECK(success.FromJust()); On 2016/03/17 at 20:39:54, adamk wrote: > On 2016/03/17 20:33:59, adamk wrote: > > On 2016/03/17 20:26:33, Dan Ehrenberg wrote: > > > On 2016/03/17 at 19:22:21, adamk wrote: > > > > I agree that this ought to be the case, but I'm not sure our > > > CreateDataProperty function agrees. See this code from > > > JSObject::CreateDataProperty: > > > > > > > > if (it->IsFound()) { > > > > if (!it->IsConfigurable()) return Just(false); > > > > } else { > > > > if (!JSObject::IsExtensible(Handle<JSObject>::cast(it->GetReceiver()))) > > > > return Just(false); > > > > } > > > > > > > > Can you add tests cases for those two cases (one where '0' is present but > > > non-configurable and one where the returned object doesn't have '0' and is > > > non-extensible)? > > > > > > > > From my reading of the code those should throw but will not (and your DCHECK > > > will fire). > > > > > > > > It seems like we may need some fixes in objects.cc; hope no existing code > > > depends on this non-throwing behavior... > > > > > > For this patch, I just fixed it on this side, WDYT? > > > > I'd prefer it be fixed properly, the CreateDataProperty API is just wrong at the > > moment. I don't think the bug fix is urgent enough to want to do this > > out-of-order. > > I do think it would make sense to do the CreateDataProperty fix in a separate patch, though; I just think it should land before this one. This new version depends on https://codereview.chromium.org/1809233002 to throw instead.
Looking good, but some suggestions on matching the surrounding coding style. https://codereview.chromium.org/1814933002/diff/40001/src/builtins.cc File src/builtins.cc (right): https://codereview.chromium.org/1814933002/diff/40001/src/builtins.cc#newcode709 src/builtins.cc:709: bool visit(uint32_t i, Handle<Object> elm) { Can you add a MUST_USE_RESULT on the front of this? That'll keep future code from making the same mistake. https://codereview.chromium.org/1814933002/diff/40001/src/builtins.cc#newcode724 src/builtins.cc:724: if (!success.IsJust()) return false; The usual way of writing this is: MAYBE_RETURN(JSReceiver::CreateDataProperty(...), false); That won't get you the DCHECK, but I think that's OK, since THROW_ON_ERROR as part of the API implies that IsJust implies success anyway. https://codereview.chromium.org/1814933002/diff/40001/src/builtins.cc#newcode... src/builtins.cc:1413: if (isolate->has_pending_exception()) return isolate->heap()->exception(); The pattern elsewhere in this file is: if (!SomethingThatMightThrow()) return isolate->heap()->exception() For example, the IterateElements call just above. Please use that model here too.
https://codereview.chromium.org/1814933002/diff/40001/src/builtins.cc File src/builtins.cc (right): https://codereview.chromium.org/1814933002/diff/40001/src/builtins.cc#newcode709 src/builtins.cc:709: bool visit(uint32_t i, Handle<Object> elm) { On 2016/03/17 at 21:31:31, adamk wrote: > Can you add a MUST_USE_RESULT on the front of this? That'll keep future code from making the same mistake. Done https://codereview.chromium.org/1814933002/diff/40001/src/builtins.cc#newcode724 src/builtins.cc:724: if (!success.IsJust()) return false; On 2016/03/17 at 21:31:31, adamk wrote: > The usual way of writing this is: > > MAYBE_RETURN(JSReceiver::CreateDataProperty(...), false); > > That won't get you the DCHECK, but I think that's OK, since THROW_ON_ERROR as part of the API implies that IsJust implies success anyway. Done (I'm sad about the lost DCHECK though) https://codereview.chromium.org/1814933002/diff/40001/src/builtins.cc#newcode... src/builtins.cc:1413: if (isolate->has_pending_exception()) return isolate->heap()->exception(); On 2016/03/17 at 21:31:31, adamk wrote: > The pattern elsewhere in this file is: > > if (!SomethingThatMightThrow()) return isolate->heap()->exception() > > For example, the IterateElements call just above. Please use that model here too. Done (but I'm also sad about this DCHECK)
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/1814933002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814933002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/12551)
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/1814933002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814933002/80001
lgtm % formatting (which I think is why the presubmit is failing) https://codereview.chromium.org/1814933002/diff/40001/src/builtins.cc File src/builtins.cc (right): https://codereview.chromium.org/1814933002/diff/40001/src/builtins.cc#newcode... src/builtins.cc:1413: if (isolate->has_pending_exception()) return isolate->heap()->exception(); On 2016/03/17 22:11:01, Dan Ehrenberg wrote: > On 2016/03/17 at 21:31:31, adamk wrote: > > The pattern elsewhere in this file is: > > > > if (!SomethingThatMightThrow()) return isolate->heap()->exception() > > > > For example, the IterateElements call just above. Please use that model here > too. > > Done (but I'm also sad about this DCHECK) Not sure what's really to be done here, this pattern shows up in a million places throughout the codebase, so one DCHECK isn't going to save us. I think it's mostly just a consequence of trying to do exceptions without...using exceptions.
The CQ bit was unchecked by littledan@chromium.org
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/1814933002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814933002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Throw the right exceptions from setting elements in Array.prototype.concat This patch fixes two bugs in Array.prototype.concat in conjunction with subclassing Arrays: - Create a new property rather than calling Set when adding elements to the output array. This means setters are not called. - If there is an exception thrown from DefineProperty, propagate it outwards properly, rather than swallowing it. This can occur, e.g., with a Proxy as the new output array. R=adamk LOG=Y BUG=chromium:595319 ========== to ========== Throw the right exceptions from setting elements in Array.prototype.concat This patch fixes two bugs in Array.prototype.concat in conjunction with subclassing Arrays: - Create a new property rather than calling Set when adding elements to the output array. This means setters are not called. - If there is an exception thrown from DefineProperty, propagate it outwards properly, rather than swallowing it. This can occur, e.g., with a Proxy as the new output array. R=adamk LOG=Y BUG=chromium:595319 Committed: https://crrev.com/7acee1ef61ce2afd6dd8688499fad7475bcd82cd Cr-Commit-Position: refs/heads/master@{#34876} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7acee1ef61ce2afd6dd8688499fad7475bcd82cd Cr-Commit-Position: refs/heads/master@{#34876} |
