Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2745313003:
Move securityCheck out of V8WrapperInstantiationScope (Closed)
DescriptionMove securityCheck out of V8WrapperInstantiationScope
Moving V8DOMWrapper.h/cpp to platform/bindings is blocked by V8WrapperInstantiationScope::securityCheck and V8WrapperInstantiationScope::convertException. Both of these methods use toLocalFrame/toDOMWindow, and cannot be moved to platform/.
This CL moves the security check into WrapperCreationSecurityCheck. WrapperCreationSecurityCheck only holds a function pointer and will be moved into platform/bindings. The implementation of the security check is moved to BindingSecurity (in bindings/core), and the function pointer is set to point to the implementation inside core/.
BUG=682322
Review-Url: https://codereview.chromium.org/2745313003
Cr-Original-Commit-Position: refs/heads/master@{#463621}
Committed: https://chromium.googlesource.com/chromium/src/+/3f7eac384ee02371be4972ba0647082bf6599eb1
Review-Url: https://codereview.chromium.org/2745313003
Cr-Commit-Position: refs/heads/master@{#466029}
Committed: https://chromium.googlesource.com/chromium/src/+/32ce98203b3580adb4e8553c34cbb677a0fa6203
Patch Set 1 #Patch Set 2 : Small changes #Patch Set 3 : Move entire security check into BindingSecurity #Patch Set 4 : Stop unecessary includes of BindingSecurity.h #
Total comments: 20
Patch Set 5 : Code review fixes #Patch Set 6 : Use a bit flag instead of storing a function pointer #Patch Set 7 : Move functions into static class and remove flag bit #
Total comments: 23
Patch Set 8 : Address code review feedback #
Total comments: 3
Patch Set 9 : Do security check before entering creation context #
Total comments: 10
Patch Set 10 : Code review changes #
Total comments: 21
Patch Set 11 : Add comments and DCHECKs #
Total comments: 7
Patch Set 12 : Rebase, remove Isolate* parameter #Patch Set 13 : Move init to V8Initializer #
Total comments: 2
Patch Set 14 : Create wrapper even after security check fails #Patch Set 15 : Add TODO #Messages
Total messages: 87 (46 generated)
The CQ bit was checked by adithyas@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Move securityCheck out of V8WrapperInstantiationScope BUG=682322 ========== to ========== Move securityCheck out of V8WrapperInstantiationScope Moving V8DOMWrapper.h/cpp to platform/bindings is blocked by V8WrapperInstantiationScope::securityCheck and V8WrapperInstantiationScope::convertException. Both of these methods use toLocalFrame/toDOMWindow, and cannot be moved to platform/. This CL moves the security check into a callback held by WrapperTypeInfo. The callback 'securityCheck' is set along with the other WrapperTypeInfo fields inside the generated bindings files for each class, and the implementation of the callback is kept in BindingSecurity.h. Classes with cross origin access callbacks (i.e. have methods defined with [CrossOrigin] attribute) don't do a security check, but do convert any cross-context exceptions thrown during wrapper creation to cross-origin security errors. Other classes do a security check and throw an error if cross-origin access is not allowed. BUG=682322 ==========
The CQ bit was checked by adithyas@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.
adithyas@chromium.org changed reviewers: + haraken@chromium.org, jbroman@chromium.org, jochen@chromium.org
https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:143: if you move this out, you'll want to make sure that we crash if we get here without a security check callback defined - outside of blink's complicated security model, we'll want to blanket disallow any cross context wrappers. https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:163: if (m_tryCatch.HasCaught()) that won't work as you reset the trycatch above https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:13: {% set context_access_check = '0' %} why this?
also, I wonder what the impact on binary size is?
https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:289: ExceptionState exceptionState(isolate, ExceptionState::ConstructionContext, nit: This is the same as the ExceptionState below, and the one in securityCheckForClassesWithoutAccessCheckCallbacks. Maybe just pass one from the latter method in here? https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:160: m_type->securityCheck(isolate, isolate->GetCurrentContext(), m_context, Do ArrayBuffer/ArrayBufferViews not go through V8WrapperInstantiationScope? It looks like their |securityCheck| is null (which would segfault here).
To avoid adding additional bytes to each WrapperTypeInfo, would it be an option to move the securityCheck to ScriptWrappable? Window and Location can override a virtual securityCheck.
On 2017/03/15 at 20:25:40, haraken wrote: > To avoid adding additional bytes to each WrapperTypeInfo, would it be an option to move the securityCheck to ScriptWrappable? Window and Location can override a virtual securityCheck. Aside: not to speak for or against that approach, but I don't think there are actually that many WrapperTypeInfo. On the side I've trimmed bytes from much more common data structures (the per-method descriptor struct), and that has had a binary size impact on the order of 10K. I'd expect this to be an order of magnitude less (because most interfaces have several methods).
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:279: if (currentContext.IsEmpty() || creationContext.IsEmpty()) Is that possible that currentContext.IsEmpty() nor creationContext.IsEmpty()? It's strange... https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:324: ExceptionState exceptionState(isolate, ExceptionState::ConstructionContext, In general, ExceptionState should be instantiated in the call sites to better handle multiple possibilities of throwing an exception. So, I'd recommend this function to take an ExceptionState. Plus, in this case, it looks strange that you knew it's ExceptionState::ConstructionContext. https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:124: static bool canEnterCreationContext(v8::Isolate*, Do you really want to expose this function in public? https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:129: static void securityCheckForClassesWithAccessCheckCallbacks( Does "Classes" here mean IDL interface? If so, it's a bit confusing because "class" sounds like a C++ class. Also, rethrowExceptionAsSecurityError or something like that seems a better name to me. https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:36: {{wrapper_type_info_const}}WrapperTypeInfo {{v8_class}}::wrapperTypeInfo = { gin::kEmbedderBlink, {{dom_template}}, {{context_access_check}}, {{v8_class}}::trace, {{v8_class}}::traceWrappers, {{prepare_prototype_and_interface_object_func or 'nullptr'}}, "{{interface_name}}", {{parent_wrapper_type_info}}, WrapperTypeInfo::{{wrapper_type_prototype}}, WrapperTypeInfo::{{wrapper_class_id}}, WrapperTypeInfo::{{active_scriptwrappable_inheritance}}, WrapperTypeInfo::{{lifetime}} }; I'm afraid that this would increase the binary size and/or memory size of Blink. We don't need to have a pointer, a single (or two) bit is enough. Given that we knew that only Location needs a special care, I think it's acceptable to hard-code it in WrapperTypeInfo or somewhere else. No need to have this entry in WrapperTypeInfo.
I compared the binary sizes and it looks like this patch increases the binary size by 3256 bytes (Android), of which 2980 bytes are a direct result of adding a pointer to WrapperTypeInfo. On 2017/03/15 at 20:25:40, haraken wrote: > To avoid adding additional bytes to each WrapperTypeInfo, would it be an option to move the securityCheck to ScriptWrappable? Window and Location can override a virtual securityCheck. This patch is moving the security check implementation in order to remove core/ dependencies from ScriptWrappable and related classes. So moving the securityCheck implementation to ScriptWrappable would go against that. https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:279: if (currentContext.IsEmpty() || creationContext.IsEmpty()) On 2017/03/16 at 14:05:55, Yuki wrote: > Is that possible that currentContext.IsEmpty() nor creationContext.IsEmpty()? It's strange... This was just supposed to be currentContext.IsEmpty(), there's a RELEASE_ASSERT for the creationContext earlier. https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:289: ExceptionState exceptionState(isolate, ExceptionState::ConstructionContext, On 2017/03/15 at 19:58:41, jbroman wrote: > nit: This is the same as the ExceptionState below, and the one in securityCheckForClassesWithoutAccessCheckCallbacks. Maybe just pass one from the latter method in here? Done! https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:324: ExceptionState exceptionState(isolate, ExceptionState::ConstructionContext, On 2017/03/16 at 14:05:55, Yuki wrote: > In general, ExceptionState should be instantiated in the call sites to better handle multiple possibilities of throwing an exception. So, I'd recommend this function to take an ExceptionState. Plus, in this case, it looks strange that you knew it's ExceptionState::ConstructionContext. I didn't create an exception state in the call site to remove a dependency to ExceptionState (which I'm not planning on moving to platform/ initially) in V8DOMWrapper. Also, this security check is only called when creating a wrapper, so I think it's ok if it has knowledge of ExceptionState::ConstructionContext. https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:124: static bool canEnterCreationContext(v8::Isolate*, On 2017/03/16 at 14:05:55, Yuki wrote: > Do you really want to expose this function in public? Nope, made it private. https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:129: static void securityCheckForClassesWithAccessCheckCallbacks( On 2017/03/16 at 14:05:55, Yuki wrote: > Does "Classes" here mean IDL interface? If so, it's a bit confusing because "class" sounds like a C++ class. > > Also, rethrowExceptionAsSecurityError or something like that seems a better name to me. Yes, I meant IDL interface here and not a C++ class. I think rethrowExceptionAsSecurityError makes sense here, but not for the other function, so I'll change the other one to securityCheckForInterfacesWithoutAccessCheckCallbacks. https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:143: On 2017/03/15 at 19:56:13, jochen wrote: > if you move this out, you'll want to make sure that we crash if we get here without a security check callback defined - outside of blink's complicated security model, we'll want to blanket disallow any cross context wrappers. Added an assertion. https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:160: m_type->securityCheck(isolate, isolate->GetCurrentContext(), m_context, On 2017/03/15 at 19:58:41, jbroman wrote: > Do ArrayBuffer/ArrayBufferViews not go through V8WrapperInstantiationScope? It looks like their |securityCheck| is null (which would segfault here). They do not (they don't call V8DOMWrapper::createWrapper to create wrappers). https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:163: if (m_tryCatch.HasCaught()) On 2017/03/15 at 19:56:13, jochen wrote: > that won't work as you reset the trycatch above m_type->securityCheck can throw an exception, and this is to check if it did. That call happens after the reset so I think this is ok. https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl (right): https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:13: {% set context_access_check = '0' %} On 2017/03/15 at 19:56:13, jochen wrote: > why this? Array buffers/views don't call V8DOMWrapper::createWrapper (which uses V8WrapperInstantiationScope and does the security check). https://codereview.chromium.org/2745313003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:36: {{wrapper_type_info_const}}WrapperTypeInfo {{v8_class}}::wrapperTypeInfo = { gin::kEmbedderBlink, {{dom_template}}, {{context_access_check}}, {{v8_class}}::trace, {{v8_class}}::traceWrappers, {{prepare_prototype_and_interface_object_func or 'nullptr'}}, "{{interface_name}}", {{parent_wrapper_type_info}}, WrapperTypeInfo::{{wrapper_type_prototype}}, WrapperTypeInfo::{{wrapper_class_id}}, WrapperTypeInfo::{{active_scriptwrappable_inheritance}}, WrapperTypeInfo::{{lifetime}} }; On 2017/03/16 at 14:05:55, Yuki wrote: > I'm afraid that this would increase the binary size and/or memory size of Blink. We don't need to have a pointer, a single (or two) bit is enough. > > Given that we knew that only Location needs a special care, I think it's acceptable to hard-code it in WrapperTypeInfo or somewhere else. No need to have this entry in WrapperTypeInfo. I'm using a function pointer instead of just a bit because I need to move the security check implementation out of V8WrapperInstantiationScope (and not just the V8Location check).
The CQ bit was checked by adithyas@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: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
I don't understand why you don't use bit flags. I think that we can do something like follows: a) Have a few bits in WrapperTypeInfo to indicate the type of security checks. b) Have a few callback function pointers in platform/bindings. c) Set these callback function pointers to functions in bindings/core/v8/. d) Call one of callback functions depending on the bits in WrapperTypeInfo. Then, the dependency issue will be solved, I think? IIUC, we don't need to have raw function pointers in WrapperTypeInfo.
The CQ bit was checked by adithyas@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...
On 2017/03/29 at 13:11:04, yukishiino wrote: > I don't understand why you don't use bit flags. > > I think that we can do something like follows: > a) Have a few bits in WrapperTypeInfo to indicate the type of security checks. > b) Have a few callback function pointers in platform/bindings. > c) Set these callback function pointers to functions in bindings/core/v8/. > d) Call one of callback functions depending on the bits in WrapperTypeInfo. > > Then, the dependency issue will be solved, I think? IIUC, we don't need to have raw function pointers in WrapperTypeInfo. Ahh I see, yeah that should solve the dependency issue. I've updated the CL to use a single bit in WrapperTypeInfo instead of a function pointer. Looking at it now, we could also just have one function pointer instead of 2, and the function it points to (which would be in bindings/core) can check if the type is V8Location and act accordingly. This would mean we don't need the flag bit at all, but the code would be less generalized. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/29 20:40:35, adithyas wrote: > On 2017/03/29 at 13:11:04, yukishiino wrote: > > I don't understand why you don't use bit flags. > > > > I think that we can do something like follows: > > a) Have a few bits in WrapperTypeInfo to indicate the type of security checks. > > b) Have a few callback function pointers in platform/bindings. > > c) Set these callback function pointers to functions in bindings/core/v8/. > > d) Call one of callback functions depending on the bits in WrapperTypeInfo. > > > > Then, the dependency issue will be solved, I think? IIUC, we don't need to > have raw function pointers in WrapperTypeInfo. > > Ahh I see, yeah that should solve the dependency issue. I've updated the CL to > use a single bit in WrapperTypeInfo instead of a function pointer. > > Looking at it now, we could also just have one function pointer instead of 2, > and the function it points to (which would be in bindings/core) can check if the > type is V8Location and act accordingly. This would mean we don't need the flag > bit at all, but the code would be less generalized. WDYT? My preference is "no bit flag at all, and check the type in bindings/core". But if someone has a strong objection, I'm fine with a bit flag, too. By the way, I don't like to increase global symbols in general. e.g. blink::SecurityCheckFunction, blink::setSecurityCheckFunction, blink::securityCheck!!! They look like having too general name compared to a specific purpose. Could you put them inside a new STATIC_ONLY class or something like that? (And don't give it a too general name.)
On 2017/03/30 at 08:07:22, yukishiino wrote: > On 2017/03/29 20:40:35, adithyas wrote: > > On 2017/03/29 at 13:11:04, yukishiino wrote: > > > I don't understand why you don't use bit flags. > > > > > > I think that we can do something like follows: > > > a) Have a few bits in WrapperTypeInfo to indicate the type of security checks. > > > b) Have a few callback function pointers in platform/bindings. > > > c) Set these callback function pointers to functions in bindings/core/v8/. > > > d) Call one of callback functions depending on the bits in WrapperTypeInfo. > > > > > > Then, the dependency issue will be solved, I think? IIUC, we don't need to > > have raw function pointers in WrapperTypeInfo. > > > > Ahh I see, yeah that should solve the dependency issue. I've updated the CL to > > use a single bit in WrapperTypeInfo instead of a function pointer. > > > > Looking at it now, we could also just have one function pointer instead of 2, > > and the function it points to (which would be in bindings/core) can check if the > > type is V8Location and act accordingly. This would mean we don't need the flag > > bit at all, but the code would be less generalized. WDYT? > > My preference is "no bit flag at all, and check the type in bindings/core". But if someone has a strong objection, I'm fine with a bit flag, too. > > By the way, I don't like to increase global symbols in general. e.g. > blink::SecurityCheckFunction, > blink::setSecurityCheckFunction, > blink::securityCheck!!! > They look like having too general name compared to a specific purpose. > > Could you put them inside a new STATIC_ONLY class or something like that? (And don't give it a too general name.) Ok, I moved the functions into a STATIC_ONLY class and removed the flag bit.
The CQ bit was checked by adithyas@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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Could you update the CL description? https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:276: bool BindingSecurity::canEnterCreationContext( nit: Could you place the implementation in the same order to the declaration in the header? The style guide recommends the same order. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:281: if (currentContext.IsEmpty()) This must never happen, I think. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:296: CHECK_EQ(SecurityError, exceptionState.code()); This is guaranteed. If you'd like to add a CHECK, then it's better to put in canAccessFrame. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:300: RELEASE_ASSERT(currentWorld.worldId() == s/RELEASE_ASSERT/CHECK_EQ/ https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:315: v8::Local<v8::Context> currentContext, I'd suggest s/currentContext/accessingContext/ s/creationContext/targetContext/ or, if you think that this is specific to the wrapper creation and you'd like to say "creationContext", then I'd recommend not to take |currentContext|. You can use isolate->GetCurrentContext() which is exactly the current context while |currentContext| is not guaranteed to be the current context. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:327: if (!crossContextException.IsEmpty()) { I think this function is "rethrow an exception in a secure manner", and if there is no exception, nothing to do here at all. Why don't you do the early-exit? https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:329: LocalDOMWindow* callingWindow = currentDOMWindow(isolate); Be consistent with the arguments. Here you ignore |currentContext| and use currentDOMWindow(isolate). If you really want to take the accessing context, this should be toLocalDOMWindow(accessingContext). https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:335: } else { You can return just after exceptionState.throwSecurityError (and it's recommended because you shouldn't do more things after you threw an exception), and then we don't need |else| here. Please reduce the nest level. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:336: if (canEnterCreationContext(isolate, currentContext, creationContext, I'd personally feel that this helper function doesn't help me understand the code well. Probably it would be easier to read if the code is expanded here. Or, probably because the name "canEnterCreationContext" sounds like it's just checking, but we're actually expecting it to throw an exception. (shouldAllowAccessTo should have the same issue, but I've already accustomed to it...) https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:161: WrapperCreationSecurityCheck::securityCheck( This code seems expected to (re)throw an exception in a secure manner, rather than just performing a security check. Maybe better to change the name? https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:165: if (m_tryCatch.HasCaught()) You've reset m_tryCatch on line 160. This is meaningless, isn't it?
Description was changed from ========== Move securityCheck out of V8WrapperInstantiationScope Moving V8DOMWrapper.h/cpp to platform/bindings is blocked by V8WrapperInstantiationScope::securityCheck and V8WrapperInstantiationScope::convertException. Both of these methods use toLocalFrame/toDOMWindow, and cannot be moved to platform/. This CL moves the security check into a callback held by WrapperTypeInfo. The callback 'securityCheck' is set along with the other WrapperTypeInfo fields inside the generated bindings files for each class, and the implementation of the callback is kept in BindingSecurity.h. Classes with cross origin access callbacks (i.e. have methods defined with [CrossOrigin] attribute) don't do a security check, but do convert any cross-context exceptions thrown during wrapper creation to cross-origin security errors. Other classes do a security check and throw an error if cross-origin access is not allowed. BUG=682322 ========== to ========== Move securityCheck out of V8WrapperInstantiationScope Moving V8DOMWrapper.h/cpp to platform/bindings is blocked by V8WrapperInstantiationScope::securityCheck and V8WrapperInstantiationScope::convertException. Both of these methods use toLocalFrame/toDOMWindow, and cannot be moved to platform/. This CL moves the security check into WrapperCreationSecurityCheck. WrapperCreationSecurityCheck only holds a function pointer and will be moved into platform/bindings. The implementation of the security check is moved to BindingSecurity (in bindings/core), and the function pointer is set to point to the implementation inside core/. BUG=682322 ==========
The CQ bit was checked by adithyas@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...
https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:276: bool BindingSecurity::canEnterCreationContext( On 2017/03/31 at 09:49:37, Yuki wrote: > nit: Could you place the implementation in the same order to the declaration in the header? > The style guide recommends the same order. Fixed https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:296: CHECK_EQ(SecurityError, exceptionState.code()); On 2017/03/31 at 09:49:37, Yuki wrote: > This is guaranteed. If you'd like to add a CHECK, then it's better to put in canAccessFrame. Sorry, this CHECK existed before, and I wasn't quite sure why. I've removed it now. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:315: v8::Local<v8::Context> currentContext, On 2017/03/31 at 09:49:37, Yuki wrote: > I'd suggest > > s/currentContext/accessingContext/ > s/creationContext/targetContext/ > > or, if you think that this is specific to the wrapper creation and you'd like to say "creationContext", then I'd recommend not to take |currentContext|. You can use isolate->GetCurrentContext() which is exactly the current context while |currentContext| is not guaranteed to be the current context. I do think this is specific to wrapper creation so I will use isolate->GetCurrentContext(). https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:327: if (!crossContextException.IsEmpty()) { On 2017/03/31 at 09:49:37, Yuki wrote: > I think this function is "rethrow an exception in a secure manner", and if there is no exception, nothing to do here at all. Why don't you do the early-exit? Makes sense, changed to an early-exit. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:329: LocalDOMWindow* callingWindow = currentDOMWindow(isolate); On 2017/03/31 at 09:49:37, Yuki wrote: > Be consistent with the arguments. Here you ignore |currentContext| and use currentDOMWindow(isolate). If you really want to take the accessing context, this should be toLocalDOMWindow(accessingContext). Fixed to use isolate->GetCurrentContext() / currentDOMWindow() consistently. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:335: } else { On 2017/03/31 at 09:49:37, Yuki wrote: > You can return just after exceptionState.throwSecurityError (and it's recommended because you shouldn't do more things after you threw an exception), and then we don't need |else| here. Please reduce the nest level. Done https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:336: if (canEnterCreationContext(isolate, currentContext, creationContext, On 2017/03/31 at 09:49:37, Yuki wrote: > I'd personally feel that this helper function doesn't help me understand the code well. Probably it would be easier to read if the code is expanded here. > > Or, probably because the name "canEnterCreationContext" sounds like it's just checking, but we're actually expecting it to throw an exception. (shouldAllowAccessTo should have the same issue, but I've already accustomed to it...) Coming up with a good name is difficult here :) I think you're right, the helper function does not make it easier to understand, so I've just inlined the function. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:161: WrapperCreationSecurityCheck::securityCheck( On 2017/03/31 at 09:49:37, Yuki wrote: > This code seems expected to (re)throw an exception in a secure manner, rather than just performing a security check. Maybe better to change the name? OK, changed to a more descriptive name. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:165: if (m_tryCatch.HasCaught()) On 2017/03/31 at 09:49:37, Yuki wrote: > You've reset m_tryCatch on line 160. This is meaningless, isn't it? Hmm, does Reset() completely disable the TryCatch? securityCheck can throw an exception, and if it does throw an exception, the tryCatch would catch it and HasCaught() would return true right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Mostly looks good. https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:165: if (m_tryCatch.HasCaught()) On 2017/03/31 17:49:28, adithyas wrote: > On 2017/03/31 at 09:49:37, Yuki wrote: > > You've reset m_tryCatch on line 160. This is meaningless, isn't it? > > Hmm, does Reset() completely disable the TryCatch? securityCheck can throw an > exception, and if it does throw an exception, the tryCatch would catch it and > HasCaught() would return true right? Ah, now I see the point. Then, I'd prefer an early-exit here, too. if (!m_didEnterContext) { m_tryCatch.ReThrow(); return; } m_context->Exit(); if (!m_tryCatch.HasCaught()) return; // A cross-context exception exists, so rethrow it. verifyContextAccessAndHandleCrossContextException(); m_tryCatch.ReThrow(); Given the above logic, I'd feel that rethrowCrossContextException(); m_tryCatch.ReThrow(); would be more straightforward because "(re)throw an exception" is the expected behavior. The following m_tryCatch.ReThrow() makes sense at a glance. https://codereview.chromium.org/2745313003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:270: if (crossContextException.IsEmpty()) I meant that we can do an early-exit at the beginning. void BindingSecurity::wrapperCreationSecurityCheck(...) { if (crossContextException.IsEmpty()) return; ExceptionState exceptionState(...); ... } However, now I think that it's better to do this early-exit on the call site. See my reply at: https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... Given that we do the early-exit on the call site, we can write: void BindingSecurity::wrapperCreationSecurityCheck(...) { DCHECK(!crossContextException.IsEmpty()); ... } And then, I think that the expected behavior of this function will be "rethrow an exception", and I'd prefer "rethrowCrossContextException" or something. It's a bit strange that fooCheck() returns void. https://codereview.chromium.org/2745313003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:304: } You may want: DCHECK(exceptionState.hadException());
https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:165: if (m_tryCatch.HasCaught()) On 2017/04/03 at 08:29:25, Yuki wrote: > On 2017/03/31 17:49:28, adithyas wrote: > > On 2017/03/31 at 09:49:37, Yuki wrote: > > > You've reset m_tryCatch on line 160. This is meaningless, isn't it? > > > > Hmm, does Reset() completely disable the TryCatch? securityCheck can throw an > > exception, and if it does throw an exception, the tryCatch would catch it and > > HasCaught() would return true right? > > Ah, now I see the point. Then, I'd prefer an early-exit here, too. > > if (!m_didEnterContext) { > m_tryCatch.ReThrow(); > return; > } > m_context->Exit(); > > if (!m_tryCatch.HasCaught()) > return; > > // A cross-context exception exists, so rethrow it. > verifyContextAccessAndHandleCrossContextException(); > m_tryCatch.ReThrow(); > > Given the above logic, I'd feel that > rethrowCrossContextException(); > m_tryCatch.ReThrow(); > would be more straightforward because "(re)throw an exception" is the expected behavior. The following m_tryCatch.ReThrow() makes sense at a glance. I think verifyContextAccessAndHandleCrossContextException needs to be called regardless of whether a cross context exception is thrown or not. The security checks (shouldAllowAccessToFrame/DetachedWindow) inside the method needs to be run regardless of whether a cross context exception was thrown. I could maybe split it up into two methods: verifyContextAccess and rethrowCrossContextException if (!m_didEnterContext) { m_tryCatch.ReThrow(); return; } m_context->Exit(); crossContextException = m_tryCatch.Exception(); m_tryCatch.Reset(); if (!verifyContextAccess()) { // verifyContextAccess throws a security error when it returns false m_tryCatch.ReThrow(); return; } if (crossContextException.Empty()) return; rethrowCrossContextException(crossContextException); m_tryCatch.ReThrow(); WDYT? https://codereview.chromium.org/2745313003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:270: if (crossContextException.IsEmpty()) On 2017/04/03 at 08:29:26, Yuki wrote: > I meant that we can do an early-exit at the beginning. > > void BindingSecurity::wrapperCreationSecurityCheck(...) { > if (crossContextException.IsEmpty()) > return; > > ExceptionState exceptionState(...); > ... > } > > However, now I think that it's better to do this early-exit on the call site. See my reply at: > https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou... > > Given that we do the early-exit on the call site, we can write: > > void BindingSecurity::wrapperCreationSecurityCheck(...) { > DCHECK(!crossContextException.IsEmpty()); > > ... > } > > And then, I think that the expected behavior of this function will be "rethrow an exception", and I'd prefer "rethrowCrossContextException" or something. It's a bit strange that fooCheck() returns void. Based on my reply on your earlier comment, I was thinking this could be 2 functions instead: bool BindingSecurity::checkCreationContextAccess(creationContext, type) { if (type == V8Location) return true; frame = toLocalFrameIfNotDetached(creationContext) if (!frame) { return shouldAllowAccessToDetachedWindow(...); // could throw security error } else { return shouldAllowAccessToFrame(...); // could throw security error } } void BindingSecurity::rethrowCrossContextException(crossContextException, type, creationContext) { if (type == V8Location) { // convert to security error and throw } else { // rethrow crossContextException }
See my reply to the comment.
I think that what we need to do here is:
1. Check the origins.
1-a. If it's a Location, OK
1-b. Or, if it's same origin, OK
1-c. Otherwise, it's cross origin. NG.
Abort the wrapper creation.
2. Enter into the creation context.
3. Create a wrapper object.
4. Exit from the creation context.
5. Rethrow an exception if any.
1. must be before 3. and 5. must be after 3.
https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right):
https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:165: if
(m_tryCatch.HasCaught())
On 2017/04/03 15:20:54, adithyas wrote:
> On 2017/04/03 at 08:29:25, Yuki wrote:
> > On 2017/03/31 17:49:28, adithyas wrote:
> > > On 2017/03/31 at 09:49:37, Yuki wrote:
> > > > You've reset m_tryCatch on line 160. This is meaningless, isn't it?
> > >
> > > Hmm, does Reset() completely disable the TryCatch? securityCheck can throw
> an
> > > exception, and if it does throw an exception, the tryCatch would catch it
> and
> > > HasCaught() would return true right?
> >
> > Ah, now I see the point. Then, I'd prefer an early-exit here, too.
> >
> > if (!m_didEnterContext) {
> > m_tryCatch.ReThrow();
> > return;
> > }
> > m_context->Exit();
> >
> > if (!m_tryCatch.HasCaught())
> > return;
> >
> > // A cross-context exception exists, so rethrow it.
> > verifyContextAccessAndHandleCrossContextException();
> > m_tryCatch.ReThrow();
> >
> > Given the above logic, I'd feel that
> > rethrowCrossContextException();
> > m_tryCatch.ReThrow();
> > would be more straightforward because "(re)throw an exception" is the
expected
> behavior. The following m_tryCatch.ReThrow() makes sense at a glance.
>
> I think verifyContextAccessAndHandleCrossContextException needs to be called
> regardless of whether a cross context exception is thrown or not. The security
> checks (shouldAllowAccessToFrame/DetachedWindow) inside the method needs to be
> run regardless of whether a cross context exception was thrown.
>
> I could maybe split it up into two methods: verifyContextAccess and
> rethrowCrossContextException
>
> if (!m_didEnterContext) {
> m_tryCatch.ReThrow();
> return;
> }
> m_context->Exit();
>
> crossContextException = m_tryCatch.Exception();
> m_tryCatch.Reset();
>
> if (!verifyContextAccess()) { // verifyContextAccess throws a security error
> when it returns false
> m_tryCatch.ReThrow();
> return;
> }
>
> if (crossContextException.Empty())
> return;
>
> rethrowCrossContextException(crossContextException);
> m_tryCatch.ReThrow();
>
> WDYT?
I'm getting better understanding. The original implementation was correct about
when to perform the security check.
It's too late to perform the security check in the destructor. We need to
perform the security check *BEFORE* we're going to create a new wrapper object.
Otherwise, there may be a security leak. On this point, the original
implementation was correct. However, when the security check failed, we
shouldn't even try to create a new wrapper object (the original implementation
ignores the failure of the security check, I don't like it. We should abort the
wrapper creation in that case).
Ok sounds good, I wasn't sure about whether aborting before creating the wrapper was correct behaviour, but it makes sense. I've updated the CL. Another thing I'm not sure about is what to do with cross context exceptions thrown when creating non-Location wrappers. The previous implementation would only convert cross context exceptions to SecurityError for Location. For non Location wrapper creation, we just rethrow the exception as is without converting. I've maintained the same behaviour in the new implementation, do you think that's correct?
LGTM in general. On 2017/04/05 20:50:08, adithyas wrote: > Ok sounds good, I wasn't sure about whether aborting before creating the wrapper > was correct behaviour, but it makes sense. I've updated the CL. The current CL doesn't support "aborting", but I'm okay to add it in a follow-up CL because the original implementation (wrongly) didn't it. But I think that it's correct to abort. > Another thing I'm not sure about is what to do with cross context exceptions > thrown when creating non-Location wrappers. The previous implementation would > only convert cross context exceptions to SecurityError for Location. For non > Location wrapper creation, we just rethrow the exception as is without > converting. I've maintained the same behaviour in the new implementation, do you > think that's correct? I think that it's NOT correct, but acceptable. The ideal behavior is to convert an exception in the creation context to the one in the current context without losing any information (exception type, line number, function name, etc.). But it wouldn't be easy. So, I'm fine to keep the same behavior so far. Probably, we could "clone" an exception object in the current context (if V8 API supprots it). Otherwise, it may not be so easy. https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right): https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:123: // throws a SecurityError if it doesn't have access. Probably it's good to write that these two functions (including rethrowCrossContextException) are designed to be called only from V8WrapperInstantiationScope. https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:124: static bool shouldEnterCreationContext(v8::Isolate*, Can we follow the existing convention like the following? shouldAllowAccessToCreationContext(...) because this function behaves mostly in the same way as other shouldAllowAccessTo family. https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp (right): https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp:44: V8WrapperInstantiationScope scope(creationContext, isolate, type); You're not aborting the wrapper creation. We should have the following here. if (|scope| failed access check (i.e. |scope| threw an exception)) return v8::Local<v8::Object>(); In general, we shouldn't continue to run when an exception is thrown. Almost all V8 APIs fail while an exception is being thrown. https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:144: m_context = v8::Local<v8::Context>::New(isolate, contextForWrapper); Why do we need to call v8::Local::New? I thought that m_context = contextForWrapper; is enough. https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:164: v8::Isolate* isolate = m_context->GetIsolate(); Put this line just before its use, or you may not need this variable.
The CQ bit was checked by adithyas@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right): https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:123: // throws a SecurityError if it doesn't have access. On 2017/04/06 at 08:26:15, Yuki wrote: > Probably it's good to write that these two functions (including rethrowCrossContextException) are designed to be called only from V8WrapperInstantiationScope. Done. https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:124: static bool shouldEnterCreationContext(v8::Isolate*, On 2017/04/06 at 08:26:15, Yuki wrote: > Can we follow the existing convention like the following? > shouldAllowAccessToCreationContext(...) > because this function behaves mostly in the same way as other shouldAllowAccessTo family. Done. https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp (right): https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp:44: V8WrapperInstantiationScope scope(creationContext, isolate, type); On 2017/04/06 at 08:26:15, Yuki wrote: > You're not aborting the wrapper creation. > > We should have the following here. > > if (|scope| failed access check (i.e. |scope| threw an exception)) > return v8::Local<v8::Object>(); > > In general, we shouldn't continue to run when an exception is thrown. Almost all V8 APIs fail while an exception is being thrown. Ok, done. https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:144: m_context = v8::Local<v8::Context>::New(isolate, contextForWrapper); On 2017/04/06 at 08:26:16, Yuki wrote: > Why do we need to call v8::Local::New? > I thought that > m_context = contextForWrapper; > is enough. This is how it was written when V8WrapperInstantiationScope was first written, I'm not really sure why. I've updated it to m_context = contextForWrapper. https://codereview.chromium.org/2745313003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:164: v8::Isolate* isolate = m_context->GetIsolate(); On 2017/04/06 at 08:26:15, Yuki wrote: > Put this line just before its use, or you may not need this variable. Fixed.
Mostly looks good. https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:284: return !currentWorld.isMainWorld() || I'm just curious but do you know why we need the isMainWorld check? https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp (left): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp:119: // reach this code path. Should be generalized. I think we discussed this before but this comment was wrong, right? This code path is for objects other than Location. That's why you removed the comment from this CL, right? https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp:138: CHECK_EQ(SecurityError, exceptionState.code()); Can we keep this CHECK? https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:169: v8::Isolate* isolate = m_context->GetIsolate(); Would you add a comment on why we need this complex conversion logic? https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.cpp (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.cpp:18: s_securityCheck = func; Add DCHECK(!s_securityCheck). https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.cpp:23: s_rethrowException = func; Ditto. https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.h (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.h:16: class CORE_EXPORT WrapperCreationSecurityCheck { Add a class-level comment. https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.h:29: static void setRethrowExceptionFunction(RethrowExceptionFunction); Is there any reason you adopted the function pointer injection pattern instead of introducing BindingSecurityBase (so that BindingSecurity can override the behavior of the methods)? https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/CoreInitializer.cpp (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/CoreInitializer.cpp:147: BindingSecurity::initWrapperCreationSecurityCheck(); I'd prefer moving this to V8Initializer.
https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:284: return !currentWorld.isMainWorld() || On 2017/04/07 04:23:36, haraken wrote: > > I'm just curious but do you know why we need the isMainWorld check? IIUC, - content scripts are running in the main world. - other extensions are running in an isolated world, and we don't need to care about cross origin things because it must be all the same origins. - workers are running in a worker world, and we don't need to care about cross origin things because it must be all the same origins. So, my understanding is that this is just an optimization and we can remove this if we want. However, I think that we'd like to be careful if we will remove it, and I doubt if there are enough origin-related scenarios in extension tests. https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp (left): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp:138: CHECK_EQ(SecurityError, exceptionState.code()); On 2017/04/07 04:23:36, haraken wrote: > > Can we keep this CHECK? IIUC, shouldAllowAccessToFrame only throws a SecurityError if it throws. If we'd like to have this kind of checks, then I'd like to have it as DCHECK in canAccessFrame() in BindingSecurity.cpp. https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.h (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.h:29: static void setRethrowExceptionFunction(RethrowExceptionFunction); On 2017/04/07 04:23:36, haraken wrote: > > Is there any reason you adopted the function pointer injection pattern instead > of introducing BindingSecurityBase (so that BindingSecurity can override the > behavior of the methods)? Just FYI, BindingSecurity is STATIC_ONLY so far.
https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:284: return !currentWorld.isMainWorld() || On 2017/04/07 07:42:14, Yuki wrote: > On 2017/04/07 04:23:36, haraken wrote: > > > > I'm just curious but do you know why we need the isMainWorld check? > > IIUC, > > - content scripts are running in the main world. > > - other extensions are running in an isolated world, and we don't need to care > about cross origin things because it must be all the same origins. > > - workers are running in a worker world, and we don't need to care about cross > origin things because it must be all the same origins. > > So, my understanding is that this is just an optimization and we can remove this > if we want. However, I think that we'd like to be careful if we will remove it, > and I doubt if there are enough origin-related scenarios in extension tests. Extensions == content scripts. Content scripts are running on an isolated world. Also the content scripts are not allowed to touch cross-origin things, if I'm understanding correctly. So I'm wondering why we should have this isMainWorld() check. https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.h (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.h:29: static void setRethrowExceptionFunction(RethrowExceptionFunction); On 2017/04/07 07:42:14, Yuki wrote: > On 2017/04/07 04:23:36, haraken wrote: > > > > Is there any reason you adopted the function pointer injection pattern instead > > of introducing BindingSecurityBase (so that BindingSecurity can override the > > behavior of the methods)? > > Just FYI, BindingSecurity is STATIC_ONLY so far. Makes sense.
https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp (left): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp:119: // reach this code path. Should be generalized. On 2017/04/07 at 04:23:36, haraken wrote: > I think we discussed this before but this comment was wrong, right? This code path is for objects other than Location. That's why you removed the comment from this CL, right? Yes, that's right. https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h:169: v8::Isolate* isolate = m_context->GetIsolate(); On 2017/04/07 at 04:23:36, haraken wrote: > Would you add a comment on why we need this complex conversion logic? Ok done. https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.cpp (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.cpp:18: s_securityCheck = func; On 2017/04/07 at 04:23:36, haraken wrote: > Add DCHECK(!s_securityCheck). Added DCHECK here and below. https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.h (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WrapperCreationSecurityCheck.h:16: class CORE_EXPORT WrapperCreationSecurityCheck { On 2017/04/07 at 04:23:36, haraken wrote: > Add a class-level comment. Done. https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/CoreInitializer.cpp (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/CoreInitializer.cpp:147: BindingSecurity::initWrapperCreationSecurityCheck(); On 2017/04/07 at 04:23:36, haraken wrote: > I'd prefer moving this to V8Initializer. Would V8Initializer::initializeMainThread() work? I can't put it in V8Initializer::initializeCommon, because it could get run more than once (main thread + worker threads).
LGTM https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/CoreInitializer.cpp (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/CoreInitializer.cpp:147: BindingSecurity::initWrapperCreationSecurityCheck(); On 2017/04/07 18:42:53, adithyas wrote: > On 2017/04/07 at 04:23:36, haraken wrote: > > I'd prefer moving this to V8Initializer. > > Would V8Initializer::initializeMainThread() work? I can't put it in > V8Initializer::initializeCommon, because it could get run more than once (main > thread + worker threads). Yes, V8Initializer::initializeMainThread sounds nice. https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:258: v8::Isolate* isolate, You don't need to pass in an Isolate*, since you can get it by creationContext->GetIsolate(). https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:284: return !currentWorld.isMainWorld() || I'd like to understand what the isMainWorld check is for. Would you mind investigating a git blame history a bit? https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:290: v8::Isolate* isolate, Ditto. Remove the Isolate* parameter. https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:296: type->interfaceName); It looks redundant to create an ExceptionState. I think you can just make BindingSecurity::rethrowCrossContextException a V8 exception. You can address it in a follow-up CL.
https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:296: type->interfaceName); On 2017/04/09 15:29:00, haraken wrote: > > It looks redundant to create an ExceptionState. I think you can just make > BindingSecurity::rethrowCrossContextException a V8 exception. You can address it > in a follow-up CL. Yes, it's somewhat redundant, but ExceptionState supports creating an exception message (in a somewhat unified format). I think that we want to take its advantage. In general, it's not on a performance-sensitive path if it's throwing an exception.
https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/CoreInitializer.cpp (right): https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/CoreInitializer.cpp:147: BindingSecurity::initWrapperCreationSecurityCheck(); On 2017/04/09 at 15:28:59, haraken wrote: > On 2017/04/07 18:42:53, adithyas wrote: > > On 2017/04/07 at 04:23:36, haraken wrote: > > > I'd prefer moving this to V8Initializer. > > > > Would V8Initializer::initializeMainThread() work? I can't put it in > > V8Initializer::initializeCommon, because it could get run more than once (main > > thread + worker threads). > > Yes, V8Initializer::initializeMainThread sounds nice. Done. https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:258: v8::Isolate* isolate, On 2017/04/09 at 15:28:59, haraken wrote: > You don't need to pass in an Isolate*, since you can get it by creationContext->GetIsolate(). Fixed, here and everywhere else. https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:284: return !currentWorld.isMainWorld() || On 2017/04/09 at 15:29:00, haraken wrote: > I'd like to understand what the isMainWorld check is for. Would you mind investigating a git blame history a bit? This is the CL that added the check in: https://codereview.chromium.org/1323453005#msg15 jochen@'s comment explains it: "the security token of the isolated world is chrome-extension://abcd..., so the security check against the Frame's security origin will always fail. so for isolated worlds, all we check is that the access is within the same isolated world."
The CQ bit was checked by adithyas@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.
This CL LGTM, feel free to land this :) On 2017/04/10 18:02:00, adithyas wrote: > https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/CoreInitializer.cpp (right): > > https://codereview.chromium.org/2745313003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/CoreInitializer.cpp:147: > BindingSecurity::initWrapperCreationSecurityCheck(); > On 2017/04/09 at 15:28:59, haraken wrote: > > On 2017/04/07 18:42:53, adithyas wrote: > > > On 2017/04/07 at 04:23:36, haraken wrote: > > > > I'd prefer moving this to V8Initializer. > > > > > > Would V8Initializer::initializeMainThread() work? I can't put it in > > > V8Initializer::initializeCommon, because it could get run more than once > (main > > > thread + worker threads). > > > > Yes, V8Initializer::initializeMainThread sounds nice. > > Done. > > https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): > > https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:258: v8::Isolate* > isolate, > On 2017/04/09 at 15:28:59, haraken wrote: > > You don't need to pass in an Isolate*, since you can get it by > creationContext->GetIsolate(). > > Fixed, here and everywhere else. > > https://codereview.chromium.org/2745313003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:284: return > !currentWorld.isMainWorld() || > On 2017/04/09 at 15:29:00, haraken wrote: > > I'd like to understand what the isMainWorld check is for. Would you mind > investigating a git blame history a bit? > > This is the CL that added the check in: > https://codereview.chromium.org/1323453005#msg15 > > jochen@'s comment explains it: "the security token of the isolated world is > chrome-extension://abcd..., so the security check against the Frame's security > origin will always fail. > > so for isolated worlds, all we check is that the access is within the same > isolated world." Hmm. Does it mean that we're allowing an isolated world to access cross-origin iframes sometimes (i.e., when the cross context is used when we instantiate a wrapper)?
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2745313003/#ps240001 (title: "Move init to V8Initializer")
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": 240001, "attempt_start_ts": 1491920630975980,
"parent_rev": "c7369bf0679442fa79338ab4502ef836b2244c03", "commit_rev":
"3f7eac384ee02371be4972ba0647082bf6599eb1"}
Message was sent while issue was closed.
Description was changed from ========== Move securityCheck out of V8WrapperInstantiationScope Moving V8DOMWrapper.h/cpp to platform/bindings is blocked by V8WrapperInstantiationScope::securityCheck and V8WrapperInstantiationScope::convertException. Both of these methods use toLocalFrame/toDOMWindow, and cannot be moved to platform/. This CL moves the security check into WrapperCreationSecurityCheck. WrapperCreationSecurityCheck only holds a function pointer and will be moved into platform/bindings. The implementation of the security check is moved to BindingSecurity (in bindings/core), and the function pointer is set to point to the implementation inside core/. BUG=682322 ========== to ========== Move securityCheck out of V8WrapperInstantiationScope Moving V8DOMWrapper.h/cpp to platform/bindings is blocked by V8WrapperInstantiationScope::securityCheck and V8WrapperInstantiationScope::convertException. Both of these methods use toLocalFrame/toDOMWindow, and cannot be moved to platform/. This CL moves the security check into WrapperCreationSecurityCheck. WrapperCreationSecurityCheck only holds a function pointer and will be moved into platform/bindings. The implementation of the security check is moved to BindingSecurity (in bindings/core), and the function pointer is set to point to the implementation inside core/. BUG=682322 Review-Url: https://codereview.chromium.org/2745313003 Cr-Commit-Position: refs/heads/master@{#463621} Committed: https://chromium.googlesource.com/chromium/src/+/3f7eac384ee02371be4972ba0647... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/3f7eac384ee02371be4972ba0647...
Message was sent while issue was closed.
On 2017/04/11 at 00:29:12, haraken wrote: > > Hmm. Does it mean that we're allowing an isolated world to access cross-origin iframes sometimes (i.e., when the cross context is used when we instantiate a wrapper)? I think so, but it only has access to wrappers in the same isolated world. There's a CHECK_EQ to see if the creationContext and currentContext are in the same world.
Message was sent while issue was closed.
On 2017/04/11 14:50:35, adithyas wrote: > On 2017/04/11 at 00:29:12, haraken wrote: > > > > Hmm. Does it mean that we're allowing an isolated world to access cross-origin > iframes sometimes (i.e., when the cross context is used when we instantiate a > wrapper)? > > I think so, but it only has access to wrappers in the same isolated world. > There's a CHECK_EQ to see if the creationContext and currentContext are in the > same world. Thanks. It's not nice that there's a way for a Chrome extension to touch things in a cross-origin iframe, although I'm not sure how dangerous it will be.
Message was sent while issue was closed.
On 2017/04/11 14:50:35, adithyas wrote: > On 2017/04/11 at 00:29:12, haraken wrote: > > > > Hmm. Does it mean that we're allowing an isolated world to access cross-origin > iframes sometimes (i.e., when the cross context is used when we instantiate a > wrapper)? > > I think so, but it only has access to wrappers in the same isolated world. > There's a CHECK_EQ to see if the creationContext and currentContext are in the > same world. The same world doesn't mean the same origin. It can be a cross origin. If a content script is allowed to run on "foo.org" and the main page is "foo.org", it shouldn't be okay that the content script touches DOM objects in an iframe of "bar.org". I don't yet fully understand the situation, though.
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2815373002/ by adithyas@chromium.org. The reason for reverting is: https://bugs.chromium.org/p/chromium/issues/detail?id=711373 The DCHECK in ScriptWrappable::Wrap indicates that we do not properly handle an empty wrapper being returned by V8DOMWrapper::CreateWrapper. So a failed security check would cause this crash. A potential short-term fix would be to just return a wrapper regardless of whether the security check passed or failed (which is what the previous implementation did)?.
Requesting another review of this CL. I've changed it to no longer return an empty value in CreateWrapper if the security check fails. This is consistent with old behavior and should not cause any regressions.
LGTM
This CL LGTM, but remember that it's wrong to continue to run having an exception thrown. We need to address the issue separately. https://codereview.chromium.org/2745313003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp (right): https://codereview.chromium.org/2745313003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp:45: if (scope.AccessCheckFailed()) { It's okay that this CL is not focusing on this issue, but this is still problematic. It's wrong to continue to run having an exception thrown. Ideally, the DCHECK in ToV8() should be fixed instead. At least, we should have a TODO comment here.
https://codereview.chromium.org/2745313003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp (right): https://codereview.chromium.org/2745313003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp:45: if (scope.AccessCheckFailed()) { On 2017/04/20 at 09:32:48, Yuki wrote: > It's okay that this CL is not focusing on this issue, but this is still problematic. It's wrong to continue to run having an exception thrown. Ideally, the DCHECK in ToV8() should be fixed instead. > > At least, we should have a TODO comment here. Added in a TODO, I'll try fixing this in a follow up CL.
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2745313003/#ps280001 (title: "Add TODO")
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 adithyas@chromium.org
The CQ bit was checked by adithyas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1492699998969300,
"parent_rev": "6125669b9b361e721383f149e074355b7c696784", "commit_rev":
"32ce98203b3580adb4e8553c34cbb677a0fa6203"}
Message was sent while issue was closed.
Description was changed from ========== Move securityCheck out of V8WrapperInstantiationScope Moving V8DOMWrapper.h/cpp to platform/bindings is blocked by V8WrapperInstantiationScope::securityCheck and V8WrapperInstantiationScope::convertException. Both of these methods use toLocalFrame/toDOMWindow, and cannot be moved to platform/. This CL moves the security check into WrapperCreationSecurityCheck. WrapperCreationSecurityCheck only holds a function pointer and will be moved into platform/bindings. The implementation of the security check is moved to BindingSecurity (in bindings/core), and the function pointer is set to point to the implementation inside core/. BUG=682322 Review-Url: https://codereview.chromium.org/2745313003 Cr-Commit-Position: refs/heads/master@{#463621} Committed: https://chromium.googlesource.com/chromium/src/+/3f7eac384ee02371be4972ba0647... ========== to ========== Move securityCheck out of V8WrapperInstantiationScope Moving V8DOMWrapper.h/cpp to platform/bindings is blocked by V8WrapperInstantiationScope::securityCheck and V8WrapperInstantiationScope::convertException. Both of these methods use toLocalFrame/toDOMWindow, and cannot be moved to platform/. This CL moves the security check into WrapperCreationSecurityCheck. WrapperCreationSecurityCheck only holds a function pointer and will be moved into platform/bindings. The implementation of the security check is moved to BindingSecurity (in bindings/core), and the function pointer is set to point to the implementation inside core/. BUG=682322 Review-Url: https://codereview.chromium.org/2745313003 Cr-Original-Commit-Position: refs/heads/master@{#463621} Committed: https://chromium.googlesource.com/chromium/src/+/3f7eac384ee02371be4972ba0647... Review-Url: https://codereview.chromium.org/2745313003 Cr-Commit-Position: refs/heads/master@{#466029} Committed: https://chromium.googlesource.com/chromium/src/+/32ce98203b3580adb4e8553c34cb... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/32ce98203b3580adb4e8553c34cb... |
