Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1406293011:
Re-ship @@toStringTag (Closed)
DescriptionRe-ship @@toStringTag
It was originally shipped in https://crrev.com/eef2b9b09723ba1dae3ec0172341e93e9030ada0,
but was reverted due to poor interaction with Blink.
That interaction seems to be fixed thanks to changes to the V8 API
and to @@toStringTag handling on access-checked objects.
BUG=v8:3502
LOG=y
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.v8:v8_linux_nosnap_rel;tryserver.blink:linux_blink_rel
Committed: https://crrev.com/2fa4732739907bd8e52a421a5243184cd4e765d3
Cr-Commit-Position: refs/heads/master@{#31846}
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Added API tests to answer my security question #
Messages
Total messages: 28 (14 generated)
Description was changed from ========== Ship --harmony-tostring ========== to ========== Ship --harmony-tostring CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng,tryserver.v8:v8_linux_nosnap_rel,tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by adamk@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/1406293011/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406293011/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Transient error: Too many "tryserver.chromium.linux:linux_chromium_rel_ng,tryserver.v8:v8_linux_nosnap_rel,tryserver.blink:linux_blink_rel" delimiters in ":". Correct syntax is "tryserver:bot1,bot2;tryserver2:bot3,bot4;".
Description was changed from ========== Ship --harmony-tostring CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng,tryserver.v8:v8_linux_nosnap_rel,tryserver.blink:linux_blink_rel ========== to ========== Ship --harmony-tostring CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.v8:v8_linux_nosnap_rel;tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by adamk@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/1406293011/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406293011/1
Description was changed from ========== Ship --harmony-tostring CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.v8:v8_linux_nosnap_rel;tryserver.blink:linux_blink_rel ========== to ========== Re-ship @@toStringTag It was originally shipped in https://crrev.com/eef2b9b09723ba1dae3ec0172341e93e9030ada0, but was reverted due to poor interaction with Blink. That interaction seems to be fixed thanks to changes to the V8 API and to @@toStringTag handling on access-checked objects. BUG=v8:3502 LOG=y CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.v8:v8_linux_nosnap_rel;tryserver.blink:linux_blink_rel ==========
adamk@chromium.org changed reviewers: + rossberg@chromium.org
adamk@chromium.org changed reviewers: + verwaest@chromium.org
+verwaest to audit security; would you mind taking a look at v8::Object::ObjectProtoToString() in api.cc and checking my work that it'll do the appropriate access checks when trying to get the [Symbol.toStringTag] property on the receiver if the receiver is access checked? I don't see anything special going on, but I have a vague memory of something security-related going on here. On Window, the toString method itself is already vended per-Context (using special bindings generator code in Blink).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM on the contents of this CL, deferring to Toon for access checks audit.
The CQ bit was checked by adamk@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/1406293011/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406293011/20001
The CQ bit was checked by adamk@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/1406293011/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406293011/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've added a test to test the exact thing I was worried about, and it passes. Therefore I'm going to land this and get verwaest to asynchronously verify that this stuff is correct, in the interest of getting some canary coverage with the flag flipped to on before the branch point.
The CQ bit was checked by adamk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/1406293011/#ps40001 (title: "Added API tests to answer my security question")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406293011/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406293011/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2fa4732739907bd8e52a421a5243184cd4e765d3 Cr-Commit-Position: refs/heads/master@{#31846} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
