Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2677653002:
Fix receiver checks for v8::Function on a remote context. (Closed)
DescriptionFix receiver checks for v8::Function on a remote context receiver.
v8 allows the embedder to specify a global template to use when
creating a new context. However, v8 does not use the supplied
template directly when creating the global proxy: it creates a
unique template for each global proxy. However, this is problematic
for remote contexts: functions cannot use strict receiver checks
with the remote context, as the global template will never match
the global proxy.
To fix this, remote contexts now also include a remote global
object in the prototype chain that is instantiated with the global
template. This mirrors the way the global proxy is configured for a
full v8 context, and allows strict receiver checks to work.
BUG=527190
Review-Url: https://codereview.chromium.org/2677653002
Cr-Commit-Position: refs/heads/master@{#43361}
Committed: https://chromium.googlesource.com/v8/v8/+/96eda1f7d166cdeb6da643505edd70561f503ed8
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : Fix comments #
Total comments: 3
Patch Set 5 : Rebase to ToT #Patch Set 6 : Fix bad cast. #
Total comments: 2
Patch Set 7 : . #
Messages
Total messages: 31 (18 generated)
Description was changed from ========== Make it possible for v8::Function to be invoked on a remote context. BUG= ========== to ========== Fix receiver checks for v8::Function on a remote context. v8 allows the embedder to specify a global template to use when creating a new context. However, v8 does not use the supplied template directly when creating the global proxy: it creates a unique template for each global proxy. However, this is problematic for remote contexts: functions cannot use strict receiver checks with the remote context's global proxy, as the global template will never match the global proxy. To fix this, remote contexts now also include a remote global object in the prototype chain that is instantiated with the global template. This mirrors the way the global proxy is configured for a full v8 context, and allows strict receiver checks to work. BUG=527190 ==========
dcheng@chromium.org changed reviewers: + jochen@chromium.org, verwaest@chromium.org
Description was changed from ========== Fix receiver checks for v8::Function on a remote context. v8 allows the embedder to specify a global template to use when creating a new context. However, v8 does not use the supplied template directly when creating the global proxy: it creates a unique template for each global proxy. However, this is problematic for remote contexts: functions cannot use strict receiver checks with the remote context's global proxy, as the global template will never match the global proxy. To fix this, remote contexts now also include a remote global object in the prototype chain that is instantiated with the global template. This mirrors the way the global proxy is configured for a full v8 context, and allows strict receiver checks to work. BUG=527190 ========== to ========== Fix receiver checks for v8::Function on a remote context receiver. v8 allows the embedder to specify a global template to use when creating a new context. However, v8 does not use the supplied template directly when creating the global proxy: it creates a unique template for each global proxy. However, this is problematic for remote contexts: functions cannot use strict receiver checks with the remote context, as the global template will never match the global proxy. To fix this, remote contexts now also include a remote global object in the prototype chain that is instantiated with the global template. This mirrors the way the global proxy is configured for a full v8 context, and allows strict receiver checks to work. BUG=527190 ==========
https://codereview.chromium.org/2677653002/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2677653002/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:4825: global_proxy->map()->set_has_hidden_prototype(true); I don't actually understand why this is needed, so any insight here would be great. Alternatively... I could just delete line 4812, but since I'd like to better understand what's going on here...
https://codereview.chromium.org/2677653002/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2677653002/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:4825: global_proxy->map()->set_has_hidden_prototype(true); On 2017/02/17 at 07:29:08, dcheng wrote: > I don't actually understand why this is needed, so any insight here would be great. Alternatively... I could just delete line 4812, but since I'd like to better understand what's going on here... ForceSetPrototype creates a new map
https://codereview.chromium.org/2677653002/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2677653002/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:4825: global_proxy->map()->set_has_hidden_prototype(true); On 2017/02/17 08:40:26, jochen wrote: > On 2017/02/17 at 07:29:08, dcheng wrote: > > I don't actually understand why this is needed, so any insight here would be > great. Alternatively... I could just delete line 4812, but since I'd like to > better understand what's going on here... > > ForceSetPrototype creates a new map Initialization of a full context does this too; yet when ConfigureGlobalObjects() is done, the global proxy is marked as having a hidden prototype. I guess I'm not understanding how all the pieces fit together here. Some questions that might help me understand... 1) Is manually calling set_has_hidden_prototype here wrong? 2) Is there any purpose to calling set_has_hidden_prototype(true) on the global_proxy_map? 3) Is there any v8 newbie docs to better understand the various objects and how they relate? I kind of understand how some of the high-level pieces fit together, but things like how maps and objects interact is still pretty mysterious to me.
Also, I have an alternate approach that turns HasInstanceInGlobalProxy() into a JSGlobalProxy method and updates CheckCompatibleReceiver() to also consult the method. This allows us to keep using v8::Signature for the receiver check. However, in the end, I thought it was safer to emulate what the full v8 context setup looks like: I was afraid of missing other areas in v8 (not covered by Blink tests) that require special JSGlobalProxy handling.
On 2017/02/17 at 10:08:27, dcheng wrote: > https://codereview.chromium.org/2677653002/diff/60001/src/bootstrapper.cc > File src/bootstrapper.cc (right): > > https://codereview.chromium.org/2677653002/diff/60001/src/bootstrapper.cc#new... > src/bootstrapper.cc:4825: global_proxy->map()->set_has_hidden_prototype(true); > On 2017/02/17 08:40:26, jochen wrote: > > On 2017/02/17 at 07:29:08, dcheng wrote: > > > I don't actually understand why this is needed, so any insight here would be > > great. Alternatively... I could just delete line 4812, but since I'd like to > > better understand what's going on here... > > > > ForceSetPrototype creates a new map > > Initialization of a full context does this too; yet when ConfigureGlobalObjects() is done, the global proxy is marked as having a hidden prototype. I guess I'm not understanding how all the pieces fit together here. Some questions that might help me understand... > > 1) Is manually calling set_has_hidden_prototype here wrong? no, I think it's correct. I'd have to step through the code with a debugger to figure out where this is done for non-remote contexts.. > 2) Is there any purpose to calling set_has_hidden_prototype(true) on the global_proxy_map? global proxy and global object should appear to be one object to JS > 3) Is there any v8 newbie docs to better understand the various objects and how they relate? I kind of understand how some of the high-level pieces fit together, but things like how maps and objects interact is still pretty mysterious to me. go/v8 > Helpful document & go/v8cheat has some docs that might help
anyway, lgtm if the tests pass
The CQ bit was checked by dcheng@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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/21460) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
PTAL, I fixed a bad cast in FunctionTemplate::HasInstance, since the global object of a JSGlobalProxy is no longer always a JSGlobalObject (due to the placeholder object for a remote global proxy).
The CQ bit was checked by dcheng@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2677653002/diff/100001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2677653002/diff/100001/src/api.cc#newcode6574 src/api.cc:6574: if (iter.IsAtEnd()) return false; how can this happen? Shouldn't there now always be the placeholder?
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
https://codereview.chromium.org/2677653002/diff/100001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2677653002/diff/100001/src/api.cc#newcode6574 src/api.cc:6574: if (iter.IsAtEnd()) return false; On 2017/02/22 09:03:14, jochen wrote: > how can this happen? Shouldn't there now always be the placeholder? I was reverting this to the original version, but I think the only time this would be null is if the JSGlobalProxy is detached. However, calling HasInstance() in that case should be a bug, so I changed this to a DCHECK
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/22 09:29:53, dcheng wrote: > https://codereview.chromium.org/2677653002/diff/100001/src/api.cc > File src/api.cc (right): > > https://codereview.chromium.org/2677653002/diff/100001/src/api.cc#newcode6574 > src/api.cc:6574: if (iter.IsAtEnd()) return false; > On 2017/02/22 09:03:14, jochen wrote: > > how can this happen? Shouldn't there now always be the placeholder? > > I was reverting this to the original version, but I think the only time this > would be null is if the JSGlobalProxy is detached. However, calling > HasInstance() in that case should be a bug, so I changed this to a DCHECK I ran the v8 unit tests (enabling all the asserts... I think), and layout tests in regular and in --site-per-process mode, and they agree.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by dcheng@chromium.org
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": 120001, "attempt_start_ts": 1487758245390820,
"parent_rev": "3f303da2920e21f631325e54904290ba20b86827", "commit_rev":
"96eda1f7d166cdeb6da643505edd70561f503ed8"}
Message was sent while issue was closed.
Description was changed from ========== Fix receiver checks for v8::Function on a remote context receiver. v8 allows the embedder to specify a global template to use when creating a new context. However, v8 does not use the supplied template directly when creating the global proxy: it creates a unique template for each global proxy. However, this is problematic for remote contexts: functions cannot use strict receiver checks with the remote context, as the global template will never match the global proxy. To fix this, remote contexts now also include a remote global object in the prototype chain that is instantiated with the global template. This mirrors the way the global proxy is configured for a full v8 context, and allows strict receiver checks to work. BUG=527190 ========== to ========== Fix receiver checks for v8::Function on a remote context receiver. v8 allows the embedder to specify a global template to use when creating a new context. However, v8 does not use the supplied template directly when creating the global proxy: it creates a unique template for each global proxy. However, this is problematic for remote contexts: functions cannot use strict receiver checks with the remote context, as the global template will never match the global proxy. To fix this, remote contexts now also include a remote global object in the prototype chain that is instantiated with the global template. This mirrors the way the global proxy is configured for a full v8 context, and allows strict receiver checks to work. BUG=527190 Review-Url: https://codereview.chromium.org/2677653002 Cr-Commit-Position: refs/heads/master@{#43361} Committed: https://chromium.googlesource.com/v8/v8/+/96eda1f7d166cdeb6da643505edd70561f5... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/v8/v8/+/96eda1f7d166cdeb6da643505edd70561f5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
