Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1262353002:
Add access checks to V8WrapperInstationScope. (Closed)
DescriptionAdd access checks to V8WrapperInstationScope.
We skip those access checks when dealing with Location, as V8Location has its own checks in place according to the Location's cross-origin policy (https://html.spec.whatwg.org/multipage/browsers.html#security-location).
BUG=455160
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201908
Patch Set 1 #
Total comments: 7
Patch Set 2 : Update #Patch Set 3 : Update #
Total comments: 2
Patch Set 4 : Update #
Total comments: 1
Messages
Total messages: 30 (4 generated)
epertoso@chromium.org changed reviewers: + haraken@chromium.org
haraken@chromium.org changed reviewers: + jochen@chromium.org
Add more explanations to the CL description. https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... File Source/bindings/core/v8/V8DOMWrapper.cpp (right): https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... Source/bindings/core/v8/V8DOMWrapper.cpp:76: bool withSecurityCheck = !type->equals(&V8Window::wrapperTypeInfo) && !type->equals(&V8Location::wrapperTypeInfo); I guess this might regress performance. As commented in V8DOMWrapper.h, I wonder why we want to enable the security check only for location. https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... File Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... Source/bindings/core/v8/V8DOMWrapper.h:124: if (withSecurityCheck) { Would you help me understand why we want to enable the check only for Location? Given that this is a slow path, I guess it would be better to enable the security check for all objects. https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... Source/bindings/core/v8/V8DOMWrapper.h:127: if (!m_context.IsEmpty()) { m_context shouldn't be empty here. https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... Source/bindings/core/v8/V8DOMWrapper.h:129: RELEASE_ASSERT(!frame || BindingSecurity::shouldAllowAccessToFrame(isolate, frame, DoNotReportSecurityError)); Just to confirm: This will allow a wrapper creation on a cross-origin frame that is already detached, but is it an expected behavior? c.f., shouldAllowAccessToFrame() returns false for a null frame.
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... File Source/bindings/core/v8/V8DOMWrapper.cpp (right): https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... Source/bindings/core/v8/V8DOMWrapper.cpp:76: bool withSecurityCheck = !type->equals(&V8Window::wrapperTypeInfo) && !type->equals(&V8Location::wrapperTypeInfo); On 2015/07/30 10:43:36, haraken wrote: > > I guess this might regress performance. As commented in V8DOMWrapper.h, I wonder > why we want to enable the security check only for location. It seems that the CL is enabling the security check except for Location. By the way, we never come to here with Window. https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... File Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... Source/bindings/core/v8/V8DOMWrapper.h:111: V8WrapperInstantiationScope(v8::Local<v8::Object> creationContext, v8::Isolate* isolate, bool withSecurityCheck = true) The style guide recommends an enum value. https://www.chromium.org/blink/coding-style#TOC-Names See 10th rule.
shiino-san: Do you have any suggestion about benchmarks we should try? https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... File Source/bindings/core/v8/V8DOMWrapper.cpp (right): https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... Source/bindings/core/v8/V8DOMWrapper.cpp:76: bool withSecurityCheck = !type->equals(&V8Window::wrapperTypeInfo) && !type->equals(&V8Location::wrapperTypeInfo); On 2015/07/30 11:00:54, Yuki wrote: > On 2015/07/30 10:43:36, haraken wrote: > > > > I guess this might regress performance. As commented in V8DOMWrapper.h, I > wonder > > why we want to enable the security check only for location. > > It seems that the CL is enabling the security check except for Location. > > By the way, we never come to here with Window. Thanks, that totally makes sense to me. - Remove the type->equals(&V8Window::wrapperTypeInfo) check. - Add ASSERT(!type->equals(&V8Window::wrapperTypeInfo)). - Add a comment why Location is special-cased.
On 2015/07/30 at 11:08:06, haraken wrote: > shiino-san: Do you have any suggestion about benchmarks we should try? > > https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... > File Source/bindings/core/v8/V8DOMWrapper.cpp (right): > > https://codereview.chromium.org/1262353002/diff/1/Source/bindings/core/v8/V8D... > Source/bindings/core/v8/V8DOMWrapper.cpp:76: bool withSecurityCheck = !type->equals(&V8Window::wrapperTypeInfo) && !type->equals(&V8Location::wrapperTypeInfo); > On 2015/07/30 11:00:54, Yuki wrote: > > On 2015/07/30 10:43:36, haraken wrote: > > > > > > I guess this might regress performance. As commented in V8DOMWrapper.h, I > > wonder > > > why we want to enable the security check only for location. > > > > It seems that the CL is enabling the security check except for Location. > > > > By the way, we never come to here with Window. > > Thanks, that totally makes sense to me. > > - Remove the type->equals(&V8Window::wrapperTypeInfo) check. > - Add ASSERT(!type->equals(&V8Window::wrapperTypeInfo)). > - Add a comment why Location is special-cased. Done.
> On 2015/07/30 at 11:08:06, haraken wrote: > > shiino-san: Do you have any suggestion about benchmarks we should try? I know little about benchmarks. I think you better know. What came up in my mind is dromaeo tests. Probably "DOM Modification" test is creating many DOM wrappers. "DOM Traversal" test looks good, too, because traversing DOM trees require to create DOM Wrappers for every node. Since I'm not sure which is best, I'd run all dromaeo DOM tests.
On 2015/07/30 13:33:47, Yuki wrote: > > On 2015/07/30 at 11:08:06, haraken wrote: > > > shiino-san: Do you have any suggestion about benchmarks we should try? > > I know little about benchmarks. I think you better know. > > What came up in my mind is dromaeo tests. Probably "DOM Modification" test is > creating many DOM wrappers. "DOM Traversal" test looks good, too, because > traversing DOM trees require to create DOM Wrappers for every node. Since I'm > not sure which is best, I'd run all dromaeo DOM tests. Also PerformanceTests/Parser/query-selector-all-* would be useful since it creates a lot of wrappers of NodeLists. I think query-selector-all-* was the benchmark that regressed when shiino-san added a RELEASE_ASSERT to the wrapper-creation path.
On 2015/07/30 at 13:36:51, haraken wrote: > On 2015/07/30 13:33:47, Yuki wrote: > > > On 2015/07/30 at 11:08:06, haraken wrote: > > > > shiino-san: Do you have any suggestion about benchmarks we should try? > > > > I know little about benchmarks. I think you better know. > > > > What came up in my mind is dromaeo tests. Probably "DOM Modification" test is > > creating many DOM wrappers. "DOM Traversal" test looks good, too, because > > traversing DOM trees require to create DOM Wrappers for every node. Since I'm > > not sure which is best, I'd run all dromaeo DOM tests. > > Also PerformanceTests/Parser/query-selector-all-* would be useful since it creates a lot of wrappers of NodeLists. I think query-selector-all-* was the benchmark that regressed when shiino-san added a RELEASE_ASSERT to the wrapper-creation path. I've run the parser benchmarks (on trybot-linux: http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-08...) and dromaeo.domcoremodify (on trybot-all-linux: http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-08...). Should I collect more data?
On 2015/08/04 08:13:17, epertoso wrote: > On 2015/07/30 at 13:36:51, haraken wrote: > > On 2015/07/30 13:33:47, Yuki wrote: > > > > On 2015/07/30 at 11:08:06, haraken wrote: > > > > > shiino-san: Do you have any suggestion about benchmarks we should try? > > > > > > I know little about benchmarks. I think you better know. > > > > > > What came up in my mind is dromaeo tests. Probably "DOM Modification" test > is > > > creating many DOM wrappers. "DOM Traversal" test looks good, too, because > > > traversing DOM trees require to create DOM Wrappers for every node. Since > I'm > > > not sure which is best, I'd run all dromaeo DOM tests. > > > > Also PerformanceTests/Parser/query-selector-all-* would be useful since it > creates a lot of wrappers of NodeLists. I think query-selector-all-* was the > benchmark that regressed when shiino-san added a RELEASE_ASSERT to the > wrapper-creation path. > > I've run the parser benchmarks (on trybot-linux: > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-08...) > and dromaeo.domcoremodify (on trybot-all-linux: > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-08...). > > Should I collect more data? It seems that multiple benchmarks in query-selector-* are regressing. Can you run Parser/query-selector-last.html multiple times in your local machine and see if the observed regression is a real one or not?
On 2015/08/04 at 08:15:42, haraken wrote: > On 2015/08/04 08:13:17, epertoso wrote: > > On 2015/07/30 at 13:36:51, haraken wrote: > > > On 2015/07/30 13:33:47, Yuki wrote: > > > > > On 2015/07/30 at 11:08:06, haraken wrote: > > > > > > shiino-san: Do you have any suggestion about benchmarks we should try? > > > > > > > > I know little about benchmarks. I think you better know. > > > > > > > > What came up in my mind is dromaeo tests. Probably "DOM Modification" test > > is > > > > creating many DOM wrappers. "DOM Traversal" test looks good, too, because > > > > traversing DOM trees require to create DOM Wrappers for every node. Since > > I'm > > > > not sure which is best, I'd run all dromaeo DOM tests. > > > > > > Also PerformanceTests/Parser/query-selector-all-* would be useful since it > > creates a lot of wrappers of NodeLists. I think query-selector-all-* was the > > benchmark that regressed when shiino-san added a RELEASE_ASSERT to the > > wrapper-creation path. > > > > I've run the parser benchmarks (on trybot-linux: > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-08...) > > and dromaeo.domcoremodify (on trybot-all-linux: > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-08...). > > > > Should I collect more data? > > It seems that multiple benchmarks in query-selector-* are regressing. Can you run Parser/query-selector-last.html multiple times in your local machine and see if the observed regression is a real one or not? Sorry for the delay. I ran it with --pageset-repeat set to 50, and I got: ToT: 976.01 ± 1.04% With Patch: 923.90 ± 1.18% 5.34% Worse. Seems like a real one.
i think that's an acceptable regression.
On 2015/08/12 16:31:16, jochen (ooo) wrote: > i think that's an acceptable regression. That sounds like a significant regression to me. We should try more to reduce the regression. a) I'm not sure if you want to do this, but you can use RELEASE_ASSERT_WITH_SECURITY_IMPLICATION instead. It is not enabled on production builds so there is no risk of regressing the performance. We use the macro for ScriptWrappable's asserts, PartitionAlloc etc. b) toFrameIfNotDetached and shouldAllowAccessToFrame are very slow. Optimize the call path or reduce the number we need to call in those methods. For example, you can avoid calling shouldAllowAccessToFrame for the same pair of contexts many times by caching the contexts you've verified at the last call (and invalidate the contexts when the associated frame navigates).
On 2015/08/12 at 23:31:40, haraken wrote: > On 2015/08/12 16:31:16, jochen (ooo) wrote: > > i think that's an acceptable regression. > > That sounds like a significant regression to me. We should try more to reduce the regression. > > a) I'm not sure if you want to do this, but you can use RELEASE_ASSERT_WITH_SECURITY_IMPLICATION instead. It is not enabled on production builds so there is no risk of regressing the performance. We use the macro for ScriptWrappable's asserts, PartitionAlloc etc. > > b) toFrameIfNotDetached and shouldAllowAccessToFrame are very slow. Optimize the call path or reduce the number we need to call in those methods. For example, you can avoid calling shouldAllowAccessToFrame for the same pair of contexts many times by caching the contexts you've verified at the last call (and invalidate the contexts when the associated frame navigates). Looks like that with b) the performances get a bit better: http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-08... Before I go ahead, I have two questions: - is it OK to cache the contexts by raw pointers? - is V8DOMWrapper::createWrapper code always going to be called from the same thread?
On 2015/08/20 10:54:41, epertoso wrote: > On 2015/08/12 at 23:31:40, haraken wrote: > > On 2015/08/12 16:31:16, jochen (ooo) wrote: > > > i think that's an acceptable regression. > > > > That sounds like a significant regression to me. We should try more to reduce > the regression. > > > > a) I'm not sure if you want to do this, but you can use > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION instead. It is not enabled on > production builds so there is no risk of regressing the performance. We use the > macro for ScriptWrappable's asserts, PartitionAlloc etc. > > > > b) toFrameIfNotDetached and shouldAllowAccessToFrame are very slow. Optimize > the call path or reduce the number we need to call in those methods. For > example, you can avoid calling shouldAllowAccessToFrame for the same pair of > contexts many times by caching the contexts you've verified at the last call > (and invalidate the contexts when the associated frame navigates). > > Looks like that with b) the performances get a bit better: > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-08... > > Before I go ahead, I have two questions: > - is it OK to cache the contexts by raw pointers? It is unsafe. Can we use a weak persistent handle or something like that? Folks around you will be more familiar with it :) > - is V8DOMWrapper::createWrapper code always going to be called from the same > thread? No, V8DOMWrapper::createWrapper can be called by non-main threads.
On 2015/08/20 at 11:44:39, haraken wrote: > On 2015/08/20 10:54:41, epertoso wrote: > > On 2015/08/12 at 23:31:40, haraken wrote: > > > On 2015/08/12 16:31:16, jochen (ooo) wrote: > > > > i think that's an acceptable regression. > > > > > > That sounds like a significant regression to me. We should try more to reduce > > the regression. > > > > > > a) I'm not sure if you want to do this, but you can use > > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION instead. It is not enabled on > > production builds so there is no risk of regressing the performance. We use the > > macro for ScriptWrappable's asserts, PartitionAlloc etc. > > > > > > b) toFrameIfNotDetached and shouldAllowAccessToFrame are very slow. Optimize > > the call path or reduce the number we need to call in those methods. For > > example, you can avoid calling shouldAllowAccessToFrame for the same pair of > > contexts many times by caching the contexts you've verified at the last call > > (and invalidate the contexts when the associated frame navigates). > > > > Looks like that with b) the performances get a bit better: > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-08... > > > > Before I go ahead, I have two questions: > > - is it OK to cache the contexts by raw pointers? > > It is unsafe. Can we use a weak persistent handle or something like that? Folks around you will be more familiar with it :) > > > - is V8DOMWrapper::createWrapper code always going to be called from the same > > thread? > > No, V8DOMWrapper::createWrapper can be called by non-main threads. Further data: our code is never reached during that particular benchmark (query-selector-last.html), rightfully so as there is only one context (so the condition 'contextForWrapper == m_context', which was already in V8WrapperInstantiationScope's constructor, is always true). I would just assume that the different results of this benchmark are noisy.
On 2015/09/08 08:30:20, epertoso wrote: > On 2015/08/20 at 11:44:39, haraken wrote: > > On 2015/08/20 10:54:41, epertoso wrote: > > > On 2015/08/12 at 23:31:40, haraken wrote: > > > > On 2015/08/12 16:31:16, jochen (ooo) wrote: > > > > > i think that's an acceptable regression. > > > > > > > > That sounds like a significant regression to me. We should try more to > reduce > > > the regression. > > > > > > > > a) I'm not sure if you want to do this, but you can use > > > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION instead. It is not enabled on > > > production builds so there is no risk of regressing the performance. We use > the > > > macro for ScriptWrappable's asserts, PartitionAlloc etc. > > > > > > > > b) toFrameIfNotDetached and shouldAllowAccessToFrame are very slow. > Optimize > > > the call path or reduce the number we need to call in those methods. For > > > example, you can avoid calling shouldAllowAccessToFrame for the same pair of > > > contexts many times by caching the contexts you've verified at the last call > > > (and invalidate the contexts when the associated frame navigates). > > > > > > Looks like that with b) the performances get a bit better: > > > > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-08... > > > > > > Before I go ahead, I have two questions: > > > - is it OK to cache the contexts by raw pointers? > > > > It is unsafe. Can we use a weak persistent handle or something like that? > Folks around you will be more familiar with it :) > > > > > - is V8DOMWrapper::createWrapper code always going to be called from the > same > > > thread? > > > > No, V8DOMWrapper::createWrapper can be called by non-main threads. > > Further data: our code is never reached during that particular benchmark > (query-selector-last.html), rightfully so as there is only one context (so the > condition 'contextForWrapper == m_context', which was already in > V8WrapperInstantiationScope's constructor, is always true). I would just assume > that the different results of this benchmark are noisy. Thanks for the clarification! Then it wouldn't make sense to dig into the flakiness. LGTM. https://codereview.chromium.org/1262353002/diff/40001/Source/bindings/core/v8... File Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/1262353002/diff/40001/Source/bindings/core/v8... Source/bindings/core/v8/V8DOMWrapper.h:124: if (withSecurityCheck) { This code should be in a slow path. Can we move this code to a cpp file just in case (to avoid causing a perf regression caused by code addition to a hot call path)?
https://codereview.chromium.org/1262353002/diff/40001/Source/bindings/core/v8... File Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/1262353002/diff/40001/Source/bindings/core/v8... Source/bindings/core/v8/V8DOMWrapper.h:124: if (withSecurityCheck) { On 2015/09/08 at 08:48:07, haraken wrote: > This code should be in a slow path. Can we move this code to a cpp file just in case (to avoid causing a perf regression caused by code addition to a hot call path)? Done.
On 2015/09/08 09:29:36, epertoso wrote: > https://codereview.chromium.org/1262353002/diff/40001/Source/bindings/core/v8... > File Source/bindings/core/v8/V8DOMWrapper.h (right): > > https://codereview.chromium.org/1262353002/diff/40001/Source/bindings/core/v8... > Source/bindings/core/v8/V8DOMWrapper.h:124: if (withSecurityCheck) { > On 2015/09/08 at 08:48:07, haraken wrote: > > This code should be in a slow path. Can we move this code to a cpp file just > in case (to avoid causing a perf regression caused by code addition to a hot > call path)? > > Done. Still LGTM
The CQ bit was checked by epertoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1262353002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1262353002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201908
Message was sent while issue was closed.
https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... File Source/bindings/core/v8/V8DOMWrapper.h (right): https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... Source/bindings/core/v8/V8DOMWrapper.h:124: if (contextForWrapper == m_context) epertoso@ and jochen@: I'm getting a bit confused. In what scenario can info.Holder()->CreationContext() be different from isolate->GetCurrentContext()? Imagine that you have a main frame and an iframe. Imagine that you execute the following code in the main frame. - If you call iframe.contentDocument, in the binding callback for contentDocument, which context do the isolate->GetCurrentContext and the info.Holder()->CreationContext return? (My guess is that both return the iframe's context.) - If you call iframe.contentDocument.createElement(), in the binding callback for createElement(), which context do the isolate->GetCurrentContext and the info.Holder()->CreationContext return? (My guess is that both return the iframe's context.) - In what scenario, do the isolate->GetCurrentContext and the info.Holder()->CreationContext return different contexts?
Message was sent while issue was closed.
On 2015/12/22 at 07:17:51, haraken wrote: > https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... > File Source/bindings/core/v8/V8DOMWrapper.h (right): > > https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... > Source/bindings/core/v8/V8DOMWrapper.h:124: if (contextForWrapper == m_context) > > epertoso@ and jochen@: I'm getting a bit confused. In what scenario can info.Holder()->CreationContext() be different from isolate->GetCurrentContext()? > > Imagine that you have a main frame and an iframe. Imagine that you execute the following code in the main frame. > > - If you call iframe.contentDocument, in the binding callback for contentDocument, which context do the isolate->GetCurrentContext and the info.Holder()->CreationContext return? > > (My guess is that both return the iframe's context.) current context will be the main frame, while creation context will be the iframe > > - If you call iframe.contentDocument.createElement(), in the binding callback for createElement(), which context do the isolate->GetCurrentContext and the info.Holder()->CreationContext return? > > (My guess is that both return the iframe's context.) same as above > > - In what scenario, do the isolate->GetCurrentContext and the info.Holder()->CreationContext return different contexts?
Message was sent while issue was closed.
On 2016/01/07 10:06:07, jochen wrote: > On 2015/12/22 at 07:17:51, haraken wrote: > > > https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... > > File Source/bindings/core/v8/V8DOMWrapper.h (right): > > > > > https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... > > Source/bindings/core/v8/V8DOMWrapper.h:124: if (contextForWrapper == > m_context) > > > > epertoso@ and jochen@: I'm getting a bit confused. In what scenario can > info.Holder()->CreationContext() be different from isolate->GetCurrentContext()? > > > > Imagine that you have a main frame and an iframe. Imagine that you execute the > following code in the main frame. > > > > - If you call iframe.contentDocument, in the binding callback for > contentDocument, which context do the isolate->GetCurrentContext and the > info.Holder()->CreationContext return? > > > > (My guess is that both return the iframe's context.) > > current context will be the main frame, while creation context will be the > iframe Thanks, that makes sense :) > > > > > - If you call iframe.contentDocument.createElement(), in the binding callback > for createElement(), which context do the isolate->GetCurrentContext and the > info.Holder()->CreationContext return? > > > > (My guess is that both return the iframe's context.) > > same as above > > > > > - In what scenario, do the isolate->GetCurrentContext and the > info.Holder()->CreationContext return different contexts?
Message was sent while issue was closed.
(Updating the old thread just FYI...) On 2016/01/07 10:06:07, jochen wrote: > On 2015/12/22 at 07:17:51, haraken wrote: > > > https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... > > File Source/bindings/core/v8/V8DOMWrapper.h (right): > > > > > https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... > > Source/bindings/core/v8/V8DOMWrapper.h:124: if (contextForWrapper == > m_context) > > > > epertoso@ and jochen@: I'm getting a bit confused. In what scenario can > info.Holder()->CreationContext() be different from isolate->GetCurrentContext()? > > > > Imagine that you have a main frame and an iframe. Imagine that you execute the > following code in the main frame. > > > > - If you call iframe.contentDocument, in the binding callback for > contentDocument, which context do the isolate->GetCurrentContext and the > info.Holder()->CreationContext return? > > > > (My guess is that both return the iframe's context.) > > current context will be the main frame, while creation context will be the > iframe > > > > > - If you call iframe.contentDocument.createElement(), in the binding callback > for createElement(), which context do the isolate->GetCurrentContext and the > info.Holder()->CreationContext return? > > > > (My guess is that both return the iframe's context.) > > same as above I noticed that our theory is not right. When we call iframe.contentDocument, in the C++ callback for contentDocument: - info.Holder()->CreationContext points to the main frame's context. - isolate->GetCurrentContext points to the main frame's context. When we call iframe.contentDocument.createElement(), in the C++ callback for createElement: - info.Holder()->CreationContext points to iframe's context. - isolate->GetCurrentContext points to iframe's context. This is because V8 updates the current context to info.Holder()->CreationContext before V8 calls back binding methods (https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/execution.c...). So info.Holder()->CreationContext should be equal to isolate->GetCurrentContext at the point when V8 calls back binding methods. I don't know when these two point to different contexts when we create a new wrapper via V8WrapperInstantiationScope.
Message was sent while issue was closed.
On 2016/04/20 at 07:22:35, haraken wrote: > (Updating the old thread just FYI...) > > On 2016/01/07 10:06:07, jochen wrote: > > On 2015/12/22 at 07:17:51, haraken wrote: > > > > > https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... > > > File Source/bindings/core/v8/V8DOMWrapper.h (right): > > > > > > > > https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... > > > Source/bindings/core/v8/V8DOMWrapper.h:124: if (contextForWrapper == > > m_context) > > > > > > epertoso@ and jochen@: I'm getting a bit confused. In what scenario can > > info.Holder()->CreationContext() be different from isolate->GetCurrentContext()? > > > > > > Imagine that you have a main frame and an iframe. Imagine that you execute the > > following code in the main frame. > > > > > > - If you call iframe.contentDocument, in the binding callback for > > contentDocument, which context do the isolate->GetCurrentContext and the > > info.Holder()->CreationContext return? > > > > > > (My guess is that both return the iframe's context.) > > > > current context will be the main frame, while creation context will be the > > iframe > > > > > > > > - If you call iframe.contentDocument.createElement(), in the binding callback > > for createElement(), which context do the isolate->GetCurrentContext and the > > info.Holder()->CreationContext return? > > > > > > (My guess is that both return the iframe's context.) > > > > same as above > > I noticed that our theory is not right. > > When we call iframe.contentDocument, in the C++ callback for contentDocument: > > - info.Holder()->CreationContext points to the main frame's context. > - isolate->GetCurrentContext points to the main frame's context. > > When we call iframe.contentDocument.createElement(), in the C++ callback for createElement: > > - info.Holder()->CreationContext points to iframe's context. > - isolate->GetCurrentContext points to iframe's context. > > This is because V8 updates the current context to info.Holder()->CreationContext before V8 calls back binding methods (https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/execution.c...). > > So info.Holder()->CreationContext should be equal to isolate->GetCurrentContext at the point when V8 calls back binding methods. I don't know when these two point to different contexts when we create a new wrapper via V8WrapperInstantiationScope. given that we still see crashes on document.all and performance.now I suspect that it's possible. I know why we crash on document.all - it's implemented via an interceptor and the above reasoning doesn't hold. I don't know what goes wrong with performance.now
Message was sent while issue was closed.
On 2016/04/20 07:43:13, jochen wrote: > On 2016/04/20 at 07:22:35, haraken wrote: > > (Updating the old thread just FYI...) > > > > On 2016/01/07 10:06:07, jochen wrote: > > > On 2015/12/22 at 07:17:51, haraken wrote: > > > > > > > > https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... > > > > File Source/bindings/core/v8/V8DOMWrapper.h (right): > > > > > > > > > > > > https://codereview.chromium.org/1262353002/diff/60001/Source/bindings/core/v8... > > > > Source/bindings/core/v8/V8DOMWrapper.h:124: if (contextForWrapper == > > > m_context) > > > > > > > > epertoso@ and jochen@: I'm getting a bit confused. In what scenario can > > > info.Holder()->CreationContext() be different from > isolate->GetCurrentContext()? > > > > > > > > Imagine that you have a main frame and an iframe. Imagine that you execute > the > > > following code in the main frame. > > > > > > > > - If you call iframe.contentDocument, in the binding callback for > > > contentDocument, which context do the isolate->GetCurrentContext and the > > > info.Holder()->CreationContext return? > > > > > > > > (My guess is that both return the iframe's context.) > > > > > > current context will be the main frame, while creation context will be the > > > iframe > > > > > > > > > > > - If you call iframe.contentDocument.createElement(), in the binding > callback > > > for createElement(), which context do the isolate->GetCurrentContext and the > > > info.Holder()->CreationContext return? > > > > > > > > (My guess is that both return the iframe's context.) > > > > > > same as above > > > > I noticed that our theory is not right. > > > > When we call iframe.contentDocument, in the C++ callback for contentDocument: > > > > - info.Holder()->CreationContext points to the main frame's context. > > - isolate->GetCurrentContext points to the main frame's context. > > > > When we call iframe.contentDocument.createElement(), in the C++ callback for > createElement: > > > > - info.Holder()->CreationContext points to iframe's context. > > - isolate->GetCurrentContext points to iframe's context. > > > > This is because V8 updates the current context to > info.Holder()->CreationContext before V8 calls back binding methods > (https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/execution.c...). > > > > So info.Holder()->CreationContext should be equal to > isolate->GetCurrentContext at the point when V8 calls back binding methods. I > don't know when these two point to different contexts when we create a new > wrapper via V8WrapperInstantiationScope. > > given that we still see crashes on document.all and performance.now I suspect > that it's possible. > > I know why we crash on document.all - it's implemented via an interceptor and > the above reasoning doesn't hold. I don't know what goes wrong with > performance.now Trying to add a release-assert to understand when those two point to different contexts: https://codereview.chromium.org/1901393002. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
