haraken, you added the code in the linked CL, so you might be the best person to resolve this bug. I'll start investigating tomorrow regardless; if I see the issue, I can take it over.

Thanks

Hmm, this looks strange. A window object should be recreated every time you navigate (unless you're navigating from an initial empty document), so V8PerContextData::constructorForTypeSlowCase should be called.

Would you double-check if you're creating a new window when you navigate?

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/DocumentLoader.cpp?q=documentloader&sq=package:chromium&dr=CSs&l=725

dcheng@ might have some ideas.

I'm not familiar with origin trials, so how should I be using those test pages?

One thing I odd that I noticed is:
- Open in new tab: always reports NO for window.BluetoothDevice
- Open in new incognito tab: reports YES for window.BluetoothDevice

Should it be reporting YES in case 1?

index.html has a valid origin trial token, so it should be reporting YES every time, for both navigator and window attributes.

index2.html does not have a token, so it should always report NO for both.

Ideally, we should never have a situation where a YES and NO are mixed on the same page. That means that V8PerContextData::constructorForTypeSlowCase was called for Navigator to install the origin trials on that page, but not for Window.

V8PerContextData::constructorForType for window objects is called in WindowProxy::initialize. I'm assuming that:

- WindowProxy::initialize is called for each context.
- A new context is created for each navigation.
- Therefore, V8PerContextData::constructorForType is called for each navigation.

However, my assumption should be wrong somewhere.

Dcheng is suspecting that WindowProxy::initialize is being called before the parser reaches the meta tag in some scenarios.

Re: #6 -- that would explain it, especially on navigation, if some of the V8 state is being reused, rather than being constructed just-in-time.

I wonder if we can delay WindowProxy::initialize, until we encounter the first actual use of JS in a document. That'll end up being even more important for Feature Policy, since encountering a meta tag would mean that we need to avoid attaching certain attributes.

Would you check at what point WindowProxy::initialize is called when the page navigates? I'm not sure if we can delay WindowProxy::initialize.

That said, it would be pretty easy to delay calling V8PerContextData::constructorForType.

It looks like it should be called from WebFrame::swap() -- that clears out the existing PerContextData and then immediately re-initializes the WindowProxy by calling setGlobals(). I suspect that's happening before the document parsing has started, but I haven't traced it back further than that yet.

That was premature. What I actually see on navigation is this call stack:

#2 0x7fcf5f6028b2 blink::installConditionalFeaturesForModules()
#3 0x7fcf60df980c blink::installConditionalFeatures()
#4 0x7fcf60eb737b blink::V8PerContextData::constructorForTypeSlowCase()
#5 0x7fcf60e1ce4f blink::V8PerContextData::constructorForType()
#6 0x7fcf60edaa7f blink::WindowProxy::initialize()
#7 0x7fcf60eda508 blink::WindowProxy::initializeIfNeeded()
#8 0x7fcf60e32bea blink::ScriptController::windowProxy()
#9 0x7fcf60e32b76 blink::ScriptController::initializeMainWorld()
#10 0x7fcf60e33315 blink::ScriptController::updateDocument()
#11 0x7fcf61726b70 blink::LocalDOMWindow::installNewDocument()
#12 0x7fcf61df8667 blink::DocumentLoader::createWriterFor()
#13 0x7fcf61df8301 blink::DocumentLoader::ensureWriter()
#14 0x7fcf61df6a15 blink::DocumentLoader::commitData()
#15 0x7fcf61df8d93 blink::DocumentLoader::processData()
#16 0x7fcf61df8c06 blink::DocumentLoader::dataReceived()
#17 0x7fcf6167de35 blink::RawResource::appendData()
#18 0x7fcf616acce9 blink::ResourceLoader::didReceiveData()
#19 0x7fcf73089088 content::WebURLLoaderImpl::Context::OnReceivedData()
#20 0x7fcf730899c3 content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData()
#21 0x7fcf730171bf content::ResourceDispatcher::OnReceivedData()

So ScriptController::updateDocument() is re-initializing the WindowProxy object even before FrameLoader::didBeginDocument is called, so we won't even have a chance to examine the HTTP header stream, let alone the output of the document parser.


By contrast, if the page is the first page loaded into the renderer, the stack looks like this:

#2 0x7f60ec2738b2 blink::installConditionalFeaturesForModules()
#3 0x7f60eda6a80c blink::installConditionalFeatures()
#4 0x7f60edb2837b blink::V8PerContextData::constructorForTypeSlowCase()
#5 0x7f60eda8de4f blink::V8PerContextData::constructorForType()
#6 0x7f60edb4ba7f blink::WindowProxy::initialize()
#7 0x7f60edb4b508 blink::WindowProxy::initializeIfNeeded()
#8 0x7f60edaa3bea blink::ScriptController::windowProxy()
#9 0x7f60ee3aebaf blink::LocalFrame::windowProxy()
#10 0x7f60edaff814 blink::toV8ContextEvenIfDetached()
#11 0x7f60edaff6d0 blink::toV8Context()
#12 0x7f60edabbd15 blink::ScriptState::forWorld()
#13 0x7f60edabbc95 blink::ScriptState::forMainWorld()
#14 0x7f60edaa4e30 blink::ScriptController::evaluateScriptInMainWorld()
#15 0x7f60edaa5104 blink::ScriptController::executeScriptInMainWorld()
#16 0x7f60ee087135 blink::ScriptLoader::doExecuteScript()
#17 0x7f60ee085edf blink::ScriptLoader::executeScript()
#18 0x7f60ee084ad2 blink::ScriptLoader::prepareScript()
#19 0x7f60ee5eb7b8 blink::HTMLScriptRunner::runScript()
#20 0x7f60ee5eb470 blink::HTMLScriptRunner::execute()
#21 0x7f60ee5b8e9f blink::HTMLDocumentParser::runScriptsForPausedTreeBuilder()
#22 0x7f60ee5bb4e8 blink::HTMLDocumentParser::processTokenizedChunkFromBackgroundParser()
#23 0x7f60ee5b8a8f blink::HTMLDocumentParser::pumpPendingSpeculations()
#24 0x7f60ee5b85d5 blink::HTMLDocumentParser::resumeParsingAfterYield()
#25 0x7f60ee5dc33d blink::HTMLParserScheduler::continueParsing()

The WindowProxy is initialized much later, only when the script is about to be executed.

Yea, there are a number of ways that can trigger initialization of the context before the <meta> tag is read. One of them is updateDocument(): if the frame was associated with an initialized context in the past, we immediately call updateDocument() to make sure the cached Document wrapper is not stale.

I have a CL that tries to fix that particular problem already (for another reason). However, a content script that inserts script at document_start would also trigger similar issues.

I took a look at https://codereview.chromium.org/2392843002/, and it appears to resolve this issue: meta tags are correctly recognized before the window object is initialized. I can't comment as to it's safety :)

How robust should the <meta> tag mechanism be? The content script case I mentioned would still break. In addition, there are other things that can inject JS at odd timings that cause contexts to be initialized sooner than expected:
- Mojo-in-JS
- Maybe accessibility
- Layout test bindings
- Maybe instant search bindings?

Right now, we strongly encourage the use of HTTP headers for origin trials; Meta tags are usable for developers who don't have control over the HTTP-side of their serving infrastructure, but we tell devs that there are lots of cases where they won't work.

Specifically, we say that it will work if the <meta> tag is encountered in the document <head>, *before* any <link> or <script> tags. If a content script writes a <script> tag into the document before the meta tag, then I'm okay with the context getting initialized at that point, and essentially freezing the set of origin trial features.

Layout tests are handled specially -- we have a test interface which is attached as an origin trial to window.internals, so we're not concerned there.

I'm not sure how accessibility works; I'll take a look; if it runs scripts in the main world, then I might have to revisit how and when we initialize the origin trials. (Anything that runs in an isolated world should be okay; the document's trial tokens aren't going to affect them anyway)

Instant search bindings is a new phrase for me :) I will try to get you an answer on that.

To be clear, I'm not sure if any of the things I listed are problems. I think it's something to be aware of: we make a lot of callbacks from Blink into //content when a document is loading, and there are no prohibitions against running script in the main world: it's highly discouraged, but it's also hard to guarantee that it never happens.

In any case, I'll work on getting my CL fixed and landed.

Just to update, I'm trying to figure out where a good point for lazily initializing contexts is.

Originally, I wanted to try doing it in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/ToV8.cpp?rcl=0&l=30, but that still doesn't fix https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/misc/window-open-then-write.html?rcl=0&l=11.

However, even if I got this to work, that's still not good enough: if a LocalFrame is navigated, the old context will be detached: however, the JS object is already created, so if it not accessed through ToV8 again, then the context won't be properly reinitialized. Blah...

how about we just don't put origin trials on window and document?

or alternatively, require a header for those, no meta tags

Unfortunately, every time an origin trial adds a new interface, rather than just an attribute, the interface itself is supposed to be visible on the global object. It's not that we're adding new methods on window or document, it's that things like StorageManager, BluetoothDevice, USBInterface, etc, are all interfaces which are attached to window (or whatever global scope they apply to).

Maybe we'd still be within the letter of the spec if we didn't expose them, and didn't allow any new constructable interfaces as origin trials; I'll have to reread it to be sure, though.

that leaves if with the option to not support the meta tag :)

I talked with jochen@ offline, and lazily initializing the context is impractical: it'll degrade perf a lot because we'd have to hook every time a v8::Object for a global proxy is touched.

jochen@ also mentioned another interesting point: how would this interact with the work to snapshot the context to lower the cost of WindowProxy::initialize?

If we can remove the <meta>-tag semantics, that's the best option.

If we cannot remove the <meta>-tag semantics, can we do something like this?

- Do not install any origin-trials until </head> is reached.
- When </head> is reached, and there is already a wrapper of window/document/navigator, install the origin-trials at that point.
- Otherwise, dynamically install the origin-trials when the wrapper of window/document/navigator is created.

Regarding the context snapshotting, I don't think it's a problem because my plan is to take the snapshot of the context before any context-dependent things (including origin-trials) are installed.

Maybe we can explain that trials with <meta> tags are restricted om functionality, i.e. no visible interfaces, so don't feature-detect on those. We'd also have to disable any future trials that included constructable interfaces.

> If we cannot remove the <meta>-tag semantics, can we do something like this?
>
> - Do not install any origin-trials until </head> is reached.
> - When </head> is reached, and there is already a wrapper of window/document/navigator, install the origin-trials at that point.
> - Otherwise, dynamically install the origin-trials when the wrapper of window/document/navigator is created.

This would be weirder for developers -- trials would work in <body> scripts, but not <head> scripts -- at least not until the body is parsed.

If we modified it slightly, though, and initialize window/self when </head> or <head><link> or <head><script> were encountered, then I think it would work. I could reinstate that behavior from M54.

From my perspective, it seems like it'd be a little awkward to tie context initialization into the parser like that, but I'm not an expert on parsing. Since I don't work on origin trials directly, would it be OK if I assign this bug to you (iclelland@) to figure out how to resolve this?

+kouhei, a parser expert

Agreed that the <meta>-tag semantic removal is the best option.

> If we modified it slightly, though, and initialize window/self when </head> or <head><link> or <head><script> were encountered, then I think it would work. I could reinstate that behavior from M54.
How is this different from lazy initializing context?

I think the difference is that we're not deliberately delaying any initialization to wait for origin trial tokens. Instead, we're letting the context get initialized exactly as before, and then once we get to the point where we know that no more meta tags can apply, *if the context has already been initialized*, then we add the origin trials on the global object.

This is similar to what we used to do for all origin trial attributes, up to M54: We would queue up the initializations as headers / meta tags were encountered, and actually install them after the context is initialized. That was slow because we had to instantiate every interface during WindowProxy initialization ( crbug.com/637148 ).

What we do now is to install the origin trial attributes on an interface when that interface is first accessed (also lazy, but in a different way). This leads to *this* bug, where the global is constructed well before <meta> tags

I'm proposing reinstating the first behaviour, but *only* for the global object. If a token is encountered, queue it until the context is initialized normally.

IIUC, the proposal is to add a hook to the parser which is signaled when "</head> or <head><link> or <head><script>" condition is met.
This is possible, but add complexity to the parser code. We have a similar tracking for CSP which is non-trivial, and would like to avoid new one. (We just removed one for AppCache too.)

Fixed the empty status.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a251a4b78a1f173a03ac2f0239c8a48216eed92

commit 8a251a4b78a1f173a03ac2f0239c8a48216eed92
Author: iclelland <iclelland@chromium.org>
Date: Fri Nov 04 02:42:17 2016

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 

Review-Url: https://codereview.chromium.org/2458183002
Cr-Commit-Position: refs/heads/master@{#429772}

[modify] https://crrev.com/8a251a4b78a1f173a03ac2f0239c8a48216eed92/third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp
[modify] https://crrev.com/8a251a4b78a1f173a03ac2f0239c8a48216eed92/third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h
[modify] https://crrev.com/8a251a4b78a1f173a03ac2f0239c8a48216eed92/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
[modify] https://crrev.com/8a251a4b78a1f173a03ac2f0239c8a48216eed92/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
[modify] https://crrev.com/8a251a4b78a1f173a03ac2f0239c8a48216eed92/third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h

Your change meets the bar and is auto-approved for M55 (branch: 2883)

Merged into M55 (branch 2883) as
https://chromium.googlesource.com/chromium/src/+/efad4ae5482fb984e072f615df678b72a2d53b08

Review URL: https://codereview.chromium.org/2473413002/

Not sure why bugdroid didn't pick this up at the time.