Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2626183003:
Switch RemoteWindowProxy to use v8::Context::NewRemoteContext. (Closed)
|
Created:
3 years, 11 months ago by dcheng Modified:
3 years, 9 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, mlamouri+watch-blink_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, blink-reviews, chromium-apps-reviews_chromium.org, kinuko+watch, blink-reviews-api_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch RemoteWindowProxy to use v8::Context::NewRemoteContext.
BUG=527190
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2626183003
Cr-Commit-Position: refs/heads/master@{#453128}
Committed: https://chromium.googlesource.com/chromium/src/+/4a40a757e7029a3f59b12543cbf617eceb15f7f0
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : Finish partial comment #
Total comments: 5
Patch Set 5 : Add DCHECK for detached global proxy #Patch Set 6 : rebase #Patch Set 7 : . #
Total comments: 11
Patch Set 8 : rebase #
Total comments: 9
Patch Set 9 : comments #
Total comments: 4
Patch Set 10 : . #
Total comments: 8
Patch Set 11 : comments #Patch Set 12 : Consolidate comments #Patch Set 13 : EventTarget as well #
Total comments: 19
Patch Set 14 : Comment all the things #Patch Set 15 : . #Patch Set 16 : Another comment #
Total comments: 4
Patch Set 17 : rebase #Patch Set 18 : rebase #
Total comments: 10
Patch Set 19 : Add DCHECK #Patch Set 20 : Address comment. #Patch Set 21 : rebase #Messages
Total messages: 101 (48 generated)
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 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...
Description was changed from ========== Switch RemoteWindowProxy to use v8::Context::NewRemoteContext. Not ready for review yet. BUG=527190 ========== to ========== Switch RemoteWindowProxy to use v8::Context::NewRemoteContext. BUG=527190 ==========
dcheng@chromium.org changed reviewers: + haraken@chromium.org, jochen@chromium.org, yukishiino@chromium.org
https://codereview.chromium.org/2626183003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.h (right): https://codereview.chromium.org/2626183003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.h:98: RefPtr<ScriptState> m_scriptState; The changes in LocalWindowProxy are just moving code from the WindowProxy base class. https://codereview.chromium.org/2626183003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:54: V8DOMWrapper::clearNativeInfo(isolate(), m_globalProxy.newLocal(isolate())); Note: I intentionally didn't update the cross-origin interceptors to handle a null case here. In theory, there should be no code that tries to access the remote window proxy while it's being swapped. https://codereview.chromium.org/2626183003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/2626183003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:855: return frame->script().windowProxy(world)->contextIfInitialized(); For now, necessary to get to the WindowProxy as a LocalWindowProxy. I am looking at simplifying this in a followup by making this non-virtual. https://codereview.chromium.org/2626183003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (left): https://codereview.chromium.org/2626183003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:106: DCHECK(m_globalProxy == m_scriptState->context()->Global()); It's not possible to do these checks without a context. I also didn't want to make globalIfNotDetached() virtual... so I think we don't have much of a choice here =( https://codereview.chromium.org/2626183003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:117: ASSERT(m_scriptState->isGlobalObjectDetached()); I'm going to move this bool to be tracked in the WindowProxy base class so we don't lose this ASSERT. Will upload PS shortly.
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Hmm. Unfortunately it looks like my machine died, so I'm done for the night. As I understand, there are two outstanding bugs: - v8 is throwing illegal invocation exceptions while calling functions on the remote window proxy's global proxy - while debugging the first problem, I noticed that accessing the creation context of the object returned by NewRemoteContext() doesn't seem to work--inspector crashes when trying to enumerate the properties of the remote window proxy's global proxy. I have some ideas of how to solve the second, but the first problem is pretty strange: I will investigate more when I get my machine working again...
PTAL. https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:109: V8DOMWrapper::setNativeInfo(isolate(), m_globalProxy.newLocal(isolate()), Note: this recently changed in https://codereview.chromium.org/2617733004/, and I'm still investigating if it's important to use associateObjectWIthWrapper for remote contexts. It doesn't work as is, because it depends on ScriptState::from() to look up the current world, which doesn't work for a remote context. https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:77: << "Context not detached by calling clearForNavigation()"; DCHECK() requires that I guard this with #if DCHECK_IS_ON() which looks silly... so I chose DLOG_IF() instead (which explicitly promises not to reference the condition) https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:24: {% if method.returns_promise or (interface_name == 'Window' and method.is_cross_origin) %} This is needed because a remote context only has a JSGlobalPoxy and no JSGlobalObject, so the strict receiver [1] in v8 fails otherwise. I tried to fix this with https://codereview.chromium.org/2677653002/, but wasn't able to make a lot of progress. [1] https://chromium.googlesource.com/v8/v8/+/d1055c1fbddb75d628ef6f74a739ea67e28...
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/02/14 10:04:38, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > ios-device on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) > ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) Ah, it looks like https://codereview.chromium.org/2617733004/ was reverted... rebasing.
On 2017/02/14 10:12:41, dcheng wrote: > On 2017/02/14 10:04:38, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > ios-device on master.tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) > > ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) > > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) > > Ah, it looks like https://codereview.chromium.org/2617733004/ was reverted... > rebasing. Sorry for inconvenience. That CL passed CQ, but caused crashes in prod, so I had to revert it. Will try to land it again.
On 2017/02/14 10:14:28, Yuki wrote: > On 2017/02/14 10:12:41, dcheng wrote: > > On 2017/02/14 10:04:38, commit-bot: I haz the power wrote: > > > Dry run: Try jobs failed on following builders: > > > ios-device on master.tryserver.chromium.mac (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) > > > ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) > > > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) > > > > Ah, it looks like https://codereview.chromium.org/2617733004/ was reverted... > > rebasing. > > Sorry for inconvenience. That CL passed CQ, but caused crashes in prod, so I > had to revert it. Will try to land it again. Can I be CCed on the reland so I can keep track of any rebasing needed? =)
On 2017/02/14 10:17:17, dcheng wrote: > On 2017/02/14 10:14:28, Yuki wrote: > > On 2017/02/14 10:12:41, dcheng wrote: > > > On 2017/02/14 10:04:38, commit-bot: I haz the power wrote: > > > > Dry run: Try jobs failed on following builders: > > > > ios-device on master.tryserver.chromium.mac (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) > > > > ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) > > > > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) > > > > > > Ah, it looks like https://codereview.chromium.org/2617733004/ was > reverted... > > > rebasing. > > > > Sorry for inconvenience. That CL passed CQ, but caused crashes in prod, so I > > had to revert it. Will try to land it again. > > Can I be CCed on the reland so I can keep track of any rebasing needed? =) Oh, I didn't mean that you need to wait for it. Please land this CL without waiting mine. I think I need some time to investigate the cause. I'll cc you if this CL is not yet be landed.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Overall looks good. https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:92: if (m_globalProxy.isEmpty()) m_globalProxy should be empty here, right? DCHECK(m_globalProxy.isEmpty()); https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:102: void RemoteWindowProxy::setupWindowPrototypeChain() { Would you add a comment and explain: - the shape of the prototype chain - why the remote global proxy doesn't need to have an inner global object etc ? https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:108: #if 1 Remove #if 1. https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebRemoteFrame.h (right): https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebRemoteFrame.h:15: class Object; We normally just include v8.h.
https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:60: DCHECK(m_lifecycle == Lifecycle::ContextInitialized); nit: DCHECK_EQ? https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:113: CHECK(m_globalProxy == globalProxy); nit: This CHECK doesn't make much sense just after the assignment. https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:24: {% if method.returns_promise or (interface_name == 'Window' and method.is_cross_origin) %} On 2017/02/14 10:02:02, dcheng wrote: > This is needed because a remote context only has a JSGlobalPoxy and no > JSGlobalObject, so the strict receiver [1] in v8 fails otherwise. I tried to fix > this with https://codereview.chromium.org/2677653002/, but wasn't able to make a > lot of progress. > > [1] > https://chromium.googlesource.com/v8/v8/+/d1055c1fbddb75d628ef6f74a739ea67e28... a) Does HasInstance() return true for RemoteDOMWindow's wrapper, too? b) Don't we need to do the similar things for attributes? Why? https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:550: v8::Local<v8::Signature> signature; Could you write a comment that we don't set the signature intentionally because RemoteDOMWindow blah blah? https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebRemoteFrame.h (right): https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebRemoteFrame.h:15: class Object; Did our policy change? We always include v8.h without using forward declarations.
Note: this still needs some variation of https://codereview.chromium.org/2693203003/ to land; in addition, the WebFrameSwapTest.SetTimeoutAfterSwap is failing with illegal invocation rather than a security error. I think this is fixable... if we use [LenientThis] for all Window methods... https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:24: {% if method.returns_promise or (interface_name == 'Window' and method.is_cross_origin) %} On 2017/02/14 11:24:37, Yuki wrote: > On 2017/02/14 10:02:02, dcheng wrote: > > This is needed because a remote context only has a JSGlobalPoxy and no > > JSGlobalObject, so the strict receiver [1] in v8 fails otherwise. I tried to > fix > > this with https://codereview.chromium.org/2677653002/, but wasn't able to make > a > > lot of progress. > > > > [1] > > > https://chromium.googlesource.com/v8/v8/+/d1055c1fbddb75d628ef6f74a739ea67e28... > > a) Does HasInstance() return true for RemoteDOMWindow's wrapper, too? It seems to work in my local testing. > > b) Don't we need to do the similar things for attributes? Why? Normally, property access is handled by the cross-origin interceptor. However, we still have a problem when trying to execute functions on the Window object. I've added more comments on v8::Local<v8::Signature> to explain this. https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:550: v8::Local<v8::Signature> signature; On 2017/02/14 11:24:37, Yuki wrote: > Could you write a comment that we don't set the signature intentionally because > RemoteDOMWindow blah blah? Done. https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebRemoteFrame.h (right): https://codereview.chromium.org/2626183003/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebRemoteFrame.h:15: class Object; On 2017/02/14 11:24:38, Yuki wrote: > Did our policy change? We always include v8.h without using forward > declarations. Done (I've seen this written in different ways in the public API, but I'm happy to just include) https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:92: if (m_globalProxy.isEmpty()) On 2017/02/14 11:15:01, haraken wrote: > > m_globalProxy should be empty here, right? > > DCHECK(m_globalProxy.isEmpty()); Done. I wrote it this way to parallel how LocalWindowProxy works, as it needs to reinitialize on navigation. However, REmoteWindowProxy doesn't have that, so removing it seems reasonable. https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:102: void RemoteWindowProxy::setupWindowPrototypeChain() { On 2017/02/14 11:15:01, haraken wrote: > > Would you add a comment and explain: > > - the shape of the prototype chain > - why the remote global proxy doesn't need to have an inner global object etc > > ? I tried to explain a bit more. Let me know if it helps. (I didn't draw a diagram because there is no prototype chain at all; I'm open to suggestions on trying to add a diagram though) https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:108: #if 1 On 2017/02/14 11:15:01, haraken wrote: > > Remove #if 1. Done. https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebRemoteFrame.h (right): https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebRemoteFrame.h:15: class Object; On 2017/02/14 11:15:01, haraken wrote: > > We normally just include v8.h. Done.
Looks good overall. I'm looking forward to seeing this CL pass all bots. https://codereview.chromium.org/2626183003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:60: DCHECK(m_lifecycle == Lifecycle::ContextInitialized); nit: DCHECK_EQ is recommended. https://codereview.chromium.org/2626183003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right): https://codereview.chromium.org/2626183003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:7: #include <v8/include/v8.h> nit: <> => ""
Note: this still fails WebFrameSwapTest.SetTimeoutAfterSwap, so I need to investigate more about what's failing =( https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:92: if (m_globalProxy.isEmpty()) On 2017/02/15 11:08:41, dcheng wrote: > On 2017/02/14 11:15:01, haraken wrote: > > > > m_globalProxy should be empty here, right? > > > > DCHECK(m_globalProxy.isEmpty()); > > Done. > > I wrote it this way to parallel how LocalWindowProxy works, as it needs to > reinitialize on navigation. However, REmoteWindowProxy doesn't have that, so > removing it seems reasonable. Actually, we do need this; it might not be empty, if we already a global proxy initialized, and we swapped in a remote frame to replace the local frame. https://codereview.chromium.org/2626183003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:60: DCHECK(m_lifecycle == Lifecycle::ContextInitialized); On 2017/02/15 12:04:30, Yuki wrote: > nit: DCHECK_EQ is recommended. Done (at one point, this didn't use to compile but I forgot jbroman fixed it) https://codereview.chromium.org/2626183003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right): https://codereview.chromium.org/2626183003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:7: #include <v8/include/v8.h> On 2017/02/15 12:04:30, Yuki wrote: > nit: <> => "" Done.
Mostly looks good. https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:92: if (m_globalProxy.isEmpty()) As discussed before, shall we remove this branch and add DCHECK(m_globalProxy.isEmpty())? https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:94: bool m_isGlobalProxyDetached = true; I'd prefer renaming this to m_isGlobalProxyAttached, and set it true only while it's being attached. Or can we remove the flag by using WindowProxy::Lifecycle? https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:556: v8::Local<v8::Signature> signature; Why don't we need to have this code for OriginSafeMethodSetter? https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:640: if method.returns_promise or interface_name == 'Window' else 'V8DOMConfiguration::CheckHolder' %} I don't see any change in this CL to add a type check to attributes/methods of Window. Would you help me understand how the type check is handled? (In essence, I guess you want to add something like [LeinientThis] to Window...)
Btw, for the WebFrameSwapTest.SetTimeoutAfterSwap test, it looks like V8Window::hasInstance() fails for this case. I need to debug more to understand why it doesn't work: internally, it uses FunctionTemplate::HasInstance(), which only sees the global proxy template. What's not clear to me is why it works for everything else except this case. https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:92: if (m_globalProxy.isEmpty()) On 2017/02/16 01:11:19, haraken wrote: > > As discussed before, shall we remove this branch and add > DCHECK(m_globalProxy.isEmpty())? I'm going to add a comment to WindowProxy describing how this generally works. But we do need this if branch; it is possible for it to already be set if we're swapping the frame--in that case, we may take the globalProxy from the old frame (see WindowProxy::releaseGlobal and WindowProxy::setGlobal for how this transfer can happen). https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:94: bool m_isGlobalProxyDetached = true; On 2017/02/16 01:11:19, haraken wrote: > > I'd prefer renaming this to m_isGlobalProxyAttached, and set it true only while > it's being attached. > > Or can we remove the flag by using WindowProxy::Lifecycle? It is possible to combine this into Lifecycle as well, but I had trouble coming up with good enum names (disposed vs detached is not very clear, IMO. Neither is DetachedForClose vs DetachedForNavigation). I'm happy to flip this bool around since it seems easier to read that way. https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:556: v8::Local<v8::Signature> signature; On 2017/02/16 01:11:19, haraken wrote: > > Why don't we need to have this code for OriginSafeMethodSetter? v8::Signature is only used when instantiating a new v8::Function. Since the setter does not instantiate the function, it does not need this. https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:640: if method.returns_promise or interface_name == 'Window' else 'V8DOMConfiguration::CheckHolder' %} On 2017/02/16 01:11:19, haraken wrote: > > I don't see any change in this CL to add a type check to attributes/methods of > Window. Would you help me understand how the type check is handled? > > (In essence, I guess you want to add something like [LeinientThis] to Window...) We only need to relax the receiver check for functions that use the WindowProxy as a receiver object. When just getting an attribute or method, v8 will use the cross-origin interceptors: this works fine, because it doesn't need any receiver check. However, when calling a v8::Function, then we must relax the receiver check. The reason is this: when we instantiate a global proxy, the object template that we use is *not* the object template that we passed in: v8 constructs a fresh object template for the global proxy: https://cs.chromium.org/chromium/src/v8/src/api.cc?rcl=c1a71b70f87cdd76fadea3.... Normally, this is not a problem, because the global object (which *is* constructed with our object template) is also in the prototype chain, so the receiver check works. However, for a remote context, there is no inner global, so we can't rely on walking up the prototype chain. I'm actually not sure how V8Window::hasInstance gets around this, but it seems to work somehow... (I'm going to add this into my comment in WindowProxy)
OK, added a pretty long comment in WindowProxy.h (and consolidated some of the other comments in there)
On 2017/02/16 at 03:14:07, dcheng wrote: > Btw, for the WebFrameSwapTest.SetTimeoutAfterSwap test, it looks like V8Window::hasInstance() fails for this case. I need to debug more to understand why it doesn't work: internally, it uses FunctionTemplate::HasInstance(), which only sees the global proxy template. What's not clear to me is why it works for everything else except this case. > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:92: if (m_globalProxy.isEmpty()) > On 2017/02/16 01:11:19, haraken wrote: > > > > As discussed before, shall we remove this branch and add > > DCHECK(m_globalProxy.isEmpty())? > > I'm going to add a comment to WindowProxy describing how this generally works. But we do need this if branch; it is possible for it to already be set if we're swapping the frame--in that case, we may take the globalProxy from the old frame (see WindowProxy::releaseGlobal and WindowProxy::setGlobal for how this transfer can happen). > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:94: bool m_isGlobalProxyDetached = true; > On 2017/02/16 01:11:19, haraken wrote: > > > > I'd prefer renaming this to m_isGlobalProxyAttached, and set it true only while > > it's being attached. > > > > Or can we remove the flag by using WindowProxy::Lifecycle? > > It is possible to combine this into Lifecycle as well, but I had trouble coming up with good enum names (disposed vs detached is not very clear, IMO. Neither is DetachedForClose vs DetachedForNavigation). > > I'm happy to flip this bool around since it seems easier to read that way. > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:556: v8::Local<v8::Signature> signature; > On 2017/02/16 01:11:19, haraken wrote: > > > > Why don't we need to have this code for OriginSafeMethodSetter? > > v8::Signature is only used when instantiating a new v8::Function. Since the setter does not instantiate the function, it does not need this. > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:640: if method.returns_promise or interface_name == 'Window' else 'V8DOMConfiguration::CheckHolder' %} > On 2017/02/16 01:11:19, haraken wrote: > > > > I don't see any change in this CL to add a type check to attributes/methods of > > Window. Would you help me understand how the type check is handled? > > > > (In essence, I guess you want to add something like [LeinientThis] to Window...) > > We only need to relax the receiver check for functions that use the WindowProxy as a receiver object. > > When just getting an attribute or method, v8 will use the cross-origin interceptors: this works fine, because it doesn't need any receiver check. However, when calling a v8::Function, then we must relax the receiver check. > > The reason is this: when we instantiate a global proxy, the object template that we use is *not* the object template that we passed in: v8 constructs a fresh object template for the global proxy: https://cs.chromium.org/chromium/src/v8/src/api.cc?rcl=c1a71b70f87cdd76fadea3.... Normally, this is not a problem, because the global object (which *is* constructed with our object template) is also in the prototype chain, so the receiver check works. > > However, for a remote context, there is no inner global, so we can't rely on walking up the prototype chain. I'm actually not sure how V8Window::hasInstance gets around this, but it seems to work somehow... IIUC, V8DOMWrapper::setNativeInfo (in setupWindowPrototypeChain) does this. It sets the internal fields on the global proxy, which are what we use for V8Window::hasInstance etc. (we check that the WrapperTypeInfo is one we expect window objects to have). If this is true, then hasInstance returns true (and so V8Window::toImpl would be able to safely extract the pointer-to-ScriptWrappable field and cast it to DOMWindow, if we so desired). That's why I don't think it's necessary to inspect the prototype chain here. > (I'm going to add this into my comment in WindowProxy)
On 2017/02/16 at 04:49:53, jbroman wrote: > On 2017/02/16 at 03:14:07, dcheng wrote: > > Btw, for the WebFrameSwapTest.SetTimeoutAfterSwap test, it looks like V8Window::hasInstance() fails for this case. I need to debug more to understand why it doesn't work: internally, it uses FunctionTemplate::HasInstance(), which only sees the global proxy template. What's not clear to me is why it works for everything else except this case. > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:92: if (m_globalProxy.isEmpty()) > > On 2017/02/16 01:11:19, haraken wrote: > > > > > > As discussed before, shall we remove this branch and add > > > DCHECK(m_globalProxy.isEmpty())? > > > > I'm going to add a comment to WindowProxy describing how this generally works. But we do need this if branch; it is possible for it to already be set if we're swapping the frame--in that case, we may take the globalProxy from the old frame (see WindowProxy::releaseGlobal and WindowProxy::setGlobal for how this transfer can happen). > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:94: bool m_isGlobalProxyDetached = true; > > On 2017/02/16 01:11:19, haraken wrote: > > > > > > I'd prefer renaming this to m_isGlobalProxyAttached, and set it true only while > > > it's being attached. > > > > > > Or can we remove the flag by using WindowProxy::Lifecycle? > > > > It is possible to combine this into Lifecycle as well, but I had trouble coming up with good enum names (disposed vs detached is not very clear, IMO. Neither is DetachedForClose vs DetachedForNavigation). > > > > I'm happy to flip this bool around since it seems easier to read that way. > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:556: v8::Local<v8::Signature> signature; > > On 2017/02/16 01:11:19, haraken wrote: > > > > > > Why don't we need to have this code for OriginSafeMethodSetter? > > > > v8::Signature is only used when instantiating a new v8::Function. Since the setter does not instantiate the function, it does not need this. > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:640: if method.returns_promise or interface_name == 'Window' else 'V8DOMConfiguration::CheckHolder' %} > > On 2017/02/16 01:11:19, haraken wrote: > > > > > > I don't see any change in this CL to add a type check to attributes/methods of > > > Window. Would you help me understand how the type check is handled? > > > > > > (In essence, I guess you want to add something like [LeinientThis] to Window...) > > > > We only need to relax the receiver check for functions that use the WindowProxy as a receiver object. > > > > When just getting an attribute or method, v8 will use the cross-origin interceptors: this works fine, because it doesn't need any receiver check. However, when calling a v8::Function, then we must relax the receiver check. > > > > The reason is this: when we instantiate a global proxy, the object template that we use is *not* the object template that we passed in: v8 constructs a fresh object template for the global proxy: https://cs.chromium.org/chromium/src/v8/src/api.cc?rcl=c1a71b70f87cdd76fadea3.... Normally, this is not a problem, because the global object (which *is* constructed with our object template) is also in the prototype chain, so the receiver check works. > > > > However, for a remote context, there is no inner global, so we can't rely on walking up the prototype chain. I'm actually not sure how V8Window::hasInstance gets around this, but it seems to work somehow... > > IIUC, V8DOMWrapper::setNativeInfo (in setupWindowPrototypeChain) does this. It sets the internal fields on the global proxy, which are what we use for V8Window::hasInstance etc. (we check that the WrapperTypeInfo is one we expect window objects to have). If this is true, then hasInstance returns true (and so V8Window::toImpl would be able to safely extract the pointer-to-ScriptWrappable field and cast it to DOMWindow, if we so desired). > > That's why I don't think it's necessary to inspect the prototype chain here. Urgh, I shouldn't comment on reviews late at night. At the risk of misunderstanding again, you probably meant the logic in v8::FunctionTemplate::HasInstance. IIUC, _that_ works because NewRemoteContext reinitializes the global proxy with a map as though it had been constructed with the V8Window constructor, so the FunctionTemplate::HasInstance check works without traversing the prototype chain because the object is marked as having been constructed with from the Window DOM template. > > (I'm going to add this into my comment in WindowProxy)
On 2017/02/16 05:14:46, jbroman wrote: > On 2017/02/16 at 04:49:53, jbroman wrote: > > On 2017/02/16 at 03:14:07, dcheng wrote: > > > Btw, for the WebFrameSwapTest.SetTimeoutAfterSwap test, it looks like > V8Window::hasInstance() fails for this case. I need to debug more to understand > why it doesn't work: internally, it uses FunctionTemplate::HasInstance(), which > only sees the global proxy template. What's not clear to me is why it works for > everything else except this case. > > > > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp > (right): > > > > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:92: if > (m_globalProxy.isEmpty()) > > > On 2017/02/16 01:11:19, haraken wrote: > > > > > > > > As discussed before, shall we remove this branch and add > > > > DCHECK(m_globalProxy.isEmpty())? > > > > > > I'm going to add a comment to WindowProxy describing how this generally > works. But we do need this if branch; it is possible for it to already be set if > we're swapping the frame--in that case, we may take the globalProxy from the old > frame (see WindowProxy::releaseGlobal and WindowProxy::setGlobal for how this > transfer can happen). > > > > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): > > > > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:94: bool > m_isGlobalProxyDetached = true; > > > On 2017/02/16 01:11:19, haraken wrote: > > > > > > > > I'd prefer renaming this to m_isGlobalProxyAttached, and set it true only > while > > > > it's being attached. > > > > > > > > Or can we remove the flag by using WindowProxy::Lifecycle? > > > > > > It is possible to combine this into Lifecycle as well, but I had trouble > coming up with good enum names (disposed vs detached is not very clear, IMO. > Neither is DetachedForClose vs DetachedForNavigation). > > > > > > I'm happy to flip this bool around since it seems easier to read that way. > > > > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): > > > > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:556: > v8::Local<v8::Signature> signature; > > > On 2017/02/16 01:11:19, haraken wrote: > > > > > > > > Why don't we need to have this code for OriginSafeMethodSetter? > > > > > > v8::Signature is only used when instantiating a new v8::Function. Since the > setter does not instantiate the function, it does not need this. > > > > > > > https://codereview.chromium.org/2626183003/diff/180001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:640: if > method.returns_promise or interface_name == 'Window' else > 'V8DOMConfiguration::CheckHolder' %} > > > On 2017/02/16 01:11:19, haraken wrote: > > > > > > > > I don't see any change in this CL to add a type check to > attributes/methods of > > > > Window. Would you help me understand how the type check is handled? > > > > > > > > (In essence, I guess you want to add something like [LeinientThis] to > Window...) > > > > > > We only need to relax the receiver check for functions that use the > WindowProxy as a receiver object. > > > > > > When just getting an attribute or method, v8 will use the cross-origin > interceptors: this works fine, because it doesn't need any receiver check. > However, when calling a v8::Function, then we must relax the receiver check. > > > > > > The reason is this: when we instantiate a global proxy, the object template > that we use is *not* the object template that we passed in: v8 constructs a > fresh object template for the global proxy: > https://cs.chromium.org/chromium/src/v8/src/api.cc?rcl=c1a71b70f87cdd76fadea3.... > Normally, this is not a problem, because the global object (which *is* > constructed with our object template) is also in the prototype chain, so the > receiver check works. > > > > > > However, for a remote context, there is no inner global, so we can't rely on > walking up the prototype chain. I'm actually not sure how V8Window::hasInstance > gets around this, but it seems to work somehow... > > > > IIUC, V8DOMWrapper::setNativeInfo (in setupWindowPrototypeChain) does this. It > sets the internal fields on the global proxy, which are what we use for > V8Window::hasInstance etc. (we check that the WrapperTypeInfo is one we expect > window objects to have). If this is true, then hasInstance returns true (and so > V8Window::toImpl would be able to safely extract the pointer-to-ScriptWrappable > field and cast it to DOMWindow, if we so desired). Right, I understand that we use the internal fields to map from the Object back to the C++ implementation. But before that, we have to do the hasInstance() check, which is based on checking for a matching FunctionTemplateInfo. > > > > That's why I don't think it's necessary to inspect the prototype chain here. > > Urgh, I shouldn't comment on reviews late at night. At the risk of > misunderstanding again, you probably meant the logic in > v8::FunctionTemplate::HasInstance. IIUC, _that_ works because NewRemoteContext > reinitializes the global proxy with a map as though it had been constructed with > the V8Window constructor, so the FunctionTemplate::HasInstance check works > without traversing the prototype chain because the object is marked as having > been constructed with from the Window DOM template. The global proxy isn't constructed from the Window template though. The entry point for creating a new remote context looks like this: v8::Local<v8::Object> NewRemoteContext(v8::Local<v8::ObjectTemplate> global_template); Which we invoke like this: NewRemoteContext(GetTemplateForWindowInterface()); But v8 actually does this: v8::Local<v8::ObjectTemplate> proxy_template = ObjectTemplate::New(isolate); proxy_constructor = EnsureConstructor(isolate, *proxy_template); proxy_constructor->set_prototype_template(*global_template); And eventually we create the JSGlobalProxy using |proxy_template|. AFAICT, HasInstance() is based on a strict equality check, which should fail when we compare against GetTemplateForWindowInterface()... This is what happens for the failing case, but I haven't researched why it succeeds yet. > > > > (I'm going to add this into my comment in WindowProxy)
On 2017/02/16 05:49:01, dcheng wrote: > AFAICT, HasInstance() is based on a strict equality check, which should fail > when we compare against GetTemplateForWindowInterface()... Just in case, I'd like you guys to be aware of a hack at https://crrev.com/2500363002 . In the case of JSGlobalProxy, it checks the global object, too. I don't understand if this is related or totally irrelevant.
On 2017/02/16 06:19:05, Yuki wrote: > On 2017/02/16 05:49:01, dcheng wrote: > > AFAICT, HasInstance() is based on a strict equality check, which should fail > > when we compare against GetTemplateForWindowInterface()... > > Just in case, I'd like you guys to be aware of a hack at > https://crrev.com/2500363002 . > In the case of JSGlobalProxy, it checks the global object, too. I don't > understand if this is related or totally irrelevant. Yeah, I'm actually working on a hack in that area right now =)
https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:16: ((method.returns_promise or interface_name == 'Window' or interface_name == 'EventTarget') and not method.is_static) %} I had to add EventTarget to this list as well... To make this less hardcoded in the bindings, I propose: - adding an interface-level attribute CheckReceiver. - add MethodCall as a mandatory argument. The reason is this is really only needed for method invocations. The interface definition will look like [CheckReceiver=MethodCall] interface Window { ... }; - update the bindings generator to throw an error if a CheckReceiver=MethodCall interface inherits a non CheckReceiver=MethodCall interface. Does this sound reasonable?
https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:46: // model consists of the inner global object and the outer global proxy. I'd like to have a note about which term in Blink corresponds to which term in HTML spec, i.e. "inner global object" corresponds to Window and "outer global proxy" corresponds to WindowProxy in HTML spec. I also think we'd like to note that WindowProxy class in Blink doesn't (directly) correspond to WindowProxy in HTML spec. https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:52: // Window interface are exposed via the inner global object. Variables defined Variables defined by ??? https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:66: // security checks for same-origin/cross-origin access to the Window interface. Is this correct? AccessCheckCallbackAndHandler (i.e. V8Window::securityCheck) is set on the inner global object in case of local. My understanding is that everything including security checks is done on the inner global object if it's local. The outer global proxy's role is to detach the global object across navigations. https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:86: // LocalWindowProxy could be same-origin or cross-origin. I think that it's worth mentioning that we break the definition of the same origin in the spec. Even if two pages are the same origin in the spec, if it's served in a remote frame, then we treat as they're cross origin. I think it's worth writing comments whenever we intentionally do different things than the spec. https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:107: // -- null Is this really null? I thought it's an empty handle. v8::Local<v8::Object>() is different from v8::Null(isolate). https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:136: // which doesn't need to perform receiver checks. Currently cross origin accessible attributes are implemented as data properties (not accessor properties), so this works. However, once we implement them as accessor properties, we'll need the same things as methods because author script can extract get and set function objects. https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:16: ((method.returns_promise or interface_name == 'Window' or interface_name == 'EventTarget') and not method.is_static) %} On 2017/02/16 10:48:36, dcheng wrote: > I had to add EventTarget to this list as well... > > To make this less hardcoded in the bindings, I propose: > > - adding an interface-level attribute CheckReceiver. > - add MethodCall as a mandatory argument. The reason is this is really only > needed for method invocations. The interface definition will look like > > [CheckReceiver=MethodCall] > interface Window { ... }; > > - update the bindings generator to throw an error if a CheckReceiver=MethodCall > interface inherits a non CheckReceiver=MethodCall interface. > > Does this sound reasonable? Could you help me understand the situation? You're saying that a) We cannot rely on v8::Signature because it doesn't work for remote global proxy. b) v8::FunctionTemplate::HasInstance (or its family) works for remote global proxy, too. (You're now making it work for remote cases.) Are these right? I don't know internal details of V8, but is it difficult to make v8::Signature work for remote global proxy? The remote global proxy is already so special. I'm wondering if we could make the remote global proxy look like a local global object rather than a local global proxy.
https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:46: // model consists of the inner global object and the outer global proxy. On 2017/02/16 12:47:39, Yuki wrote: > I'd like to have a note about which term in Blink corresponds to which term in > HTML spec, i.e. "inner global object" corresponds to Window and "outer global > proxy" corresponds to WindowProxy in HTML spec. > > I also think we'd like to note that WindowProxy class in Blink doesn't > (directly) correspond to WindowProxy in HTML spec. Done. I reworded a lot of this comment; hopefully, it's a bit more understandable now. https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:52: // Window interface are exposed via the inner global object. Variables defined On 2017/02/16 12:47:39, Yuki wrote: > Variables defined by ??? Done =) https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:66: // security checks for same-origin/cross-origin access to the Window interface. On 2017/02/16 12:47:39, Yuki wrote: > Is this correct? > > AccessCheckCallbackAndHandler (i.e. V8Window::securityCheck) is set on the inner > global object in case of local. > > My understanding is that everything including security checks is done on the > inner global object if it's local. The outer global proxy's role is to detach > the global object across navigations. This is actually something I'm not sure about. From a conceptual point of view, I think we should think of the security checks as belonging on the outer global proxy itself. However, from an implementation perspective, I think we currently have access checks on both the proxy template and the global template. See https://cs.chromium.org/chromium/src/v8/src/api.cc?rcl=547f4de8ab3155d315bba8... +jochen I wonder if we can remove this, now that we're using cross-origin interceptors. https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:86: // LocalWindowProxy could be same-origin or cross-origin. On 2017/02/16 12:47:39, Yuki wrote: > I think that it's worth mentioning that we break the definition of the same > origin in the spec. Even if two pages are the same origin in the spec, if it's > served in a remote frame, then we treat as they're cross origin. > > I think it's worth writing comments whenever we intentionally do different > things than the spec. Done. https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:107: // -- null On 2017/02/16 12:47:39, Yuki wrote: > Is this really null? I thought it's an empty handle. v8::Local<v8::Object>() > is different from v8::Null(isolate). From my printf debugging, I believe this is correct. Also see https://cs.chromium.org/chromium/src/v8/src/bootstrapper.cc?rcl=547f4de8ab315... https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:136: // which doesn't need to perform receiver checks. On 2017/02/16 12:47:39, Yuki wrote: > Currently cross origin accessible attributes are implemented as data properties > (not accessor properties), so this works. However, once we implement them as > accessor properties, we'll need the same things as methods because author script > can extract get and set function objects. This comment is attempting to document things as they are (well, mostly. I cheated a little on the access checks comments when talking about the outer global proxy). However, for this comment, I think we should only update this to mention data properties when we actually change how it's implemented. https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:16: ((method.returns_promise or interface_name == 'Window' or interface_name == 'EventTarget') and not method.is_static) %} On 2017/02/16 12:47:39, Yuki wrote: > On 2017/02/16 10:48:36, dcheng wrote: > > I had to add EventTarget to this list as well... > > > > To make this less hardcoded in the bindings, I propose: > > > > - adding an interface-level attribute CheckReceiver. > > - add MethodCall as a mandatory argument. The reason is this is really only > > needed for method invocations. The interface definition will look like > > > > [CheckReceiver=MethodCall] > > interface Window { ... }; > > > > - update the bindings generator to throw an error if a > CheckReceiver=MethodCall > > interface inherits a non CheckReceiver=MethodCall interface. > > > > Does this sound reasonable? > > Could you help me understand the situation? You're saying that > > a) We cannot rely on v8::Signature because it doesn't work for remote global > proxy. Correct. Due to the way we create the proxy template for a JSGlobalProxy, Blink's DOM template for Window will never match. > > b) v8::FunctionTemplate::HasInstance (or its family) works for remote global > proxy, too. (You're now making it work for remote cases.) > > Are these right? > > I don't know internal details of V8, but is it difficult to make v8::Signature > work for remote global proxy? The remote global proxy is already so special. > I'm wondering if we could make the remote global proxy look like a local global > object rather than a local global proxy. Yes, this is proving to be quite hacky. That being said, I'd like to land some version of this sooner rather than later; let me see if I can hack the compatible receiver check so we can handle this internally in v8.
Overall, I think now I have (somewhat) good understanding, and I'm okay with this CL. LGTM on my side. (Having said that, I highly prefer signature support in V8 for remote frames. Hope dchecng@ implement it.) https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:107: // -- null On 2017/02/16 19:50:23, dcheng wrote: > On 2017/02/16 12:47:39, Yuki wrote: > > Is this really null? I thought it's an empty handle. v8::Local<v8::Object>() > > is different from v8::Null(isolate). > > From my printf debugging, I believe this is correct. Also see > https://cs.chromium.org/chromium/src/v8/src/bootstrapper.cc?rcl=547f4de8ab315... Thanks for checking. nit: you might want to make it -- has prototype --> null to follow the above style (it's a hidden prototype, though). https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:16: ((method.returns_promise or interface_name == 'Window' or interface_name == 'EventTarget') and not method.is_static) %} On 2017/02/16 19:50:23, dcheng wrote: > On 2017/02/16 12:47:39, Yuki wrote: > > On 2017/02/16 10:48:36, dcheng wrote: > > > I had to add EventTarget to this list as well... > > > > > > To make this less hardcoded in the bindings, I propose: > > > > > > - adding an interface-level attribute CheckReceiver. > > > - add MethodCall as a mandatory argument. The reason is this is really only > > > needed for method invocations. The interface definition will look like > > > > > > [CheckReceiver=MethodCall] > > > interface Window { ... }; > > > > > > - update the bindings generator to throw an error if a > > CheckReceiver=MethodCall > > > interface inherits a non CheckReceiver=MethodCall interface. > > > > > > Does this sound reasonable? > > > > Could you help me understand the situation? You're saying that > > > > a) We cannot rely on v8::Signature because it doesn't work for remote global > > proxy. > > Correct. Due to the way we create the proxy template for a JSGlobalProxy, > Blink's DOM template for Window will never match. > > > > > b) v8::FunctionTemplate::HasInstance (or its family) works for remote global > > proxy, too. (You're now making it work for remote cases.) > > > > Are these right? > > > > I don't know internal details of V8, but is it difficult to make v8::Signature > > work for remote global proxy? The remote global proxy is already so special. > > I'm wondering if we could make the remote global proxy look like a local > global > > object rather than a local global proxy. > > Yes, this is proving to be quite hacky. That being said, I'd like to land some > version of this sooner rather than later; let me see if I can hack the > compatible receiver check so we can handle this internally in v8. Thanks for the explanation. Overall, I'm okay with this. If you think we can get rid of this hack by moving it into V8, I don't think we need to introduce [CheckReceiver=MethodCall]. Instead, I'd like to have comments to describe the situation and plan. I'm also fine with introducing an extended attribute, but I'd prefer another name that indicates/implies "hack to handle remote frames". At least, [CheckReceiver] is a bit confusing with [CheckSecurity=Receiver]. This work can be a separate CL.
On 2017/02/17 06:49:31, Yuki wrote: > Overall, I think now I have (somewhat) good understanding, and I'm okay with > this CL. LGTM on my side. > > (Having said that, I highly prefer signature support in V8 for remote frames. > Hope dchecng@ implement it.) > > https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): > > https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:107: // -- null > On 2017/02/16 19:50:23, dcheng wrote: > > On 2017/02/16 12:47:39, Yuki wrote: > > > Is this really null? I thought it's an empty handle. > v8::Local<v8::Object>() > > > is different from v8::Null(isolate). > > > > From my printf debugging, I believe this is correct. Also see > > > https://cs.chromium.org/chromium/src/v8/src/bootstrapper.cc?rcl=547f4de8ab315... > > Thanks for checking. > > nit: you might want to make it > -- has prototype --> null > to follow the above style (it's a hidden prototype, though). > > https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): > > https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:16: > ((method.returns_promise or interface_name == 'Window' or interface_name == > 'EventTarget') and not method.is_static) %} > On 2017/02/16 19:50:23, dcheng wrote: > > On 2017/02/16 12:47:39, Yuki wrote: > > > On 2017/02/16 10:48:36, dcheng wrote: > > > > I had to add EventTarget to this list as well... > > > > > > > > To make this less hardcoded in the bindings, I propose: > > > > > > > > - adding an interface-level attribute CheckReceiver. > > > > - add MethodCall as a mandatory argument. The reason is this is really > only > > > > needed for method invocations. The interface definition will look like > > > > > > > > [CheckReceiver=MethodCall] > > > > interface Window { ... }; > > > > > > > > - update the bindings generator to throw an error if a > > > CheckReceiver=MethodCall > > > > interface inherits a non CheckReceiver=MethodCall interface. > > > > > > > > Does this sound reasonable? > > > > > > Could you help me understand the situation? You're saying that > > > > > > a) We cannot rely on v8::Signature because it doesn't work for remote global > > > proxy. > > > > Correct. Due to the way we create the proxy template for a JSGlobalProxy, > > Blink's DOM template for Window will never match. > > > > > > > > b) v8::FunctionTemplate::HasInstance (or its family) works for remote global > > > proxy, too. (You're now making it work for remote cases.) > > > > > > Are these right? > > > > > > I don't know internal details of V8, but is it difficult to make > v8::Signature > > > work for remote global proxy? The remote global proxy is already so > special. > > > I'm wondering if we could make the remote global proxy look like a local > > global > > > object rather than a local global proxy. > > > > Yes, this is proving to be quite hacky. That being said, I'd like to land some > > version of this sooner rather than later; let me see if I can hack the > > compatible receiver check so we can handle this internally in v8. > > Thanks for the explanation. Overall, I'm okay with this. > > If you think we can get rid of this hack by moving it into V8, I don't think we > need to introduce [CheckReceiver=MethodCall]. Instead, I'd like to have > comments to describe the situation and plan. > > I'm also fine with introducing an extended attribute, but I'd prefer another > name that indicates/implies "hack to handle remote frames". At least, > [CheckReceiver] is a bit confusing with [CheckSecurity=Receiver]. This work can > be a separate CL. Yeah, how hard will it be to implement the type check in the V8 side? Doing the type check on the binding side is really error-prone (we've so far caused a couple of security issues because of the missing checks), so I'd prefer avoiding it as much as possible.
On 2017/02/17 06:59:12, haraken wrote: > Yeah, how hard will it be to implement the type check in the V8 side? Doing the > type check on the binding side is really error-prone (we've so far caused a > couple of security issues because of the missing checks), so I'd prefer avoiding > it as much as possible. The instance checks were really bothering me: I prepared a patch to make it so the CheckCompatibleReceiver() check in Blink understood the special conventions of JSGlobalProxy. However, I felt that this was making v8 too complicated. After some debugging, I managed to get v8::Signature working with v8 remote contexts, so I think everyone should be happy (once I can clean up the hack in the v8 patch =) https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:107: // -- null On 2017/02/17 06:49:31, Yuki wrote: > On 2017/02/16 19:50:23, dcheng wrote: > > On 2017/02/16 12:47:39, Yuki wrote: > > > Is this really null? I thought it's an empty handle. > v8::Local<v8::Object>() > > > is different from v8::Null(isolate). > > > > From my printf debugging, I believe this is correct. Also see > > > https://cs.chromium.org/chromium/src/v8/src/bootstrapper.cc?rcl=547f4de8ab315... > > Thanks for checking. > > nit: you might want to make it > -- has prototype --> null > to follow the above style (it's a hidden prototype, though). Done. https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:16: ((method.returns_promise or interface_name == 'Window' or interface_name == 'EventTarget') and not method.is_static) %} On 2017/02/17 06:49:31, Yuki wrote: > On 2017/02/16 19:50:23, dcheng wrote: > > On 2017/02/16 12:47:39, Yuki wrote: > > > On 2017/02/16 10:48:36, dcheng wrote: > > > > I had to add EventTarget to this list as well... > > > > > > > > To make this less hardcoded in the bindings, I propose: > > > > > > > > - adding an interface-level attribute CheckReceiver. > > > > - add MethodCall as a mandatory argument. The reason is this is really > only > > > > needed for method invocations. The interface definition will look like > > > > > > > > [CheckReceiver=MethodCall] > > > > interface Window { ... }; > > > > > > > > - update the bindings generator to throw an error if a > > > CheckReceiver=MethodCall > > > > interface inherits a non CheckReceiver=MethodCall interface. > > > > > > > > Does this sound reasonable? > > > > > > Could you help me understand the situation? You're saying that > > > > > > a) We cannot rely on v8::Signature because it doesn't work for remote global > > > proxy. > > > > Correct. Due to the way we create the proxy template for a JSGlobalProxy, > > Blink's DOM template for Window will never match. > > > > > > > > b) v8::FunctionTemplate::HasInstance (or its family) works for remote global > > > proxy, too. (You're now making it work for remote cases.) > > > > > > Are these right? > > > > > > I don't know internal details of V8, but is it difficult to make > v8::Signature > > > work for remote global proxy? The remote global proxy is already so > special. > > > I'm wondering if we could make the remote global proxy look like a local > > global > > > object rather than a local global proxy. > > > > Yes, this is proving to be quite hacky. That being said, I'd like to land some > > version of this sooner rather than later; let me see if I can hack the > > compatible receiver check so we can handle this internally in v8. > > Thanks for the explanation. Overall, I'm okay with this. > > If you think we can get rid of this hack by moving it into V8, I don't think we > need to introduce [CheckReceiver=MethodCall]. Instead, I'd like to have > comments to describe the situation and plan. > > I'm also fine with introducing an extended attribute, but I'd prefer another > name that indicates/implies "hack to handle remote frames". At least, > [CheckReceiver] is a bit confusing with [CheckSecurity=Receiver]. This work can > be a separate CL. I was getting pretty unhappy with how the code looked in v8, since there were too many special cases for JSGlobalProxy. The good news is I got it working, and cleaned up many of the manual checks in Blink. The bad news is the v8 patch has a small hack for hidden prototypes that I don't understand: https://codereview.chromium.org/2677653002/diff/40001/src/bootstrapper.cc I'm working on figuring out what's going wrong, so I can remove the hack.
https://codereview.chromium.org/2626183003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (left): https://codereview.chromium.org/2626183003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:514: {% if world_suffix in method.activity_logging_world_list %} Also, I had to move the activity logging helper: for a remote context, there is no ScriptState, so this will crash. I thought about trying to keep it so we preserve old behavior for everything except Window, but for binding health, it's probably to do the simplest thing first? WDYT?
https://codereview.chromium.org/2626183003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (left): https://codereview.chromium.org/2626183003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:514: {% if world_suffix in method.activity_logging_world_list %} On 2017/02/17 07:30:42, dcheng wrote: > Also, I had to move the activity logging helper: for a remote context, there is > no ScriptState, so this will crash. > > I thought about trying to keep it so we preserve old behavior for everything > except Window, but for binding health, it's probably to do the simplest thing > first? WDYT? AFAIK, no cross origin accessible property uses a V8DOMActivityLogger. Do you really hit a crash? How do you come at this line with a RemoteDOMWindow?
https://codereview.chromium.org/2626183003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (left): https://codereview.chromium.org/2626183003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:514: {% if world_suffix in method.activity_logging_world_list %} On 2017/02/17 10:53:17, Yuki wrote: > On 2017/02/17 07:30:42, dcheng wrote: > > Also, I had to move the activity logging helper: for a remote context, there > is > > no ScriptState, so this will crash. > > > > I thought about trying to keep it so we preserve old behavior for everything > > except Window, but for binding health, it's probably to do the simplest thing > > first? WDYT? > > AFAIK, no cross origin accessible property uses a V8DOMActivityLogger. Do you > really hit a crash? How do you come at this line with a RemoteDOMWindow? It happens when you do something this: window.openDatabase.call(crossOriginWindow, ...); In this case, there is no ScriptState for the receiver object; however, we don't do the security check until later, which causes the crash. See http/tests/security/cross-origin-access-call.html for more.
https://codereview.chromium.org/2626183003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (left): https://codereview.chromium.org/2626183003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:514: {% if world_suffix in method.activity_logging_world_list %} On 2017/02/17 10:57:00, dcheng wrote: > On 2017/02/17 10:53:17, Yuki wrote: > > On 2017/02/17 07:30:42, dcheng wrote: > > > Also, I had to move the activity logging helper: for a remote context, there > > is > > > no ScriptState, so this will crash. > > > > > > I thought about trying to keep it so we preserve old behavior for everything > > > except Window, but for binding health, it's probably to do the simplest > thing > > > first? WDYT? > > > > AFAIK, no cross origin accessible property uses a V8DOMActivityLogger. Do you > > really hit a crash? How do you come at this line with a RemoteDOMWindow? > > It happens when you do something this: > > window.openDatabase.call(crossOriginWindow, ...); > > In this case, there is no ScriptState for the receiver object; however, we don't > do the security check until later, which causes the crash. > > See http/tests/security/cross-origin-access-call.html for more. I see. Then, I'm okay with moving this, although this makes another asymmetry between attributes and methods. We can fix it separately.
Description was changed from ========== Switch RemoteWindowProxy to use v8::Context::NewRemoteContext. BUG=527190 ========== to ========== Switch RemoteWindowProxy to use v8::Context::NewRemoteContext. BUG=527190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Any further thoughts? If not, I'm planning on landing this once https://codereview.chromium.org/2677653002/ gets a final signoff (since I made one additional tweak to it) and is rolled into Chrome. Thanks!
LGTM.
dcheng@chromium.org changed reviewers: + lfg@chromium.org
+lfg for OWNERS review of guest view parts
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...
lgtm
The CQ bit was unchecked by dcheng@chromium.org
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/02/22 19:10:47, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Guess I need one more v8 patch, sigh =)
https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:92: if (m_globalProxy.isEmpty()) When you're re-initializing the global proxy, you create a new remote global proxy but don't update m_globalProxy. Is it an intended behavior? https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:136: // match. Would you help me understand why we need the global object for a remote window? I was thinking that a global proxy would be enough. Also what is the memory impact of instantiating the global object for a remote window? I think the original motivation of introducing the remote context is to reduce the memory usage. https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% if world_suffix in method.activity_logging_world_list %} What is this change for?
https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right): https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:92: if (m_globalProxy.isEmpty()) On 2017/02/23 04:30:20, haraken wrote: > > When you're re-initializing the global proxy, you create a new remote global > proxy but don't update m_globalProxy. Is it an intended behavior? In that case, it should always be true that m_globalProxy == globalProxy. I've added a DCHECK for that. https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:136: // match. On 2017/02/23 04:30:20, haraken wrote: > > Would you help me understand why we need the global object for a remote window? > I > was thinking that a global proxy would be enough. > > Also what is the memory impact of instantiating the global object for a remote > window? I think the original motivation of introducing the remote context is to > reduce the memory usage. So the problem is that the global proxy isn't instantiated with |globalTemplate|. This is a problem for method calls, HasInstance and FindInstanceInPrototypeChain, because these all require strict matches. Originally, I added special cases for JSGlobalProxy for these in v8, but this seemed pretty hacky. I reverted the hacks and just create a inner global object for remote contexts too now. It's not a problem in terms of memory impact, because the object is very small: we don't initialize any of the JS builtins, etc, so it's the same as any other remote object instance (e.g. a remote Location object). https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% if world_suffix in method.activity_logging_world_list %} On 2017/02/23 04:30:20, haraken wrote: > > What is this change for? This needs to be done after the security check; otherwise, we might call this on a remote context, and a remote context doesn't have an associated ScriptState.
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: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
> So the problem is that the global proxy isn't instantiated with > |globalTemplate|. This is a problem for method calls, HasInstance and > FindInstanceInPrototypeChain, because these all require strict matches. > > Originally, I added special cases for JSGlobalProxy for these in v8, but this > seemed pretty hacky. I reverted the hacks and just create a inner global object > for remote contexts too now. Makes sense. (I was thinking that the global proxy should be instantiated with the global template because we're passing the global template when calling NewRemoteContext().) https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% if world_suffix in method.activity_logging_world_list %} On 2017/02/23 04:55:41, dcheng wrote: > On 2017/02/23 04:30:20, haraken wrote: > > > > What is this change for? > > This needs to be done after the security check; otherwise, we might call this on > a remote context, and a remote context doesn't have an associated ScriptState. Hmm. Then you will need to make a similar change to attributes.cpp.tmpl? I guess there're many places where we're using ScriptState before doing the security check... https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:99: ScriptState* scriptState = ScriptState::forReceiverObject(info); Nit: You're potentially redefining the ScriptState :) The compile is just passing because it doesn't happen in practice by accident...
https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% if world_suffix in method.activity_logging_world_list %} On 2017/02/23 09:26:24, haraken wrote: > On 2017/02/23 04:55:41, dcheng wrote: > > On 2017/02/23 04:30:20, haraken wrote: > > > > > > What is this change for? > > > > This needs to be done after the security check; otherwise, we might call this > on > > a remote context, and a remote context doesn't have an associated ScriptState. > > Hmm. > > Then you will need to make a similar change to attributes.cpp.tmpl? I guess > there're many places where we're using ScriptState before doing the security > check... > As far as I know, it's only a problem for methods due to the fact that you can write code like: window.setTimeout(crossOriginWindow, ...) https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:99: ScriptState* scriptState = ScriptState::forReceiverObject(info); On 2017/02/23 09:26:24, haraken wrote: > > Nit: You're potentially redefining the ScriptState :) The compile is just > passing because it doesn't happen in practice by accident... Done.
On 2017/02/23 09:44:43, dcheng wrote: > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% if > world_suffix in method.activity_logging_world_list %} > On 2017/02/23 09:26:24, haraken wrote: > > On 2017/02/23 04:55:41, dcheng wrote: > > > On 2017/02/23 04:30:20, haraken wrote: > > > > > > > > What is this change for? > > > > > > This needs to be done after the security check; otherwise, we might call > this > > on > > > a remote context, and a remote context doesn't have an associated > ScriptState. > > > > Hmm. > > > > Then you will need to make a similar change to attributes.cpp.tmpl? I guess > > there're many places where we're using ScriptState before doing the security > > check... > > > > As far as I know, it's only a problem for methods due to the fact that you can > write code like: > > window.setTimeout(crossOriginWindow, ...) Wouldn't it be possible to do something like Object.getOwnPropertyDescriptor(window, "attribute").get.call(crossOriginWindow)? > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:99: ScriptState* > scriptState = ScriptState::forReceiverObject(info); > On 2017/02/23 09:26:24, haraken wrote: > > > > Nit: You're potentially redefining the ScriptState :) The compile is just > > passing because it doesn't happen in practice by accident... > > Done.
On 2017/02/24 05:07:48, haraken wrote: > On 2017/02/23 09:44:43, dcheng wrote: > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% if > > world_suffix in method.activity_logging_world_list %} > > On 2017/02/23 09:26:24, haraken wrote: > > > On 2017/02/23 04:55:41, dcheng wrote: > > > > On 2017/02/23 04:30:20, haraken wrote: > > > > > > > > > > What is this change for? > > > > > > > > This needs to be done after the security check; otherwise, we might call > > this > > > on > > > > a remote context, and a remote context doesn't have an associated > > ScriptState. > > > > > > Hmm. > > > > > > Then you will need to make a similar change to attributes.cpp.tmpl? I guess > > > there're many places where we're using ScriptState before doing the security > > > check... > > > > > > > As far as I know, it's only a problem for methods due to the fact that you can > > write code like: > > > > window.setTimeout(crossOriginWindow, ...) > > Wouldn't it be possible to do something like > Object.getOwnPropertyDescriptor(window, > "attribute").get.call(crossOriginWindow)? It is, but this hits the access check before it ever gets to Blink code. (I don't actually understand why, so I'm investigating this) > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:99: ScriptState* > > scriptState = ScriptState::forReceiverObject(info); > > On 2017/02/23 09:26:24, haraken wrote: > > > > > > Nit: You're potentially redefining the ScriptState :) The compile is just > > > passing because it doesn't happen in practice by accident... > > > > Done.
On 2017/02/24 08:51:44, dcheng wrote: > On 2017/02/24 05:07:48, haraken wrote: > > On 2017/02/23 09:44:43, dcheng wrote: > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right): > > > > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% if > > > world_suffix in method.activity_logging_world_list %} > > > On 2017/02/23 09:26:24, haraken wrote: > > > > On 2017/02/23 04:55:41, dcheng wrote: > > > > > On 2017/02/23 04:30:20, haraken wrote: > > > > > > > > > > > > What is this change for? > > > > > > > > > > This needs to be done after the security check; otherwise, we might call > > > this > > > > on > > > > > a remote context, and a remote context doesn't have an associated > > > ScriptState. > > > > > > > > Hmm. > > > > > > > > Then you will need to make a similar change to attributes.cpp.tmpl? I > guess > > > > there're many places where we're using ScriptState before doing the > security > > > > check... > > > > > > > > > > As far as I know, it's only a problem for methods due to the fact that you > can > > > write code like: > > > > > > window.setTimeout(crossOriginWindow, ...) > > > > Wouldn't it be possible to do something like > > Object.getOwnPropertyDescriptor(window, > > "attribute").get.call(crossOriginWindow)? > > It is, but this hits the access check before it ever gets to Blink code. > (I don't actually understand why, so I'm investigating this) OK, so the reason is v8 handles the access check automatically in HandleApiCallHelper for invoking the getter from getOwnPropertyDescriptor, etc. However, methods don't have this access check: that's why the generated methods for setTimeout, etc, have to do their own access check. > > > > > > > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:99: > ScriptState* > > > scriptState = ScriptState::forReceiverObject(info); > > > On 2017/02/23 09:26:24, haraken wrote: > > > > > > > > Nit: You're potentially redefining the ScriptState :) The compile is just > > > > passing because it doesn't happen in practice by accident... > > > > > > Done.
On 2017/02/24 08:58:05, dcheng wrote: > On 2017/02/24 08:51:44, dcheng wrote: > > On 2017/02/24 05:07:48, haraken wrote: > > > On 2017/02/23 09:44:43, dcheng wrote: > > > > > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% if > > > > world_suffix in method.activity_logging_world_list %} > > > > On 2017/02/23 09:26:24, haraken wrote: > > > > > On 2017/02/23 04:55:41, dcheng wrote: > > > > > > On 2017/02/23 04:30:20, haraken wrote: > > > > > > > > > > > > > > What is this change for? > > > > > > > > > > > > This needs to be done after the security check; otherwise, we might > call > > > > this > > > > > on > > > > > > a remote context, and a remote context doesn't have an associated > > > > ScriptState. > > > > > > > > > > Hmm. > > > > > > > > > > Then you will need to make a similar change to attributes.cpp.tmpl? I > > guess > > > > > there're many places where we're using ScriptState before doing the > > security > > > > > check... > > > > > > > > > > > > > As far as I know, it's only a problem for methods due to the fact that you > > can > > > > write code like: > > > > > > > > window.setTimeout(crossOriginWindow, ...) > > > > > > Wouldn't it be possible to do something like > > > Object.getOwnPropertyDescriptor(window, > > > "attribute").get.call(crossOriginWindow)? > > > > It is, but this hits the access check before it ever gets to Blink code. > > (I don't actually understand why, so I'm investigating this) > > OK, so the reason is v8 handles the access check automatically in > HandleApiCallHelper for invoking the getter from getOwnPropertyDescriptor, etc. > However, methods don't have this access check: that's why the generated methods > for setTimeout, etc, have to do their own access check. Do you know why we don't have the access check for methods whereas we have the check for attributes? (Sorry for sticking to this issue too much -- I'm worried about the access check since it has caused a couple of security issues in the past...)
On 2017/02/25 00:35:34, haraken wrote: > On 2017/02/24 08:58:05, dcheng wrote: > > On 2017/02/24 08:51:44, dcheng wrote: > > > On 2017/02/24 05:07:48, haraken wrote: > > > > On 2017/02/23 09:44:43, dcheng wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > > > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% if > > > > > world_suffix in method.activity_logging_world_list %} > > > > > On 2017/02/23 09:26:24, haraken wrote: > > > > > > On 2017/02/23 04:55:41, dcheng wrote: > > > > > > > On 2017/02/23 04:30:20, haraken wrote: > > > > > > > > > > > > > > > > What is this change for? > > > > > > > > > > > > > > This needs to be done after the security check; otherwise, we might > > call > > > > > this > > > > > > on > > > > > > > a remote context, and a remote context doesn't have an associated > > > > > ScriptState. > > > > > > > > > > > > Hmm. > > > > > > > > > > > > Then you will need to make a similar change to attributes.cpp.tmpl? I > > > guess > > > > > > there're many places where we're using ScriptState before doing the > > > security > > > > > > check... > > > > > > > > > > > > > > > > As far as I know, it's only a problem for methods due to the fact that > you > > > can > > > > > write code like: > > > > > > > > > > window.setTimeout(crossOriginWindow, ...) > > > > > > > > Wouldn't it be possible to do something like > > > > Object.getOwnPropertyDescriptor(window, > > > > "attribute").get.call(crossOriginWindow)? > > > > > > It is, but this hits the access check before it ever gets to Blink code. > > > (I don't actually understand why, so I'm investigating this) > > > > OK, so the reason is v8 handles the access check automatically in > > HandleApiCallHelper for invoking the getter from getOwnPropertyDescriptor, > etc. > > However, methods don't have this access check: that's why the generated > methods > > for setTimeout, etc, have to do their own access check. > > Do you know why we don't have the access check for methods whereas we have the > check for attributes? > > (Sorry for sticking to this issue too much -- I'm worried about the access check > since it has caused a couple of security issues in the past...) I did some investigation in the debugger. For normal methods calls, it seems that accept_any_receiver() is set to true, so HandleApiCallHelper() doesn't do the security check: https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-api.cc?rcl=7652... However, for accessor properties, it seems like we manually set SetAcceptAnyReceiver to false: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... Thus, this is the difference we see in behavior. I'd like to try installing non-cross origin Window/Location methods with this set to false, but in a followup if that's OK =)
On 2017/02/25 03:16:38, dcheng wrote: > On 2017/02/25 00:35:34, haraken wrote: > > On 2017/02/24 08:58:05, dcheng wrote: > > > On 2017/02/24 08:51:44, dcheng wrote: > > > > On 2017/02/24 05:07:48, haraken wrote: > > > > > On 2017/02/23 09:44:43, dcheng wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > > > > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > > > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% > if > > > > > > world_suffix in method.activity_logging_world_list %} > > > > > > On 2017/02/23 09:26:24, haraken wrote: > > > > > > > On 2017/02/23 04:55:41, dcheng wrote: > > > > > > > > On 2017/02/23 04:30:20, haraken wrote: > > > > > > > > > > > > > > > > > > What is this change for? > > > > > > > > > > > > > > > > This needs to be done after the security check; otherwise, we > might > > > call > > > > > > this > > > > > > > on > > > > > > > > a remote context, and a remote context doesn't have an associated > > > > > > ScriptState. > > > > > > > > > > > > > > Hmm. > > > > > > > > > > > > > > Then you will need to make a similar change to attributes.cpp.tmpl? > I > > > > guess > > > > > > > there're many places where we're using ScriptState before doing the > > > > security > > > > > > > check... > > > > > > > > > > > > > > > > > > > As far as I know, it's only a problem for methods due to the fact that > > you > > > > can > > > > > > write code like: > > > > > > > > > > > > window.setTimeout(crossOriginWindow, ...) > > > > > > > > > > Wouldn't it be possible to do something like > > > > > Object.getOwnPropertyDescriptor(window, > > > > > "attribute").get.call(crossOriginWindow)? > > > > > > > > It is, but this hits the access check before it ever gets to Blink code. > > > > (I don't actually understand why, so I'm investigating this) > > > > > > OK, so the reason is v8 handles the access check automatically in > > > HandleApiCallHelper for invoking the getter from getOwnPropertyDescriptor, > > etc. > > > However, methods don't have this access check: that's why the generated > > methods > > > for setTimeout, etc, have to do their own access check. > > > > Do you know why we don't have the access check for methods whereas we have the > > check for attributes? > > > > (Sorry for sticking to this issue too much -- I'm worried about the access > check > > since it has caused a couple of security issues in the past...) > > I did some investigation in the debugger. For normal methods calls, it seems > that accept_any_receiver() is set to true, so HandleApiCallHelper() doesn't do > the security check: > https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-api.cc?rcl=7652... > > However, for accessor properties, it seems like we manually set > SetAcceptAnyReceiver to false: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > > Thus, this is the difference we see in behavior. > > I'd like to try installing non-cross origin Window/Location methods with this > set to false, but in a followup if that's OK =) And just to summarize: accessor properties: we specifically enable access checks by calling SetAcceptAnyReceiver(false). This means we do not need to do access checks in Blink. methods: we do not set SetAcceptAnyReceiver, so it defaults to true. This means we need to do access checks in Blink (and we do).
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.
On 2017/02/25 03:51:46, dcheng wrote: > On 2017/02/25 03:16:38, dcheng wrote: > > On 2017/02/25 00:35:34, haraken wrote: > > > On 2017/02/24 08:58:05, dcheng wrote: > > > > On 2017/02/24 08:51:44, dcheng wrote: > > > > > On 2017/02/24 05:07:48, haraken wrote: > > > > > > On 2017/02/23 09:44:43, dcheng wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > > > > > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2626183003/diff/340001/third_party/WebKit/Sou... > > > > > > > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:81: {% > > if > > > > > > > world_suffix in method.activity_logging_world_list %} > > > > > > > On 2017/02/23 09:26:24, haraken wrote: > > > > > > > > On 2017/02/23 04:55:41, dcheng wrote: > > > > > > > > > On 2017/02/23 04:30:20, haraken wrote: > > > > > > > > > > > > > > > > > > > > What is this change for? > > > > > > > > > > > > > > > > > > This needs to be done after the security check; otherwise, we > > might > > > > call > > > > > > > this > > > > > > > > on > > > > > > > > > a remote context, and a remote context doesn't have an > associated > > > > > > > ScriptState. > > > > > > > > > > > > > > > > Hmm. > > > > > > > > > > > > > > > > Then you will need to make a similar change to > attributes.cpp.tmpl? > > I > > > > > guess > > > > > > > > there're many places where we're using ScriptState before doing > the > > > > > security > > > > > > > > check... > > > > > > > > > > > > > > > > > > > > > > As far as I know, it's only a problem for methods due to the fact > that > > > you > > > > > can > > > > > > > write code like: > > > > > > > > > > > > > > window.setTimeout(crossOriginWindow, ...) > > > > > > > > > > > > Wouldn't it be possible to do something like > > > > > > Object.getOwnPropertyDescriptor(window, > > > > > > "attribute").get.call(crossOriginWindow)? > > > > > > > > > > It is, but this hits the access check before it ever gets to Blink code. > > > > > (I don't actually understand why, so I'm investigating this) > > > > > > > > OK, so the reason is v8 handles the access check automatically in > > > > HandleApiCallHelper for invoking the getter from getOwnPropertyDescriptor, > > > etc. > > > > However, methods don't have this access check: that's why the generated > > > methods > > > > for setTimeout, etc, have to do their own access check. > > > > > > Do you know why we don't have the access check for methods whereas we have > the > > > check for attributes? > > > > > > (Sorry for sticking to this issue too much -- I'm worried about the access > > check > > > since it has caused a couple of security issues in the past...) > > > > I did some investigation in the debugger. For normal methods calls, it seems > > that accept_any_receiver() is set to true, so HandleApiCallHelper() doesn't do > > the security check: > > > https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-api.cc?rcl=7652... > > > > However, for accessor properties, it seems like we manually set > > SetAcceptAnyReceiver to false: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > > > > Thus, this is the difference we see in behavior. > > > > I'd like to try installing non-cross origin Window/Location methods with this > > set to false, but in a followup if that's OK =) > > And just to summarize: > > accessor properties: we specifically enable access checks by calling > SetAcceptAnyReceiver(false). This means we do not need to do access checks in > Blink. > methods: we do not set SetAcceptAnyReceiver, so it defaults to true. This means > we need to do access checks in Blink (and we do). Thanks for the investigation! I'm fine with fixing the issue in a follow-up. LGTM.
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org, lfg@chromium.org Link to the patchset: https://codereview.chromium.org/2626183003/#ps380001 (title: "Address comment.")
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org, lfg@chromium.org Link to the patchset: https://codereview.chromium.org/2626183003/#ps400001 (title: "rebase")
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": 400001, "attempt_start_ts": 1488142907160930,
"parent_rev": "6fbdb9993cb2aa58c89e37efba312ad00d8e16ca", "commit_rev":
"4a40a757e7029a3f59b12543cbf617eceb15f7f0"}
Message was sent while issue was closed.
Description was changed from ========== Switch RemoteWindowProxy to use v8::Context::NewRemoteContext. BUG=527190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Switch RemoteWindowProxy to use v8::Context::NewRemoteContext. BUG=527190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2626183003 Cr-Commit-Position: refs/heads/master@{#453128} Committed: https://chromium.googlesource.com/chromium/src/+/4a40a757e7029a3f59b12543cbf6... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/4a40a757e7029a3f59b12543cbf6...
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:400001) has been created in https://codereview.chromium.org/2721693002/ by dcheng@chromium.org. The reason for reverting is: Causing crashes in canary: https://crbug.com/696528, https://crbug.com/696577. |
