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

Issue 852102 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 8
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Autoplay feature policy prevents autoplay with Drive presentations

Project Member Reported by mlamouri@chromium.org, Jun 12 2018

Issue description

STR:
 1. create slides with Google Drive
 2. Insert > Video
 3. Put some YouTube keywords
 4. Press "Select"
 5. Right click on the video
 6. Format Options
 7. Unbundled "Video Playback"
 8. Check "Autoplay when presenting"
 9. Press PRESENT

Expected result: video autoplays
Actual result: video does not autoplay

Note: if closing the tab and re-opening the video seems to autoplay when pressing PRESENT (ie. only step 9 because video is already there).

I tried to debug and for what I can see, the feature policy rejects the frame. However, it seems that the frame is set up as: main frame > iframe (same origin) > youtube. I wonder if this is surfacing a bug in allow=autoplay or feature policy.

Assigning to beccahughes@ to investigate more.
 

Comment 1 by ssaviano@google.com, Jun 19 2018

Labels: Hotlist-Partner-GSuite
Cc: -iclell...@chromium.org beccahughes@chromium.org
Owner: iclell...@chromium.org
It looks like this may be a feature policy bug. We have the following frame tree:

top
 - same origin frame
  - youtube allow="autoplay"

On the YouTube frame we get false from IsFeatureEnabled where this should be true.

Comment 3 by ssaviano@google.com, Jun 22 2018

Any update here? Thanks!
I spent some time yesterday (and today) looking at this; it does appear to be a FP bug, but I'm not sure exactly where it's coming from. I'm going to instrument my local chrome and see if I can tell what's happening.

The request for autoplay is indeed being denied by feature policy; while a similar youtube embed in the same document is able to use it. The middle page, which is same-origin to the top-level page, appears to have the correct policy, but it is not being created correctly in the deepest frame (youtube). Even when trying, in devtools, to reload the frame with 

    frame.src = frame.src

the policy is not correctly constructed. Neither autoplay nor EME are being granted to the frame.
Components: Internals>Sandbox>SiteIsolation
Status: Started (was: Assigned)
This appears to be a site isolation bug (adding Internals>Sandbox>SiteIsolation component), as it does not appear when running chrome with --disable-site-isolation-trials.

The policy is being constructed incorrectly in the innermost (youtube) frame. That frame has two remote parent frames (both docs.google.com), and the middle frame's remote proxy in the youtube renderer has an incorrect policy (it does not have any default-'self' features inherited; as if it were cross-origin to its parent.)

Comment 6 by creis@chromium.org, Jun 28 2018

Cc: alex...@chromium.org
Labels: M-68 M-69 M-67 FoundIn-67
Thanks for spotting this!  I can repro on Windows 67.0.3396.99 Stable and 69.0.3475.0 Canary.

I'm curious about the note that closing and re-opening the tab makes the problem go away.  I've noticed that it also starts working again if you switch to another slide in the presentation and come back to the affected slide.  You can make the bug return by toggling the autoplay checkbox and presenting again.  (This suggests there are some reasonable workarounds for this, though we should obviously aim to get it fixed.)

Ian, do you have a sense for whether the fix will be tricky?  It would be great to target a merge to M68 if possible, but this doesn't seem super concerning if not, given the mitigating factors.  I'm CC'ing alexmos@ in case there are things about replication state to discuss.  Thanks!
(Sorry, Alex, your response came into the wrong mailbox :( )

I've been tracing it to see where the incorrect policy is actually being created -- I've managed to find a simpler repro case (so I should be able to write a test for this). It appears to only happen when the proxy for the same-origin subframe is created inside of an existing SiteInstance.

So if we have the frame tree:

   a.com
 /
b.com

and then add another frame, same-origin with the root:
  a.com
 /     \
b.com   a.com/1

Then in b.com's renderer, the policy for the new frame is created with a null origin first, which acts as cross-origin to the root, and then the origin is changed, but the logic in FeaturePolicy::CreateFromPolicyWithOrigin doesn't recalculate the inherited features, so autoplay is disabled there.

(In the slides case, the reason this is failing is that the "format options" pane contains an existing iframe to https://www.youtube.com, so when you try to present, that renderer already exists, and when the presentation iframe gets constructed, its proxy in the youtube renderer gets the wrong policy, and the actual presentation video can't use autoplay.)

I think this should be reasonably straightforward to fix; this week is probably reasonable. 
(And by Alex, I mean Charlie, of course -- sorry)
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 7

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

commit bd2a91ac57dcfe16ce53877c776843a6cf1f8216
Author: Ian Clelland <iclelland@chromium.org>
Date: Sat Jul 07 05:13:57 2018

Fix feature policy construction in proxies.

When a proxy for a newly-created frame is created in an existing
renderer, the Feature-Policy header and container policy need to be
applied after the origin is set. Previously, setting the proxy's origin
(as the last step in frame navigation) would reuse the feature policy
which was created during frame initialization without taking into
account either the HTTP header or the new origin's relationship to the
parent frame.

This CL ensures that the remote frame stores a copy of the initial
feature policy header, which it can use to reconstruct the correct
policy when the origin is set. The
FeaturePolicy::CreateFromPolicyWithOrigin method, which was used to
replicate an existing policy with an updated origin, is removed, as it
appears to never be the right behaviour.

Bug:  852102 
Change-Id: I3e5137737a5aac102f247aa52ede842f1210f53f
Reviewed-on: https://chromium-review.googlesource.com/1124931
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573149}
[modify] https://crrev.com/bd2a91ac57dcfe16ce53877c776843a6cf1f8216/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/bd2a91ac57dcfe16ce53877c776843a6cf1f8216/third_party/blink/common/feature_policy/feature_policy.cc
[modify] https://crrev.com/bd2a91ac57dcfe16ce53877c776843a6cf1f8216/third_party/blink/public/common/feature_policy/feature_policy.h
[modify] https://crrev.com/bd2a91ac57dcfe16ce53877c776843a6cf1f8216/third_party/blink/renderer/core/execution_context/security_context.cc
[modify] https://crrev.com/bd2a91ac57dcfe16ce53877c776843a6cf1f8216/third_party/blink/renderer/core/execution_context/security_context.h
[modify] https://crrev.com/bd2a91ac57dcfe16ce53877c776843a6cf1f8216/third_party/blink/renderer/core/exported/web_remote_frame_impl.cc
[modify] https://crrev.com/bd2a91ac57dcfe16ce53877c776843a6cf1f8216/third_party/blink/renderer/core/exported/web_remote_frame_impl.h

Status: Fixed (was: Started)
Verified on ToT that Drive presentations autoplay correctly with the fix.
Labels: Merge-Request-68
Per comment #6, requesting merge to M68 branch.
Project Member

Comment 12 by sheriffbot@chromium.org, Jul 9

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: abdulsyed@chromium.org
Thanks iclelland@!  Glad you were able to track it down and come up with a test for it.

I can confirm the fix on Windows Canary 69.0.3486.0 as well. r573149 landed in 69.0.3485.0, and I don't see any new crashes related to it.  Seems reasonably safe from my perspective, and worth having in M68 to deal with the regression in Slides.  abdulsyed@, your thoughts?
Labels: -Merge-Review-68 Merge-Approved-68
Approved for M68. Branch:3440. 
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 10

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e08b90b80a64022720d0b1b75c890fb11f27ab44

commit e08b90b80a64022720d0b1b75c890fb11f27ab44
Author: Ian Clelland <iclelland@chromium.org>
Date: Tue Jul 10 18:22:44 2018

Fix feature policy construction in proxies.

When a proxy for a newly-created frame is created in an existing
renderer, the Feature-Policy header and container policy need to be
applied after the origin is set. Previously, setting the proxy's origin
(as the last step in frame navigation) would reuse the feature policy
which was created during frame initialization without taking into
account either the HTTP header or the new origin's relationship to the
parent frame.

This CL ensures that the remote frame stores a copy of the initial
feature policy header, which it can use to reconstruct the correct
policy when the origin is set. The
FeaturePolicy::CreateFromPolicyWithOrigin method, which was used to
replicate an existing policy with an updated origin, is removed, as it
appears to never be the right behaviour.

TBR=iclelland@chromium.org

(cherry picked from commit bd2a91ac57dcfe16ce53877c776843a6cf1f8216)

Bug:  852102 
Change-Id: I3e5137737a5aac102f247aa52ede842f1210f53f
Reviewed-on: https://chromium-review.googlesource.com/1124931
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#573149}
Reviewed-on: https://chromium-review.googlesource.com/1131926
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#635}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/e08b90b80a64022720d0b1b75c890fb11f27ab44/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/e08b90b80a64022720d0b1b75c890fb11f27ab44/third_party/blink/common/feature_policy/feature_policy.cc
[modify] https://crrev.com/e08b90b80a64022720d0b1b75c890fb11f27ab44/third_party/blink/public/common/feature_policy/feature_policy.h
[modify] https://crrev.com/e08b90b80a64022720d0b1b75c890fb11f27ab44/third_party/blink/renderer/core/execution_context/security_context.cc
[modify] https://crrev.com/e08b90b80a64022720d0b1b75c890fb11f27ab44/third_party/blink/renderer/core/execution_context/security_context.h
[modify] https://crrev.com/e08b90b80a64022720d0b1b75c890fb11f27ab44/third_party/blink/renderer/core/exported/web_remote_frame_impl.cc
[modify] https://crrev.com/e08b90b80a64022720d0b1b75c890fb11f27ab44/third_party/blink/renderer/core/exported/web_remote_frame_impl.h

Thanks for the merge, iclelland@!

Sign in to add a comment