Feature Policy layout tests failing on Site Isolation bots |
|||||||||||
Issue descriptionStarting 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.
,
Apr 27 2017
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).
,
Apr 27 2017
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.
,
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
,
Apr 28 2017
+iclelland@ in case these layout test failures are related to issue 716478
,
Apr 28 2017
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.
,
Apr 28 2017
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)
,
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
,
Apr 28 2017
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
,
Apr 28 2017
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.
,
Apr 28 2017
s/secondary test windows/secondary test renderers/ in #c10...
,
Apr 28 2017
+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.
,
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.
,
May 1 2017
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.
,
May 1 2017
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?
,
May 1 2017
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.
,
May 1 2017
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).
,
May 1 2017
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.
,
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
,
May 3 2017
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.
,
May 3 2017
,
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
,
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
,
May 24 2017
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
,
May 24 2017
Yes. Will do!
,
May 24 2017
https://chromium-review.googlesource.com/#/c/513447/ Can someone please quickly lgtm and I will land it asap? Thanks
,
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
,
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?
,
May 25 2017
Will do. Thanks https://codereview.chromium.org/2905103002
,
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
,
Jun 6 2017
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
,
Jun 6 2017
f2.src = src3 f1.src = src1 f1.src = src2 ==> PASS PASS FAIL f2.src = src3 f1.src = src2 f1.src = src1 ==> PASS FAIL PASS
,
Jun 6 2017
Nice find! alexmos@ is OOO until Wednesday, but he (or lukasza@?) might be able to help with the replication logic.
,
Jun 6 2017
Also suspecting same root cause for Issue 718160 ( https://crbug.com/718160 , Feature policy container policy not replicated to remote frames).
,
Jun 6 2017
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
,
Jun 12 2017
,
Jun 27 2017
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.
,
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
,
Jun 28 2017
Thanks Ian =D
,
Jun 30 2017
Yes, thanks for the fix! It's great to have this working.
,
Mar 1 2018
,
Mar 1 2018
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by lukasza@chromium.org
, Apr 27 2017