New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 748102 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression
Proj-XR



Sign in to add a comment

Origin trial binding race condition on WebVR sample pages

Project Member Reported by dbbrooks@chromium.org, Jul 24 2017

Issue description

Chrome Version: 61.0.3163.8 - Also happens on ToT canary build
OS: Android N
Device: Pixel
VRCore: 1.7.161000338

This works on Chrome beta 60.0.3112.72

What steps will reproduce the problem?
(1) go to https://webvr.info/samples/03-vr-presentation.html


What is the expected result? Content appears, you can press enter VR and start WebVR presentation.

What happens instead? Black screen on that page. Also if you go to the 360 panorama page, the page is just white.

Note that the website vr.with.in still works.
 
new VRFrameData() is throwing an exception.  If you wait a bit, and enter come commands in console, it seems to work.  This looks like some kind of race condition when starting up javascript.
Components: -Internals>VR Blink>WebVR
Labels: Pri-1 Type-Bug-Regression
Status: Available (was: Untriaged)
Components: -Blink>WebVR Internals>VR
Labels: ReleaseBlock-Stable
Status: Untriaged (was: Available)
Components: -Internals>VR Blink>WebVR
Owner: billorr@chromium.org
Status: Assigned (was: Untriaged)
It looks like possibly the origin-trial features aren't being exposed to the page before the page starts running javascript, so the first time the site calls "new VRFrameData()", it fails.

Starting to bisect to find where this could have been introduced.
https://codereview.chromium.org/2970003002 looks related.

The auto-generated file differs in strings for feature name vs. the hand-crafted file.  In particular, the generated file has "webvr" while the hand-crafted uses "webvr1.1".

see: third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl

continuing the bisect, but I think feature.name could change to feature.origin_trial_feature_name to make the auto-generated and manually created files use the same string.

I don't yet see why the race condition exists though.
Cc: iclell...@chromium.org
The bisect confirms that https://codereview.chromium.org/2970003002 is the change that introduced the regression.

I think we could modify the code that generates ConditionalFeaturesForModules.cpp to read in RuntimeEnabledFeatures.json5, so we can map the idl origin trial names to origin trial feature names.

Comment 9 by amp@chromium.org, Jul 25 2017

Cc: -iclell...@chromium.org
Components: Internals>OriginTrials
Owner: iclell...@chromium.org
Summary: Origin trial bindning race condition on WebVR sample pages (was: WebVR sample pages don't work)
Updating summary, adding origin trial component and assigning to iclelland per the announcement sent out about the origin trial binding autogeneration.
Status: Started (was: Assigned)
Thanks for finding that, billorr@ -- Using the wrong string in InstallPendingConditionalFeatureForModules is definitely an issue here.

I'm not sure that could cause a race, although it may be that the code is already handling a race condition by installing the origin trial attributes via *either* InstallConditionalFeaturesForModules or InstallPendingConditionalFeatureForModules (depending on whether Window is initialized before or after the <meta> tag is encountered), and now InstallPendingConditionalFeatureForModules isn't working properly.

Is the symptom here reproducible reliably? I have a pixel I can install ToT on, so I'll try to repro it.
The symptom is close to 100% reproducible - just go to  https://webvr.info/samples/03-vr-presentation.html, and you'll see a black screen.  If you connect to the device with inspector, there was an exception in the console about "new VRFrameData()" not being defined.

I think the race could be that the window attributes can be installed through multiple paths - I haven't debugged the success case to see why it works, just noticed that if you refresh in the js debugger with a breakpoint on the "new VRFrameData()" line, and you enter some commands in the debugger when the breakpoint hits (like try calling "new VRFrameData()" yourself), then the one in the page succeeds and everything renders correctly.

You may need vr services / daydream installed on pixel to repro.  They can be obtained from the play store if you need them.


Project Member

Comment 12 by bugdroid1@chromium.org, Jul 26 2017

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

commit 44751624a05e171c067d25cbd05d3ee5d28ceacb
Author: Ian Clelland <iclelland@google.com>
Date: Wed Jul 26 17:36:02 2017

Fix Origin Trial code generation.

The string comparison in the InstallPendingConditionalFeatures methods was
comparing the supplied feature (from a decoded token) against the wrong
string. (It used the symbolic name of the feature, rather than the
string defined in RuntimeEnabledFeatures.json5.)

This CL exposes the names defined in that JSON file so that they can be
used by the generated bindings code in ConditionalFeaturesFor{Core|Modules}.cpp,
and then uses them there.

BUG= 748102 

Change-Id: I465921efe91b6739dc0f7b18f78fd9526b0d6179
Reviewed-on: https://chromium-review.googlesource.com/586634
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Jason Chase <chasej@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489684}
[modify] https://crrev.com/44751624a05e171c067d25cbd05d3ee5d28ceacb/third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py
[modify] https://crrev.com/44751624a05e171c067d25cbd05d3ee5d28ceacb/third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl
[modify] https://crrev.com/44751624a05e171c067d25cbd05d3ee5d28ceacb/third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl
[modify] https://crrev.com/44751624a05e171c067d25cbd05d3ee5d28ceacb/third_party/WebKit/Source/bindings/tests/results/core/ConditionalFeaturesForCore.cpp
[modify] https://crrev.com/44751624a05e171c067d25cbd05d3ee5d28ceacb/third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl
[modify] https://crrev.com/44751624a05e171c067d25cbd05d3ee5d28ceacb/third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl

After upgrading to Java 8, I've managed to reproduce this on my pixel -- the symptom I see is that if I open a new tab, and navigate directly to https://webvr.info/samples/03-vr-presentation.html, then the page works normally. If I then reload the page through the UI, then I get a black page.

There appear to be two other issues in the generated code which are causing this:
1. Global objects are not having their origin-trial-enabled attributes installed on the global instance correctly -- Generated code does not match handwritten code for globals.
2. An early return statement in InstallPendingConditionalFeaturesForModules is causing only one of WebVR and GamepadExtensions to be installed, since they have the same OT feature name.

I have one more patch to resolve those issues, and with it applied, the problem appears to be resolved.
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 27 2017

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

commit e698df3fa263c9ee09b6a6a28bbea243b5e2e4f4
Author: Ian Clelland <iclelland@google.com>
Date: Thu Jul 27 16:20:33 2017

Handle global interfaces correctly in Origin Trials binding code.

There were two issues in the InstallPendingConditionalFeaturesFor{Core,Modules}
functions being generated:

1. An early return statement could cause a trial's features to not be
installed, if two features used the same trial name (as happened with
GamepadExtensions and WebVR)

2. Attributes were not being properly installed on the global
interfaces.

This change brings the generated code in line with the handwritten code,
modulo the ordering of trials / interfaces in the code.

BUG= 748102 

Change-Id: I3351dd9d9b8093762595eb45f91b549eb05f1de4
Reviewed-on: https://chromium-review.googlesource.com/587608
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Jason Chase <chasej@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489959}
[modify] https://crrev.com/e698df3fa263c9ee09b6a6a28bbea243b5e2e4f4/third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py
[modify] https://crrev.com/e698df3fa263c9ee09b6a6a28bbea243b5e2e4f4/third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl
[modify] https://crrev.com/e698df3fa263c9ee09b6a6a28bbea243b5e2e4f4/third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl
[modify] https://crrev.com/e698df3fa263c9ee09b6a6a28bbea243b5e2e4f4/third_party/WebKit/Source/bindings/tests/results/core/ConditionalFeaturesForCore.cpp

Summary: Origin trial binding race condition on WebVR sample pages (was: Origin trial bindning race condition on WebVR sample pages)
Labels: Merge-Request-61
Status: Verified (was: Started)
Just tested ToT on my Pixel, and confirmed that the bindings are being properly installed, and survive navigation and reload.

Requesting merge to M61 branch.
Project Member

Comment 17 by sheriffbot@chromium.org, Jul 29 2017

Labels: -Merge-Request-61 Hotlist-Merge-Approved Merge-Approved-61
Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 18 by bugdroid1@chromium.org, Jul 31 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa1d4780c77dec723484f200faf9a699a89910ef

commit fa1d4780c77dec723484f200faf9a699a89910ef
Author: Ian Clelland <iclelland@google.com>
Date: Mon Jul 31 14:32:33 2017

Fix Origin Trial code generation.

The string comparison in the InstallPendingConditionalFeatures methods was
comparing the supplied feature (from a decoded token) against the wrong
string. (It used the symbolic name of the feature, rather than the
string defined in RuntimeEnabledFeatures.json5.)

This CL exposes the names defined in that JSON file so that they can be
used by the generated bindings code in ConditionalFeaturesFor{Core|Modules}.cpp,
and then uses them there.

BUG= 748102 
TBR=iclelland@google.com

(cherry picked from commit 44751624a05e171c067d25cbd05d3ee5d28ceacb)

Change-Id: I465921efe91b6739dc0f7b18f78fd9526b0d6179
Reviewed-on: https://chromium-review.googlesource.com/586634
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Jason Chase <chasej@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#489684}
Reviewed-on: https://chromium-review.googlesource.com/594007
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#150}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/fa1d4780c77dec723484f200faf9a699a89910ef/third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py
[modify] https://crrev.com/fa1d4780c77dec723484f200faf9a699a89910ef/third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl
[modify] https://crrev.com/fa1d4780c77dec723484f200faf9a699a89910ef/third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl
[modify] https://crrev.com/fa1d4780c77dec723484f200faf9a699a89910ef/third_party/WebKit/Source/bindings/tests/results/core/ConditionalFeaturesForCore.cpp
[modify] https://crrev.com/fa1d4780c77dec723484f200faf9a699a89910ef/third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl
[modify] https://crrev.com/fa1d4780c77dec723484f200faf9a699a89910ef/third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl

Project Member

Comment 19 by bugdroid1@chromium.org, Jul 31 2017

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

commit 1aeb7d766b7ee6ea656cf48720c26f88aa788293
Author: Ian Clelland <iclelland@google.com>
Date: Mon Jul 31 14:33:40 2017

Handle global interfaces correctly in Origin Trials binding code.

There were two issues in the InstallPendingConditionalFeaturesFor{Core,Modules}
functions being generated:

1. An early return statement could cause a trial's features to not be
installed, if two features used the same trial name (as happened with
GamepadExtensions and WebVR)

2. Attributes were not being properly installed on the global
interfaces.

This change brings the generated code in line with the handwritten code,
modulo the ordering of trials / interfaces in the code.

BUG= 748102 
TBR=iclelland@google.com

(cherry picked from commit e698df3fa263c9ee09b6a6a28bbea243b5e2e4f4)

Change-Id: I3351dd9d9b8093762595eb45f91b549eb05f1de4
Reviewed-on: https://chromium-review.googlesource.com/587608
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Jason Chase <chasej@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#489959}
Reviewed-on: https://chromium-review.googlesource.com/594008
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#151}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/1aeb7d766b7ee6ea656cf48720c26f88aa788293/third_party/WebKit/Source/bindings/scripts/generate_conditional_features.py
[modify] https://crrev.com/1aeb7d766b7ee6ea656cf48720c26f88aa788293/third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForCore.cpp.tmpl
[modify] https://crrev.com/1aeb7d766b7ee6ea656cf48720c26f88aa788293/third_party/WebKit/Source/bindings/templates/ConditionalFeaturesForModules.cpp.tmpl
[modify] https://crrev.com/1aeb7d766b7ee6ea656cf48720c26f88aa788293/third_party/WebKit/Source/bindings/tests/results/core/ConditionalFeaturesForCore.cpp

Just tested 61.0.3163.20 and it still repros. I thought it would be fixed in this version which has a base branch position of 488528. Is that correct? Thanks. It does work in the latest M62 canary.
Verified fixed in 61.0.3163.27
Okay, that's good (I was going to start checking to see when the beta was cut relative to the merge I made yesterday morning).
Components: Blink>WebXR

Sign in to add a comment