Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2458183002:
Eagerly install Origin Trial features on window (Closed)
DescriptionEagerly install Origin Trial features on window.
This fixes an issue where origin trial features which were to be installed on Window (attributes of window as well as globally visible interface objects) would not installed if the V8 context was being reused, as happens when the frame is navigated.
This also replaces the use of Isolate and DOMWrapperWorld in Origin trial methods with ScriptState, which encapsulates both of those, as well as the correct V8 context for installing origin-trial-enabled attributes.
BUG=653671
Committed: https://crrev.com/8a251a4b78a1f173a03ac2f0239c8a48216eed92
Cr-Commit-Position: refs/heads/master@{#429772}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix WebCoreTestSupport for content_shell #Patch Set 4 : Clean up #
Total comments: 28
Patch Set 5 : Addressing review comments #
Total comments: 2
Patch Set 6 : Use correct context and enter it before installing attributes on window #
Total comments: 4
Patch Set 7 : Check for isolated worlds; simplify context check #Patch Set 8 : Address comments from PS#7, move ScriptState change to new CL #
Total comments: 4
Patch Set 9 : Add DCHECK #Patch Set 10 : Rebase before commit #
Messages
Total messages: 59 (37 generated)
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by iclelland@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.
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Eagerly install Origin Trial features on window BUG= ========== to ========== Eagerly install Origin Trial features on window. This fixes an issue where origin trial features which were to be installed on Window (attributes of window as well as globally visible interface objects) would not installed if the V8 context was being reused, as happens when the frame is navigated. This also replaces the use of Isolate and DOMWrapperWorld in Origin trial methods with ScriptState, which encapsulates both of those, as well as the correct V8 context for installing origin-trial-enabled attributes. BUG=653671 ==========
iclelland@chromium.org changed reviewers: + dcheng@chromium.org, yukishiino@chromium.org
iclelland@chromium.org changed reviewers: - dcheng@chromium.org
Yuki, can you PTAL? Thanks! This reinstates some of the code that we had pre-M55, where we eagerly installed trials directly on interface objects as soon as the context was ready. We switched to lazy initialization for M55, but it turns out that it's not possible to do that for Window, since the context can be reused, and we would end up initializing the window object before the new document was parsed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/31 04:24:29, iclelland wrote: > Yuki, can you PTAL? Thanks! > > This reinstates some of the code that we had pre-M55, where we eagerly installed > trials directly on interface objects as soon as the context was ready. We > switched to lazy initialization for M55, but it turns out that it's not possible > to do that for Window, since the context can be reused, and we would end up > initializing the window object before the new document was parsed. If this is the only way, I'm okay with landing this CL for now, but just to clarify, what's a performance implication of this CL? i.e., what overhead is going to be added to the context initialization?
On 2016/10/31 10:26:45, haraken wrote: > On 2016/10/31 04:24:29, iclelland wrote: > > Yuki, can you PTAL? Thanks! > > > > This reinstates some of the code that we had pre-M55, where we eagerly > installed > > trials directly on interface objects as soon as the context was ready. We > > switched to lazy initialization for M55, but it turns out that it's not > possible > > to do that for Window, since the context can be reused, and we would end up > > initializing the window object before the new document was parsed. > > If this is the only way, I'm okay with landing this CL for now, but just to > clarify, what's a performance implication of this CL? i.e., what overhead is > going to be added to the context initialization? As far as I understand (I haven't profiled it to be certain), this should add minimal overhead to context initialization. This code path should only be taken when the global window object is already initialized, and should return very quickly in the case where there are no origin trial tokens present. If there are, we only install attributes on the window (which is already present) and do not instantiate any additional objects. (The previous performance regression -- crbug.com/637148 -- was caused by always instantiating all objects with origin trial attributes, such as Navigator and Document.) To keep the code clean, I've made WindowProxy::initialize always call |installPendingConditionalFeaturesOnWindow|, which will call through the function pointer to |installConditionalFeaturesForModules| and then |installConditionalFeaturesCore|, which will then discover that there is no OriginTrialContext and do nothing. I can make it faster by short-circuiting those functions in that case, and slightly faster still by testing in WindowProxy.cpp whether there is an OriginTrialContext and returning if not. (This would look much like the code in https://chromium.googlesource.com/chromium/src/+/fbd849942b44c5923cf15f29cab1...)
haraken@chromium.org changed reviewers: + haraken@chromium.org
Thanks for the clarification. LGTM. FWIW, peria@ is now planning to add a telemetry benchmark to track performance of WindowProxy::initialize for the main frame and iframes. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:64: if (!scriptState->perContextData()) Can this be DCHECK(m_scriptState) and DCHECK(scriptState->perContextData())? https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:68: v8::Local<v8::Function>()); Why is it okay to pass in empty prototype and interface objects? https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:269: V8PerContextData::from(context)->constructorForType( Can we remove this code now?
https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:61: void installPendingConditionalFeaturesOnWindow(const ScriptState* scriptState) { You might want to CHECK: scriptState->context() == scriptState->isolate()->GetCurrentContext() https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:83: v8::Local<v8::Object> instance, Shall we change the function name and argument name and/or order drastically? I was misunderstanding what this function is expected to do. Other utility functions in this file are designed to take instance = instance object or template of type X prototype = prototype object or template of type X interface = interface object or template of type X however, this function takes instance = the global object (or navigator object?) prototype = prototype object or template of type X interface = interface object or template of type X This is quite confusing. We should make it clear that this function is so special, and it's the global object. IIUC, this function always installs the interface object on the global object (or window.navigator?), right? Then, I'd propose exposeInterfaceObject(const WrapperTypeInfo* interface, v8::Local<v8::Object> target) or something like that. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:91: !world.isPrivateScriptIsolatedWorld()) Given that this function is so special, do you need |world| at all? Do you have any plan to do something depends on a world? https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:105: v8::Local<v8::Function> data = nit: s/data/value/ since this is used the value of the property. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:112: scriptState->context(), name, data, The context here must always be the current context regardless of what you're doing. Thus isolate->GetCurrentContext() is preferred. I know your assumption is scriptState->context() == isolate->GetCurrentContext(), but it's not clear just from this function definition. So, I prefer isolate->GetCurrentContext(). https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:118: scriptState->context(), name, data, ditto. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:32: void installConditionalFeaturesForModules( Could you add a function header comment in *.h? Especially what each argument should be. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:49: V8NavigatorPartial::installWebShare(scriptState, v8::Local<v8::Object>(), Given that you've already extracted |global|, why don't you pass in |global|? https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:393: void {{v8_class_or_partial}}::install{{origin_trial_feature.name}}(const ScriptState* scriptState, v8::Local<v8::Object> instance, v8::Local<v8::Object> prototype, v8::Local<v8::Function> interface) { I have the same thought as I commented at installAttributeInternal in V8DOMConfiguration.cpp. |instance| is not an instance of type X, right? I'd like to have a comment to elaborate what each argument is. I welcome anythings that make things clearer. Probably you'd need |instance|, |prototype|, |interface|, and |interface_exposure_target| (I don't come up with a good name) as arguments? Just in case, why other functions take instance, prototype, and interface is: a) [Unforgeable] attributes/operations must be placed on |instance|, and b) other attributes/operations must be placed on |prototype|, and c) constants must be placed on |interface| and optionally on |prototype|. but, d) the interface objects should be exposed on the global object in general. I guess you'd need all of them.
The CQ bit was checked by iclelland@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/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:61: void installPendingConditionalFeaturesOnWindow(const ScriptState* scriptState) { On 2016/11/01 11:37:54, Yuki wrote: > You might want to CHECK: > scriptState->context() == scriptState->isolate()->GetCurrentContext() In what cases is this not going to be true? I think I may have actually run across situations like that in testing, when calling this from OriginTrialContext::addTokens. It's actually what led me to fix the scriptstate issue, to ensure that I was actually using the ScriptState context correctly. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:64: if (!scriptState->perContextData()) On 2016/11/01 06:42:41, haraken wrote: > > Can this be DCHECK(m_scriptState) and DCHECK(scriptState->perContextData())? Yes, done, thanks. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:68: v8::Local<v8::Function>()); On 2016/11/01 06:42:41, haraken wrote: > > Why is it okay to pass in empty prototype and interface objects? The only thing that we use Origin Trials for on Window are constructor attributes, which need to be installed with V8COMConfiguration::OnInstance, and so the prototype and interface objects are never used. We still need to plumb them all the way through to V8DOMConfiguration::installAttribute, though, since the code generation isn't clever enough yet to create install* methods with different signatures. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:83: v8::Local<v8::Object> instance, On 2016/11/01 11:37:54, Yuki wrote: > Shall we change the function name and argument name and/or order drastically? I > was misunderstanding what this function is expected to do. > > Other utility functions in this file are designed to take > instance = instance object or template of type X > prototype = prototype object or template of type X > interface = interface object or template of type X > however, this function takes > instance = the global object (or navigator object?) > prototype = prototype object or template of type X > interface = interface object or template of type X > > This is quite confusing. We should make it clear that this function is so > special, and it's the global object. > > IIUC, this function always installs the interface object on the global object > (or window.navigator?), right? Then, I'd propose > exposeInterfaceObject(const WrapperTypeInfo* interface, > v8::Local<v8::Object> target) > or something like that. I have no problem renaming, or even restructuring the file. I'd like to make sure that we're clear what it's doing, though. The install*internal methods in this file seem to take two forms, depending on whether they are modifying a template or an instance. InstallAccessorInternal is the only exception, and is templated to take one form or the other. The actual parameters depend on what is being installed: attributes take instance and prototype, constants take prototype and interface, and accessors and methods need all three (instance, prototype and interface). This file contains an implementation for every combination of member {accessor,attribute,constant,method} and target {object,template}. This method, in particular, installs attributes from the generated binding code onto any instantiated v8 object instance (or optionally its prototype, if that's how the attribute is defined). Now, in practice: This method is only used to expose constructor interfaces on the global objects -- Window as well as ServiceWorkerGlobalScope. It has been used on WorkerGlobalScope before, by DurableStorage, but not currently. Other origin-trial-enabled features, on objects like Navigator (and HTMLLinkElement, Gamepad, etc) all use installMethod and installAccessor instead. I'm not certain if there are IDL configurations that would cause installAttribute to be used on objects other than the global objects. Given that, we could rename this one method, and leave the other seven as-is. If we're breaking the orthogonality there, then we can also just move this code into installAttribute, and rename that instead. In that case, we'd need to change the bindings code generator to know that the method to call is different in that one case (installing an attribute in an origin trial), but that should be possible. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:91: !world.isPrivateScriptIsolatedWorld()) On 2016/11/01 11:37:54, Yuki wrote: > Given that this function is so special, do you need |world| at all? > Do you have any plan to do something depends on a world? Not for origin trials. Again, this method was written to be a general mechanism to install attribute-type members on instantiated objects. I don't think there are any plans to conditionally install constructor interfaces in extensions. It's possible that with Feature Policy, we may be conditionally installing constructors like RTCPeerConnection, but that's not a final design decision yet. If that interface is currently accessible from isolated worlds, then we'd need to be able to install it there as well, when not disabled. But again, it's not final, and may be premature to say that it will be needed. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:112: scriptState->context(), name, data, On 2016/11/01 11:37:54, Yuki wrote: > The context here must always be the current context regardless of what you're > doing. Thus isolate->GetCurrentContext() is preferred. I know your assumption > is scriptState->context() == isolate->GetCurrentContext(), but it's not clear > just from this function definition. So, I prefer isolate->GetCurrentContext(). I mentioned this in another comment as well; I ran into a problem while testing this, that after a navigation, the context is reused, but when we encounter the <meta> tags for the origin trial, the scriptstate's context is initialized, but isolate->GetCurrentContext() is empty. This was actually the reason that I switched this file to use ScriptState rather than the pair (Isolate*,DOMWrapperWorld&). If that's incorrect, I can change it, but I may need to do something in OriginTrialContext::initializePendingFeatures to initialize or enter that context. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:269: V8PerContextData::from(context)->constructorForType( On 2016/11/01 06:42:41, haraken wrote: > > Can we remove this code now? I believe we can, now. Thanks! https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:49: V8NavigatorPartial::installWebShare(scriptState, v8::Local<v8::Object>(), On 2016/11/01 11:37:54, Yuki wrote: > Given that you've already extracted |global|, why don't you pass in |global|? That parameter is supposed to be the object instance to install the interface members on, *if* any of them are configured with V8DOMConfiguration::OnInterface. This doesn't happen except with constructor attrtibutes, so the parameter is unused here. Passing |global| would satisfy the requirements of the function signature, and it isn't used anyway, but it's the *wrong* object if it were used. If we improved the code generation, we could tell that the instance parameter is never used, and remove it from |installWebShare| and similar methods. But at the moment, all of the install{origin_trial} methods take all three parameters (instance, prototype, interface), even if one or more of them are unused. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:393: void {{v8_class_or_partial}}::install{{origin_trial_feature.name}}(const ScriptState* scriptState, v8::Local<v8::Object> instance, v8::Local<v8::Object> prototype, v8::Local<v8::Function> interface) { On 2016/11/01 11:37:54, Yuki wrote: > I have the same thought as I commented at installAttributeInternal in > V8DOMConfiguration.cpp. > > |instance| is not an instance of type X, right? It *is*, though. The method takes |instance|, |prototype|, and |interface|, and they correspond to the targets for V8DOMConfiguration::OnInstance, OnPrototype and OnInterface. In almost every case, not all three of these will be used, but it was easiest for the template to generate methods taking all three. > > I'd like to have a comment to elaborate what each argument is. > > I welcome anythings that make things clearer. > > Probably you'd need |instance|, |prototype|, |interface|, and > |interface_exposure_target| (I don't come up with a good name) as arguments? |instance| and |interface_exposure_target| are the same object, I believe. > Just in case, why other functions take instance, prototype, and interface is: > a) [Unforgeable] attributes/operations must be placed on |instance|, and > b) other attributes/operations must be placed on |prototype|, and > c) constants must be placed on |interface| and optionally on |prototype|. That matches my understanding. We haven't tried to handle [Unforgeable] in origin trials yet, and I suspect it wouldn't work very well. b) and c) are the reasons that we have the |prototype| and |interface| arguments. > but, > d) the interface objects should be exposed on the global object in general. This case is handled by having the interface objects configured as OnInstance attributes of the globals, so in those cases, the |instance| argument is used, and should be the global object. In other cases, |instance| is unused, and so a dummy v8::Object is passed instead. > I guess you'd need all of them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:112: scriptState->context(), name, data, On 2016/11/01 17:06:27, iclelland wrote: > On 2016/11/01 11:37:54, Yuki wrote: > > The context here must always be the current context regardless of what you're > > doing. Thus isolate->GetCurrentContext() is preferred. I know your > assumption > > is scriptState->context() == isolate->GetCurrentContext(), but it's not clear > > just from this function definition. So, I prefer > isolate->GetCurrentContext(). > > I mentioned this in another comment as well; I ran into a problem while testing > this, that after a navigation, the context is reused, but when we encounter the > <meta> tags for the origin trial, the scriptstate's context is initialized, but > isolate->GetCurrentContext() is empty. > > This was actually the reason that I switched this file to use ScriptState rather > than the pair (Isolate*,DOMWrapperWorld&). > > If that's incorrect, I can change it, but I may need to do something in > OriginTrialContext::initializePendingFeatures to initialize or enter that > context. As shiino-san mentioned, scriptState->context() == isolate->GetCurrentContext() must hold. Specifically, in what situation are you calling installDOMXXX when isolate->GetCurrentContext() is null? It means that you're doing V8 operations before entering any v8::Context, which seems wrong.
On 2016/11/02 02:35:45, haraken wrote: > https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right): > > https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:112: > scriptState->context(), name, data, > On 2016/11/01 17:06:27, iclelland wrote: > > On 2016/11/01 11:37:54, Yuki wrote: > > > The context here must always be the current context regardless of what > you're > > > doing. Thus isolate->GetCurrentContext() is preferred. I know your > > assumption > > > is scriptState->context() == isolate->GetCurrentContext(), but it's not > clear > > > just from this function definition. So, I prefer > > isolate->GetCurrentContext(). > > > > I mentioned this in another comment as well; I ran into a problem while > testing > > this, that after a navigation, the context is reused, but when we encounter > the > > <meta> tags for the origin trial, the scriptstate's context is initialized, > but > > isolate->GetCurrentContext() is empty. > > > > This was actually the reason that I switched this file to use ScriptState > rather > > than the pair (Isolate*,DOMWrapperWorld&). > > > > If that's incorrect, I can change it, but I may need to do something in > > OriginTrialContext::initializePendingFeatures to initialize or enter that > > context. > > As shiino-san mentioned, scriptState->context() == isolate->GetCurrentContext() > must hold. > > Specifically, in what situation are you calling installDOMXXX when > isolate->GetCurrentContext() is null? It means that you're doing V8 operations > before entering any v8::Context, which seems wrong. I saw this while testing, in this situation: 1. Open chrome and navigate to a page with no origin trial tokens present. 2. Navigate to a page with trial tokens present in meta tags, for a trial which install attributes on window. (WebUSB and WebBluetooth both showed this behaviour when I tested them) When navigating to the trial-enabled page, the WindowProxy from step 1 is reused, with the globals cleared and reset. I believe that the context is still initialized, though. At that point, when we encounter <meta> tags during parsing, OriginTrialContext::installPendingFeatures sees that the scriptState->context is initialized, but we're not actually running any script at that point, so isolate->GetCurrentContext returns an empty local. (Based on what you've said, I believe that's the reason it doesn't return a context). From the crashes I saw, I thought I was simply using the wrong context, and that the ScriptState had the correct one, but it sounds like that's not true. In that case, I see a couple of possible ways forward: -- I can enter a context while parsing the document head, and install the trial attributes at that point. -- I can postpone adding the trial attributes to the global until some future point, when the context is first entered. If there isn't currently a way to do this (since the context is technically already initialized) then I can add some kind of state variable to say whether it has been entered before, and then add trial attributes when it is entered the first time after being cleared.
On 2016/11/02 03:33:52, iclelland wrote: > On 2016/11/02 02:35:45, haraken wrote: > > > https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp > (right): > > > > > https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:112: > > scriptState->context(), name, data, > > On 2016/11/01 17:06:27, iclelland wrote: > > > On 2016/11/01 11:37:54, Yuki wrote: > > > > The context here must always be the current context regardless of what > > you're > > > > doing. Thus isolate->GetCurrentContext() is preferred. I know your > > > assumption > > > > is scriptState->context() == isolate->GetCurrentContext(), but it's not > > clear > > > > just from this function definition. So, I prefer > > > isolate->GetCurrentContext(). > > > > > > I mentioned this in another comment as well; I ran into a problem while > > testing > > > this, that after a navigation, the context is reused, but when we encounter > > the > > > <meta> tags for the origin trial, the scriptstate's context is initialized, > > but > > > isolate->GetCurrentContext() is empty. > > > > > > This was actually the reason that I switched this file to use ScriptState > > rather > > > than the pair (Isolate*,DOMWrapperWorld&). > > > > > > If that's incorrect, I can change it, but I may need to do something in > > > OriginTrialContext::initializePendingFeatures to initialize or enter that > > > context. > > > > As shiino-san mentioned, scriptState->context() == > isolate->GetCurrentContext() > > must hold. > > > > Specifically, in what situation are you calling installDOMXXX when > > isolate->GetCurrentContext() is null? It means that you're doing V8 operations > > before entering any v8::Context, which seems wrong. > > I saw this while testing, in this situation: > 1. Open chrome and navigate to a page with no origin trial tokens present. > 2. Navigate to a page with trial tokens present in meta tags, for a trial which > install attributes on window. (WebUSB and WebBluetooth both showed this > behaviour when I tested them) > > When navigating to the trial-enabled page, the WindowProxy from step 1 is > reused, with the globals cleared and reset. I believe that the context is still > initialized, though. At that point, when we encounter <meta> tags during > parsing, OriginTrialContext::installPendingFeatures sees that the > scriptState->context is initialized, but we're not actually running any script > at that point, so isolate->GetCurrentContext returns an empty local. (Based on > what you've said, I believe that's the reason it doesn't return a context). > > From the crashes I saw, I thought I was simply using the wrong context, and that > the ScriptState had the correct one, but it sounds like that's not true. > > In that case, I see a couple of possible ways forward: > -- I can enter a context while parsing the document head, and install the trial > attributes at that point. > -- I can postpone adding the trial attributes to the global until some future > point, when the context is first entered. If there isn't currently a way to do > this (since the context is technically already initialized) then I can add some > kind of state variable to say whether it has been entered before, and then add > trial attributes when it is entered the first time after being cleared. Thanks for the details! Can we probably enter ScriptState::Scope (which enters a V8 context) before calling installPendingActivities?
For scriptState things, please follow haraken's suggestion (to enter the V8
context before doing something).
The web spec requires us to handle "current realm" and "relevant realm" in
general, and I'd like to use the following pattern as long as possible.
v8::Isolate::GetCurrentContext() == "current realm"
scriptState of the first argument == "relevant realm"
I think this makes our codebase simple and consistent.
In your case, I think you should enter the context you have and make it the
current context.
https://codereview.chromium.org/2458183002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp
(right):
https://codereview.chromium.org/2458183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:57:
V8NavigatorPartial::installWebVR(scriptState, global, prototypeObject,
Hmm, I'm confused.
V8NavigatorPartial::installWebVR is generated from
{{v8_class_or_partial}}::install{{origin_trial_feature.name}} in
interface_base.cpp.tmpl, right?
Then, the function's arguments should be
scriptState, instance, prototype, interface
where |instance| should be of type Navigator, right?
But you're actually passing |global| of type Window to |instance|, right?
IIUC, installWebVR should take
instance object of type Navigator,
prototype object of type Navigator,
interface object of typeNavigator, and
global object where the interface object (i.e. constructor) should be exposed.
What do you think?
On 2016/11/02 04:05:18, haraken wrote: > Thanks for the details! > > Can we probably enter ScriptState::Scope (which enters a V8 context) before > calling installPendingActivities? Yes! That appears to work correctly; I will update this CL with that fix. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:61: void installPendingConditionalFeaturesOnWindow(const ScriptState* scriptState) { On 2016/11/01 17:06:27, iclelland wrote: > On 2016/11/01 11:37:54, Yuki wrote: > > You might want to CHECK: > > scriptState->context() == scriptState->isolate()->GetCurrentContext() > > In what cases is this not going to be true? I think I may have actually run > across situations like that in testing, when calling this from > OriginTrialContext::addTokens. It's actually what led me to fix the scriptstate > issue, to ensure that I was actually using the ScriptState context correctly. I've added this, now that it's entering a context properly. Thanks. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:105: v8::Local<v8::Function> data = On 2016/11/01 11:37:54, Yuki wrote: > nit: s/data/value/ since this is used the value of the property. Sure, done. Should this also be the case for installAttributeInternal(Isolate, Template, Template, ...) above? https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:112: scriptState->context(), name, data, On 2016/11/01 11:37:54, Yuki wrote: > The context here must always be the current context regardless of what you're > doing. Thus isolate->GetCurrentContext() is preferred. I know your assumption > is scriptState->context() == isolate->GetCurrentContext(), but it's not clear > just from this function definition. So, I prefer isolate->GetCurrentContext(). Thanks for your patience. I've reverted this to isolate->GetCurrentContext(). To be clear, is it still correct to use scriptState->context) when getting perContextData? (a few lines above this) https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:118: scriptState->context(), name, data, On 2016/11/01 11:37:54, Yuki wrote: > ditto. Done. https://codereview.chromium.org/2458183002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp (right): https://codereview.chromium.org/2458183002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:57: V8NavigatorPartial::installWebVR(scriptState, global, prototypeObject, On 2016/11/02 09:21:07, Yuki wrote: > Hmm, I'm confused. > > V8NavigatorPartial::installWebVR is generated from > {{v8_class_or_partial}}::install{{origin_trial_feature.name}} in > interface_base.cpp.tmpl, right? > > Then, the function's arguments should be > scriptState, instance, prototype, interface > where |instance| should be of type Navigator, right? > > But you're actually passing |global| of type Window to |instance|, right? Yes, that is right. I missed this when I reviewed https://codereview.chromium.org/2385123005 Thank you for catching it; I will fix that. > > IIUC, installWebVR should take > instance object of type Navigator, > prototype object of type Navigator, > interface object of typeNavigator, and > global object where the interface object (i.e. constructor) should be exposed. > > What do you think? There is no need for V8NavigatorPartial::installWebVR to take the global object; it doesn't install any constructors. They are all installed in V8WindowPartial::installWebVR, and in that method, |instance| is the global object on which to expose the interface.
The CQ bit was checked by iclelland@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.
LGTM https://codereview.chromium.org/2458183002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2458183002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:276: installPendingConditionalFeaturesOnWindow(m_scriptState.get()); Just help me understand: Who prevents us from installing origin-trial settings on isolated worlds? I'm assuming that you don't want to enable origin-trials on isolated worlds. https://codereview.chromium.org/2458183002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/2458183002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:185: ->isContextInitialized()) Instead you can check scriptState->contextIsValid().
The CQ bit was checked by iclelland@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...
haraken, can you take one more look? The code changes around ScriptState became unnecessary after we fixed the context issue, and they have prompted much more discussion, so I've moved them to a separate CL. (crrev.com/2474983002) I also wanted to make this change as small as possible, since my goal is to merge it to M55, as this bug is going to impact the existing origin trials. Can you just take a quick look to make sure that landing just this part of the original CL is okay? https://codereview.chromium.org/2458183002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2458183002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:276: installPendingConditionalFeaturesOnWindow(m_scriptState.get()); On 2016/11/03 14:43:07, haraken wrote: > > Just help me understand: Who prevents us from installing origin-trial settings > on isolated worlds? I'm assuming that you don't want to enable origin-trials on > isolated worlds. > We don't want to do that -- thanks for catching it. It caused problems in the past (crbug.com/619465 for one) and we had to explicitly remove them. I'll add the check back in here until we've determined that that issue is fixed. I do eventually want to make it possible to support this kind of conditional binding in isolated worlds -- not for origin trials, but for other work, like Feature Policy. For now, I'll add that check. https://codereview.chromium.org/2458183002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/2458183002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:185: ->isContextInitialized()) On 2016/11/03 14:43:07, haraken wrote: > > Instead you can check scriptState->contextIsValid(). Simpler, thanks!
LGTM https://codereview.chromium.org/2458183002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2458183002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:56: v8::Local<v8::Function> interfaceObject) { Add DCHECK(scriptState->world()->isMainWorld()). https://codereview.chromium.org/2458183002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:64: DCHECK(scriptState->perContextData()); Add DCHECK(scriptState->world()->isMainWorld()).
Patchset #9 (id:160001) has been deleted
https://codereview.chromium.org/2458183002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): https://codereview.chromium.org/2458183002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:56: v8::Local<v8::Function> interfaceObject) { On 2016/11/03 16:02:17, haraken wrote: > > Add DCHECK(scriptState->world()->isMainWorld()). I can't do that here -- that would require deeper changes, since this is called as part of V8PerContextData::constructorForTypeSlowCase(), even though it will not add any origin trial attributes in the case of isolated worlds. (I just tried, and it crashed as soon as I entered extension code, so I removed that patch. I'll re-upload it with just the other DCHECK) https://codereview.chromium.org/2458183002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:64: DCHECK(scriptState->perContextData()); On 2016/11/03 16:02:17, haraken wrote: > > Add DCHECK(scriptState->world()->isMainWorld()). Done. This one is safe (and correct :) )
On 2016/11/03 16:43:04, iclelland wrote: > https://codereview.chromium.org/2458183002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right): > > https://codereview.chromium.org/2458183002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:56: > v8::Local<v8::Function> interfaceObject) { > On 2016/11/03 16:02:17, haraken wrote: > > > > Add DCHECK(scriptState->world()->isMainWorld()). > > I can't do that here -- that would require deeper changes, since this is called > as part of V8PerContextData::constructorForTypeSlowCase(), even though it will > not add any origin trial attributes in the case of isolated worlds. > > (I just tried, and it crashed as soon as I entered extension code, so I removed > that patch. I'll re-upload it with just the other DCHECK) > > https://codereview.chromium.org/2458183002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:64: > DCHECK(scriptState->perContextData()); > On 2016/11/03 16:02:17, haraken wrote: > > > > Add DCHECK(scriptState->world()->isMainWorld()). > > Done. This one is safe (and correct :) ) Makes sense.
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2458183002/#ps200001 (title: "Rebase before commit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by iclelland@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Eagerly install Origin Trial features on window. This fixes an issue where origin trial features which were to be installed on Window (attributes of window as well as globally visible interface objects) would not installed if the V8 context was being reused, as happens when the frame is navigated. This also replaces the use of Isolate and DOMWrapperWorld in Origin trial methods with ScriptState, which encapsulates both of those, as well as the correct V8 context for installing origin-trial-enabled attributes. BUG=653671 ========== to ========== Eagerly install Origin Trial features on window. This fixes an issue where origin trial features which were to be installed on Window (attributes of window as well as globally visible interface objects) would not installed if the V8 context was being reused, as happens when the frame is navigated. This also replaces the use of Isolate and DOMWrapperWorld in Origin trial methods with ScriptState, which encapsulates both of those, as well as the correct V8 context for installing origin-trial-enabled attributes. BUG=653671 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Eagerly install Origin Trial features on window. This fixes an issue where origin trial features which were to be installed on Window (attributes of window as well as globally visible interface objects) would not installed if the V8 context was being reused, as happens when the frame is navigated. This also replaces the use of Isolate and DOMWrapperWorld in Origin trial methods with ScriptState, which encapsulates both of those, as well as the correct V8 context for installing origin-trial-enabled attributes. BUG=653671 ========== to ========== Eagerly install Origin Trial features on window. This fixes an issue where origin trial features which were to be installed on Window (attributes of window as well as globally visible interface objects) would not installed if the V8 context was being reused, as happens when the frame is navigated. This also replaces the use of Isolate and DOMWrapperWorld in Origin trial methods with ScriptState, which encapsulates both of those, as well as the correct V8 context for installing origin-trial-enabled attributes. BUG=653671 Committed: https://crrev.com/8a251a4b78a1f173a03ac2f0239c8a48216eed92 Cr-Commit-Position: refs/heads/master@{#429772} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/8a251a4b78a1f173a03ac2f0239c8a48216eed92 Cr-Commit-Position: refs/heads/master@{#429772}
Message was sent while issue was closed.
LGTM. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right): https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:105: v8::Local<v8::Function> data = On 2016/11/02 13:49:37, iclelland wrote: > On 2016/11/01 11:37:54, Yuki wrote: > > nit: s/data/value/ since this is used the value of the property. > > Sure, done. Should this also be the case for installAttributeInternal(Isolate, > Template, Template, ...) above? No. Data property setting on V8 takes a pair of (key, value) and an additional |data| field. The above installAttributeInternal is setting the additional data field. So, I think it's okay to call it |data|. But, in this function, this is the value of (key, value) pair. https://codereview.chromium.org/2458183002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:112: scriptState->context(), name, data, On 2016/11/02 13:49:37, iclelland wrote: > On 2016/11/01 11:37:54, Yuki wrote: > > The context here must always be the current context regardless of what you're > > doing. Thus isolate->GetCurrentContext() is preferred. I know your > assumption > > is scriptState->context() == isolate->GetCurrentContext(), but it's not clear > > just from this function definition. So, I prefer > isolate->GetCurrentContext(). > > Thanks for your patience. I've reverted this to isolate->GetCurrentContext(). To > be clear, is it still correct to use scriptState->context) when getting > perContextData? (a few lines above this) Yes, it's correct to use scriptState->context() to get perContextData(). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
