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

Issue 672546 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 477150



Sign in to add a comment

virtual/feature-policy/http/tests/feature-policy/fullscreen-enabledforself.php fails with --site-per-process

Project Member Reported by lukasza@chromium.org, Dec 8 2016

Issue description

Speculative repro steps:
  $ third_party/WebKit/Tools/Scripts/run-webkit-tests \
      --additional-drt-flag=--site-per-process \
      -t gn -v virtual/feature-policy/http/tests/feature-policy/fullscreen-enabledforself.php

First failing build:
https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Win/builds/17217
 
Owner: lunalu@chromium.org
Status: Assigned (was: Untriaged)
lunalu@: I'll try to repro locally and bisect to confirm, but the first failing build has the following fullscreen-related CL that adds the failing test: r436651.  Could you please take a look?
Cc: foolip@chromium.org
+foolip@ to CC in case their r436670 is related
I've confirmed that the test fails with --site-per-process from the start (i.e. from lunalu@'s r436651).  The failure is:

FAIL Fullscreen enabled for self on URL: resources/feature-policy-fullscreen.html with allowfullscreen = true assert_true: Document.fullscreenEnabled: expected true got false
FAIL Fullscreen enabled for self on URL: http://localhost:8000/feature-policy/resources/feature-policy-fullscreen.html with allowfullscreen = true assert_true: Document.fullscreenEnabled: expected true got false

I'll put together a CL to disable the test / set test expectations in third_party/WebKit/LayoutTests/FlagExpectations/site-per-process.
Do you know why it is failing? 
It should behave the same as the previous branch when feature policy flag is turned off. 
Cc: iclell...@chromium.org
I don't know why the test is failing :-(  I am not familiar with the test and the fullscreen code and was hoping you could take a closer look.  I am guessing that there is a state local to a single renderer process (either in product or test code) and therefore the state gets out of sync when multiple renderer processes / out-of-process-iframes are present.  This is just a guess though.
Looking at this now.

I will be travelling from today till next Monday. But I will try my best to work on this.
iclelland@ didn't think so, but it's possible there's something related to https://codereview.chromium.org/2499373002#msg95 that's causing the failure.
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 8 2016

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

commit 3ee7e26d7077c56d4a03a422f07eb1a0d53e1c51
Author: lukasza <lukasza@chromium.org>
Date: Thu Dec 08 20:38:36 2016

Expectations for fullscreen-enabledforself.php run with --site-per-process.

BUG= 672546 
NOTRY=true

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

[modify] https://crrev.com/3ee7e26d7077c56d4a03a422f07eb1a0d53e1c51/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Components: Blink>FeaturePolicy
Cc: -iclell...@chromium.org lunalu@chromium.org
Owner: iclell...@chromium.org
Status: Started (was: Assigned)
This is also happening in with PaymentRequest in  crbug.com/683199 ; I suspect that there is a bug in the feature policy framework around checking policies in remote parent frames.
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 24 2017

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

commit 6fafd01c2b16b4c4b23e3fb83c80722f36a3bb14
Author: iclelland <iclelland@chromium.org>
Date: Tue Jan 24 14:11:55 2017

Fix Feature Policy header replication in site-isolation.

The ToWebParsedFeaturePolicy method was not constructing the
WebParsedFeaturePolicy struct correctly, so remote frames would only receive
empty whitelists when the FP header was replicated from their local process.

BUG= 683199 , 672546 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/6fafd01c2b16b4c4b23e3fb83c80722f36a3bb14/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/6fafd01c2b16b4c4b23e3fb83c80722f36a3bb14/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Status: Fixed (was: Started)
Project Member

Comment 14 by bugdroid1@chromium.org, Jan 24 2017

Labels: merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2cd339129aa18e64658b1f15f37a53789dab5b68

commit 2cd339129aa18e64658b1f15f37a53789dab5b68
Author: Ian Clelland <iclelland@google.com>
Date: Tue Jan 24 19:18:05 2017

Fix Feature Policy header replication in site-isolation.

The ToWebParsedFeaturePolicy method was not constructing the
WebParsedFeaturePolicy struct correctly, so remote frames would only receive
empty whitelists when the FP header was replicated from their local process.

BUG= 683199 , 672546 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2644233005
Cr-Commit-Position: refs/heads/master@{#445714}
(cherry picked from commit 6fafd01c2b16b4c4b23e3fb83c80722f36a3bb14)

Review-Url: https://codereview.chromium.org/2653043002 .
Cr-Commit-Position: refs/branch-heads/2987@{#66}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/2cd339129aa18e64658b1f15f37a53789dab5b68/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/2cd339129aa18e64658b1f15f37a53789dab5b68/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Sign in to add a comment