Issue metadata
Sign in to add a comment
|
Origin trial binding race condition on WebVR sample pages |
||||||||||||||||||||||||
Issue descriptionChrome 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.
,
Jul 24 2017
,
Jul 24 2017
,
Jul 24 2017
,
Jul 24 2017
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.
,
Jul 25 2017
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.
,
Jul 25 2017
The bisect confirms that https://codereview.chromium.org/2970003002 is the change that introduced the regression.
,
Jul 25 2017
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.
,
Jul 25 2017
Updating summary, adding origin trial component and assigning to iclelland per the announcement sent out about the origin trial binding autogeneration.
,
Jul 25 2017
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.
,
Jul 25 2017
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.
,
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
,
Jul 26 2017
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.
,
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
,
Jul 27 2017
,
Jul 28 2017
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.
,
Jul 29 2017
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
,
Jul 31 2017
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
,
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
,
Jul 31 2017
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.
,
Aug 1 2017
Verified fixed in 61.0.3163.27
,
Aug 1 2017
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).
,
Jul 4
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by billorr@chromium.org
, Jul 24 2017