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

Issue 716085 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 732003



Sign in to add a comment

Feature Policy layout tests failing on Site Isolation bots

Project Member Reported by nasko@chromium.org, Apr 27 2017

Issue description

Starting with build 18887 on Site Isolation Win, the bot started failing few vibrate layout tests (https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Win/builds/18887).

It looks like https://codereview.chromium.org/2814153002 (in the blame range of that build) moved the test around and changed some of the test config.

lunalu@, could you please look into this? I will disable these tests for the time being on the Site Isolation bots, but it will be very useful to have them re-enabled quickly.
 
To try to repro locally, it should be sufficient to add --additional-drt-flag=--site-per-process when running Tools/Scripts/run-webkit-tests
Cc: lukasza@chromium.org
Looking at https://storage.googleapis.com/chromium-layout-test-archives/Site_Isolation_Linux/16140/layout-test-results/results.html, I see that sometimes a crash happens in content_shell!OnLayoutDumpResponse [blink_test_controller.cc : 775 + 0x8]:

void BlinkTestController::OnLayoutDumpResponse(RenderFrameHost* sender,
                                               const std::string& dump) {
  // Store the result.
  auto pair = frame_to_layout_dump_map_.insert(
      std::make_pair(sender->GetFrameTreeNodeId(), dump));
  bool insertion_took_place = pair.second;
  DCHECK(insertion_took_place);   <--- HERE

OTOH, sometimes the test fails with a text diff that seems as if the test didn't get configured wrt layout dump flavors it cares about.

Based on the above, maybe there is a race in the test / the test tries to finish twice?  (OTOH, the layout tests harness probably should be hardened against this).

Comment 3 by lunalu@chromium.org, Apr 27 2017

Cc: binlu@chromium.org
Not sure if this has anything to do with your recent change in vibrate tests Bin (though I strongly doubt that). Just thought I should keep you in this loop still.
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 27 2017

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

commit 94ad18d772d3d2fbf8c084b93af70502432a4256
Author: nasko <nasko@chromium.org>
Date: Thu Apr 27 20:29:01 2017

Disable failing/crashing layout tests on Site Isolation bots.

These tests are failing consistently on the Site Isolation FYI bots.
Disabling them while they are investigated and fixed.

BUG= 710098 , 716085 

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

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

Cc: iclell...@chromium.org
+iclelland@ in case these layout test failures are related to  issue 716478 

Comment 6 by nasko@chromium.org, Apr 28 2017

Summary: Feature Policy layout tests failing on Site Isolation bots (was: http/tests/feature-policy-experimental-features/vibrate-disabled.php failing on Site Isolation bots)
It looks like a few more feature policy tests are failing on our FYI bots. I will disable them too and I'm changing the title to be a bit more generic.
https://chromium-review.googlesource.com/c/490466/ is out to fix  issue 716748 .

The crashes I see in the build log don't like they're related -- "you are querying canExecuteScripts on a non contextDocument."

I'll try to reproduce locally with and without that fix, just to see if I can tell what's going on.

(Is this windows-only? The linux site-isolation trybots all seem to pass consistently)
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 28 2017

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

commit e752d174541b2992f923c19722d56d6e0c293bfd
Author: nasko <nasko@chromium.org>
Date: Fri Apr 28 20:35:14 2017

Add more Feature Policy tests to be disabled on Site Isolation FYI bots

This CL adds a few more layout tests that are failing on the Site
Isolation FYI bots.

BUG= 716085 

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

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

RE: Is this windows-only?

No.  I see some failures on the Site Isolation Linux FYI bot (*) and I can also repro locally:

$ DISPLAY=:20 third_party/WebKit/Tools/Scripts/run-webkit-tests -v -t gn --additional-drt-flag=--site-per-process --no-retry-failures http/tests/feature-policy/payment-allowed-by-container-policy-relocate.html --iterations=20

(*) Example: https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Linux/builds/16165

The problem here is that multiple secondary test windows are ~simultenaously calling into BlinkTestRunner::TestFinished.  The layout tests harness is not prepared for this and crashes - I've uploaded a fix for the crash at https://codereview.chromium.org/2852753003.

Note that even with the crash fixed, the simultenaous/racey test finish is leading to test flakiness.  AFAICT testharnessreport.js is calling testRunner.notifyDone 3 times.
s/secondary test windows/secondary test renderers/ in #c10...
Cc: tkent@chromium.org
+tkent@ - could you please share guidelines for usage of testharnessreport.js?  I see that you've added the following comment in r202422:

            // This function might not be the last 'completion callback', and
            // another completion callback might generate more results.  So, we
            // don't dump the results immediately.
            setTimeout(done, 0);

and I wonder if you have an opinion on the desired behavior when testharnessreport.js is included from multiple frames (including OOPIFs) as seems to be the case here (I've been testing with http/tests/feature-policy/payment-allowed-by-container-policy-relocate.html).  Your comment above seems to suggest that we want to support such case (and wait for results from all (?) frames before continuing), but I think that the synchronization mechanism used (setTimeout(0, done)) breaks down in presence of OOPIFs.

Comment 13 by tkent@chromium.org, Apr 30 2017

#12,

We shouldn't load multiple testharnessreport.js in a single test.
The comment is for a case where a test calls add_completion_callback() not from testhanessreport.js.

Status: Started (was: Untriaged)
Thanks for the comments above. Really appreciate it!

It seems like the problem is caused by loading multiple testharnessreport.js in a single test (each iframe loads its own testharnessreport.js in the test). 

I will fix this by either:
    1. only test with one iframe per test (i.e, split the tests), or 
    2. find a way to avoid using testharnessreport.js in the current test frame.

I tried to reproduce the crash, seems like only able to reproduce the container policy tests with relocate. Although I am still getting stderr ("render_frame_host_impl.cc(2395)] OnDidStopLoading was called twice") in various tests (though, I suspect that is a different issue).

Has anyone able to reproduce the crash on non-container-policy-relocate tests?
Re #15, I think the "OnDidStopLoading called twice bug" is getting taken care of by crbug.com/466089.

Focusing on the crash instead of the stderr here. 
RE: #c15

FWIW, I can locally repro the crash with another test:
$ DISPLAY=:20 third_party/WebKit/Tools/Scripts/run-webkit-tests -v -t gn --additional-drt-flag=--site-per-process --no-retry-failures http/tests/feature-policy-experimental-features/vibrate-disabled.php --iterations=10
...
[1/10] http/tests/feature-policy-experi...al-features/vibrate-disabled.php failed (content_shell crashed [pid=116092])
...
All 10 tests ran as expected (0 passed, 10 didn't).
Thanks lukasza@

I expect all payment* tests to crash too (as they are also loading multiple testharnessreport.js) in one test, but they don't.

But let me try removing testharnessreport.js in relocate* and vibrate* tests first.
Project Member

Comment 19 by bugdroid1@chromium.org, May 3 2017

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

commit 2823efd6fec41c2ab4f47d3b241bc6e4330bb3fc
Author: lukasza <lukasza@chromium.org>
Date: Wed May 03 16:15:36 2017

Gracefully handle tests finishing simultanously in 2+ secondary renderers.

This CL makes the layout tests harness more resiliant against
a situation where testRunner.notifyDone() is called simultaneously in
more than 1 renderer process.  Such situation usually indicates a bug in
the test (e.g. see  https://crbug.com/716085#c13 ), but this shouldn't
cause a crash or other problems in the layout tests harness.

BUG= 716085 

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

[modify] https://crrev.com/2823efd6fec41c2ab4f47d3b241bc6e4330bb3fc/content/shell/renderer/layout_test/blink_test_runner.cc
[add] https://crrev.com/2823efd6fec41c2ab4f47d3b241bc6e4330bb3fc/third_party/WebKit/LayoutTests/http/tests/misc/tests-finishing-simultaneously-expected.txt
[add] https://crrev.com/2823efd6fec41c2ab4f47d3b241bc6e4330bb3fc/third_party/WebKit/LayoutTests/http/tests/misc/tests-finishing-simultaneously.html

Thanks for doing this lukasza@!

At the same time I would like to update you all that I am reformatting all feature policy layout tests in a way that we are not loading multiple testharnessreport in one test. In stead, we will be using postMessages. A CL should be sent out soon. 
Project Member

Comment 22 by bugdroid1@chromium.org, May 5 2017

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

commit 6704e2bcf4ac3f9ecbe7d78378c30f0ec649e6a3
Author: lukasza <lukasza@chromium.org>
Date: Fri May 05 17:35:28 2017

Disable one more failing feature-policy test + remove [ Crash ] expectation.

Site Isolation Win FYI bot sees one more failing feature-policy test
that needs to be disabled in --site-per-process mode:
virtual/feature-policy/http/tests/feature-policy/payment-allowed-by-container-policy-relocate.html

Additionally, after r468997 we should no longer need a [ Crash ]
expectation for the tests that finish simultaneously in multiple frames.

BUG= 716085 
NOTRY=true
TEST=Run all tests covered by the expectations file 10 times.

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

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

Project Member

Comment 23 by bugdroid1@chromium.org, May 24 2017

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

commit 0f559bdba5bf694e3d7ad34cd90b316ac6736d95
Author: lunalu <lunalu@chromium.org>
Date: Wed May 24 15:40:53 2017

Re-enable feature policy layout tests after bug fix.

Tests previously failing on site isolation bots.
Reconstructed all feature policy layout tests:
  - replaced loading multiple testharnessreport.js by introducing postMessage's
  - formatted all tests in more consistent template

BUG= 716085 

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

[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/resources/feature-policy-vibrate-disabled.html
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/resources/feature-policy-vibrate-enabled.html
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/resources/feature-policy-vibrate-relocate.html
[add] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/resources/feature-policy-vibrate.html
[add] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/resources/helper.js
[add] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-no-reload-expected.txt
[add] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-no-reload.html
[add] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-reload.html
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate.html
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy.html
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-disabled-expected.txt
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-disabled.php
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-enabledforall.php
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-enabledforself-expected.txt
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy-experimental-features/vibrate-enabledforself.php
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy-expected.txt
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy-relocate-expected.txt
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy-relocate.html
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-allowed-by-container-policy.html
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled-expected.txt
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled.php
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforall-expected.txt
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforall.php
[add] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself-expected.txt
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/payment-allowed-by-container-policy-relocate.html
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/payment-allowed-by-container-policy.html
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/payment-disabled.php
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/payment-enabledforall.php
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/payment-enabledforself.php
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-relocate.html
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-payment-disabled.html
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-payment-enabled.html
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-payment-relocate-disabled.html
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-payment-relocate-enabled.html
[add] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-payment-relocate.html
[add] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-payment.html
[add] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/helper.js
[add] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/virtual/feature-policy-experimental-features/http/tests/feature-policy-experimental-features/vibrate-allowed-by-container-policy-relocate-and-no-reload-expected.txt
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/virtual/feature-policy-experimental-features/http/tests/feature-policy-experimental-features/vibrate-disabled-expected.txt
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/virtual/feature-policy-experimental-features/http/tests/feature-policy-experimental-features/vibrate-enabledforself-expected.txt
[modify] https://crrev.com/0f559bdba5bf694e3d7ad34cd90b316ac6736d95/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/fullscreen-allowed-by-container-policy-relocate-expected.txt
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/payment-allowed-by-container-policy-expected.txt
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/payment-allowed-by-container-policy-relocate-expected.txt
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/payment-disabled-expected.txt
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/payment-enabledforall-expected.txt
[delete] https://crrev.com/7682c6e584d676e2f002ab15aa8480df383ac0d5/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/payment-enabledforself-expected.txt

I see 2 red tests on Site Isolation Linux bot :-(

https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Linux/builds/16967

* http/tests/feature-policy/payment-allowed-by-container-policy-relocate.html
* http/tests/feature-policy/payment-allowed-by-container-policy.html


Could you please disable the tests again?

I should have pointed out the optional linux_site_isolation trybot - you can trigger this bot manually and you can also include this bot in CQ via CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Yes. 
Will do! 
https://chromium-review.googlesource.com/#/c/513447/

Can someone please quickly lgtm and I will land it asap?

Thanks
Project Member

Comment 27 by bugdroid1@chromium.org, May 24 2017

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

commit 715ce8330af39110b768f8b8dcfcd9e199d10714
Author: Luna Lu <lunalu@chromium.org>
Date: Wed May 24 19:10:45 2017

Disable Feature Policy Layout tests failing on Site Isolation.

Bug:  716085 
Change-Id: I41863a53ce4fb7a69187486b5136d172c7830ba9
Reviewed-on: https://chromium-review.googlesource.com/513447
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Luna Lu <lunalu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474382}
[modify] https://crrev.com/715ce8330af39110b768f8b8dcfcd9e199d10714/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Comment 28 by creis@chromium.org, May 25 2017

lunalu@: Our Site Isolation Win FYI is still red after r474382:
https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Win/builds/19242

Looks like we need to disable the virtual suite versions of the tests as well, for feature-policy and mojo-loading:

* virtual/feature-policy/http/tests/feature-policy/payment-allowed-by-container-policy-relocate.html
* virtual/feature-policy/http/tests/feature-policy/payment-allowed-by-container-policy.html
* virtual/mojo-loading/http/tests/feature-policy/payment-allowed-by-container-policy-relocate.html
* virtual/mojo-loading/http/tests/feature-policy/payment-allowed-by-container-policy.html

Can you disable those as well?
Project Member

Comment 30 by bugdroid1@chromium.org, May 25 2017

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

commit 205f31713c78220176d4ebea0c33d68a662700bd
Author: lunalu <lunalu@chromium.org>
Date: Thu May 25 20:01:02 2017

Disable feature policy payment tests failing on site isolation bot.

BUG= 716085 

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

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

Interesting discovery, tests only fail on iframe f2 if:
1. There exists another cross-origin iframe f1
2. f2 is also an cross-origin iframe (its origin can either be the same f1's or different)

Seems like feature policy is not replicated correctly for cross-origin iframes.

Example:
src1 = same origin
src2 = cross origin 1
src3 = cross origin 2

f1.src = src1
f1.src = src2
f2.src = src2
==> PASS PASS FAIL

f1.src = src1
f1.src = src2
f2.src = src3
==> PASS PASS FAIL 

f1.src = src1
f1.src = src2
f2.src = src1
==> PASS PASS PASS

f1.src = src2
f1.src = src1
f2.src = src3
==> PASS PASS PASS 
f2.src = src3
f1.src = src1
f1.src = src2
==> PASS PASS FAIL

f2.src = src3
f1.src = src2
f1.src = src1
==> PASS FAIL PASS 
Cc: alex...@chromium.org
Nice find!  alexmos@ is OOO until Wednesday, but he (or lukasza@?) might be able to help with the replication logic.
Also suspecting same root cause for  Issue 718160  ( https://crbug.com/718160 , Feature policy container policy not replicated to remote frames).
No idea whether this is causing the issues here, but I noticed a potential bug with feature policy replication in RenderFrameImpl::CreateFrame().  On the path where proxy_routing_id == MSG_ROUTING_NONE (i.e., when creating a new frame in a brand new process), we pass in the replicated_state.container_policy to CreateLocalChild.  But on the other path, where there is a proxy_routing_id (which is used for remote-to-local navigations), we don't pass the container_policy in to WebLocalFrame::CreateProvisional; we only pass in the sandbox flags [1].  This might be problematic if there are pending container policy changes that should take effect on this navigation - see the comment in [2].

[1] https://chromium.googlesource.com/chromium/src/+/b11d7ccc96b8092074b85b884337a4bfe7e7723c/content/renderer/render_frame_impl.cc#1034

[2] https://chromium.googlesource.com/chromium/src/+/b11d7ccc96b8092074b85b884337a4bfe7e7723c/content/browser/frame_host/render_frame_host_impl.cc#1079


Owner: loonyb...@chromium.org
Re: #35 -- That is also coming into play here -- when there is just a single OOPIF, its policy is constructed correctly in the new renderer created for it. However, when a second frame navigates cross-site, and is migrated to that renderer, the container policy is not being set on the provisional frame correctly, and is never updated (by design-- any updates post-navigation are only ever pending), so it ends up with the wrong policy.

Passing the container policy to CreateProvisional appears to correct that. Thanks for bringing that up, Alex.
Project Member

Comment 38 by bugdroid1@chromium.org, Jun 28 2017

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

commit 098da75cd0415c2e21a7ee78e47a75abdfabf461
Author: iclelland <iclelland@chromium.org>
Date: Wed Jun 28 13:46:50 2017

Set frame policy correctly in newly created renderer proxies

Previously, the initial frame policies (sandbox flags and container policy) were being set only in the browser, after the RenderFrameHost and any associated proxies were created, and were not being subsequently replicated to those proxies afterwards. This caused the feature policies constructed in remote frames to be incorrect.

Separately, container policies were not being replicated correctly to provisional render frames, so a frame which performed a cross-site navigation to an existing site instance would have the wrong policy after the navigation was committed.

This patch fixes both of those issues, and reinstates the disabled tests on the site-isolation trybots.

BUG= 716085 , 718160 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/content/browser/frame_host/frame_tree.cc
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/third_party/WebKit/Source/web/WebLocalFrameImpl.h
[modify] https://crrev.com/098da75cd0415c2e21a7ee78e47a75abdfabf461/third_party/WebKit/public/web/WebLocalFrame.h

Status: Fixed (was: Started)
Thanks Ian =D 

Comment 40 by creis@chromium.org, Jun 30 2017

Yes, thanks for the fix!  It's great to have this working.
Cc: loonyb...@chromium.org
 Issue 805791  has been merged into this issue.
Blocking: 732003

Sign in to add a comment