Stop filtering IPC messages coming from swapped-out frames. |
||||
Issue descriptionInitial experiments (*) show that only the following 7 IPCs are affected by the SwappedOutMessages filtering class when running content_browsertests (with and without --site-per-process): Allowed messages: - ViewHostMsg_Focus - ViewHostMsg_RouteCloseEvent - ViewHostMsg_ShowWidget Disallowed / filtered-out messages: - ViewHostMsg_HasTouchEventHandlers - ViewHostMsg_HideValidationMessage - ViewHostMsg_UnlockMouse - ViewHostMsg_UpdateZoomLimits So - let's use this bug to try to remove the filtering altogether. (*) https://groups.google.com/a/chromium.org/d/topic/site-isolation-dev/_2vZXguQRu0/discussion
,
Jul 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/00043bdefebe1d854b0cffbd1d35d90c0091057c commit 00043bdefebe1d854b0cffbd1d35d90c0091057c Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Sat Jul 22 06:49:05 2017 No more filtering out of IPC messages in swapped-out state. Bug: 745091 Change-Id: I869a71ea38b0bd7e8631ef7b78cdb75a8468520e Reviewed-on: https://chromium-review.googlesource.com/568740 Commit-Queue: Nasko Oskov <nasko@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#488859} [modify] https://crrev.com/00043bdefebe1d854b0cffbd1d35d90c0091057c/content/browser/renderer_host/render_view_host_impl.cc [modify] https://crrev.com/00043bdefebe1d854b0cffbd1d35d90c0091057c/content/renderer/render_widget.cc
,
Jul 24 2017
Thanks lukasza@! This is a fantastic reduction of complexity. nasko@, can you take a look for any fallout from this change, and maybe check over the places that RVH delegates IPC message handling, as we discussed? Just want to make sure we don't cause unexpected behavior changes. (I guess one option would be to file another bug for moving any IPC message handler that listens to messages via RVH?)
,
Jul 24 2017
Oh, actually, I just checked the Site Isolation Linux FYI bot, and it looks like we started failing some tests when this landed: https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Linux/builds/19116
,
Jul 25 2017
I've reverted the change to remove the filtering in https://chromium-review.googlesource.com/c/584034. The reason layout tests were failing is that in Site Isolation mode we send two more IPCs (ShellViewHostMsg_TextDump and ShellViewHostMsg_TestFinished) through the swapped out RenderViewHost, which end up altering the test expectations. We need to fix those before retrying to remove the filtering logic. I'm marking the bug as available, since I won't be able to work on it in the immediate future, so anyone should feel free to pick it up.
,
Jul 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f97e8b046e322b92d4c953f78783161a9a8b8b26 commit f97e8b046e322b92d4c953f78783161a9a8b8b26 Author: Nasko Oskov <nasko@chromium.org> Date: Tue Jul 25 02:45:42 2017 Revert "No more filtering out of IPC messages in swapped-out state." This reverts commit 00043bdefebe1d854b0cffbd1d35d90c0091057c. Reason for revert: Removing the swapped out IPC filtering resulted in some layout tests to fail with Site Isolation. The ShellViewHostMsg_TextDump and ShellViewHostMsg_TestFinished IPCs are no longer filtered, which results in different test expectations. The failing tests are: http/tests/security/mixedContent/insecure-prefetch-in-main-frame.html http/tests/security/mixedContent/blob-url-in-iframe.html http/tests/inspector/change-iframe-src.html Original change's description: > No more filtering out of IPC messages in swapped-out state. > > Bug: 745091 > Change-Id: I869a71ea38b0bd7e8631ef7b78cdb75a8468520e > Reviewed-on: https://chromium-review.googlesource.com/568740 > Commit-Queue: Nasko Oskov <nasko@chromium.org> > Reviewed-by: Nasko Oskov <nasko@chromium.org> > Cr-Commit-Position: refs/heads/master@{#488859} TBR=nasko@chromium.org,lukasza@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 745091 Change-Id: Iaa7affcc1439ea0c6cf1c56f2a99bf82aa612d11 Reviewed-on: https://chromium-review.googlesource.com/584034 Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#489197} [modify] https://crrev.com/f97e8b046e322b92d4c953f78783161a9a8b8b26/content/browser/renderer_host/render_view_host_impl.cc [modify] https://crrev.com/f97e8b046e322b92d4c953f78783161a9a8b8b26/content/renderer/render_widget.cc
,
Aug 22 2017
Argh... I should have included linux_site_isolation trybot. I've looked at http/tests/security/mixedContent/blob-url-in-iframe.html failure and it seems that a suprious/unwanted ShellViewHostMsg_InitiateLayoutDump is sent from the following callstack: #1 0x0000005656da content::BlinkTestRunner::CaptureDump() #2 0x000000565494 content::BlinkTestRunner::TestFinished() #3 0x7f617dcc2999 test_runner::TestRunner::WorkQueue::ProcessWorkSoon() #4 0x7f617dcc4fb8 test_runner::TestRunner::LocationChangeDone() #5 0x7f617dcc4efc test_runner::TestRunner::tryToClearTopLoadingFrame() #6 0x7f61804b2413 blink::ProgressTracker::ProgressCompleted() #7 0x7f61804a03c9 blink::FrameLoader::DidFinishNavigation() #8 0x7f61804895bd blink::DocumentLoader::LoadFailed() #9 0x7f618049ff2a blink::FrameLoader::StopAllLoaders() #10 0x7f617ff11c5a blink::LocalFrame::Detach() #11 0x7f617fea34c9 blink::WebFrame::Swap() #12 0x7f6184fc2d23 content::RenderFrameImpl::OnSwapOut()
,
Aug 22 2017
It seems that WebFrameTestClient::DidStopLoading or TestRunner::LocationChangeDone should stop further work if the underlying frame is in the process of being swapped out (because in this case the test shouldn't finish until the newly swapped-in frame finishes loading). OTOH, the concept of a swapped out (or detached) frame is not exposed from Frame::IsAttached to the public blink layer (i.e. there is no WebFrame::IsAttached or WebFrame::IsSwapped).
,
Aug 23 2017
Hmmm... a naive fix @ https://chromium-review.googlesource.com/c/chromium/src/+/627518 helps the following tests ... * http/tests/security/mixedContent/insecure-prefetch-in-main-frame.html * http/tests/security/mixedContent/blob-url-in-iframe.html ... but doesn't fix or breaks a few other layout tests: * http/tests/inspector/change-iframe-src.html * http/tests/inspector/resource-parameters-ipv6.html
,
Aug 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/302c0de127488a9904c6b78c496c7d1dab7c7c00 commit 302c0de127488a9904c6b78c496c7d1dab7c7c00 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Wed Aug 30 18:55:48 2017 Don't open DevTools when replicating layout test config to another renderer. Layout tests harness is able to replicate *changes* of test config that happen during test runtime. On the other hand, replication of changes only covers changes done on top of the *baseline* of the initial test configuration. Therefore, the harness separately replicates the initial test configuration (i.e. the "baseline") via LayoutTestControl::ReplicateTestConfiguration. The problem with the approach above is that some aspects of layout test config need to be replicated into each renderer (for example dump mode - pixel dump VS text dump, etc.), while other aspects should only be done *once* per test (for example showing of DevTools). Before this CL, this problem was magically/implicitly/accidentally dealt with by filtering of ShellViewHostMsg_ShowDevTools when replicating the test config to a renderer hosting a swapped-out RenderView). After this CL, this problem is explicitly dealt with, by explicitly passing an |bool initial_configuration| parameter to the ConfigureForTestWithURL method. Bug: 745091 Change-Id: I7db7b887949650e94e7059ccbdd39a2c63ad0800 Reviewed-on: https://chromium-review.googlesource.com/639512 Commit-Queue: Ćukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#498554} [modify] https://crrev.com/302c0de127488a9904c6b78c496c7d1dab7c7c00/content/shell/renderer/layout_test/blink_test_runner.cc [modify] https://crrev.com/302c0de127488a9904c6b78c496c7d1dab7c7c00/content/shell/renderer/layout_test/blink_test_runner.h [modify] https://crrev.com/302c0de127488a9904c6b78c496c7d1dab7c7c00/content/shell/test_runner/test_interfaces.cc [modify] https://crrev.com/302c0de127488a9904c6b78c496c7d1dab7c7c00/content/shell/test_runner/test_interfaces.h [modify] https://crrev.com/302c0de127488a9904c6b78c496c7d1dab7c7c00/content/shell/test_runner/web_test_interfaces.cc [modify] https://crrev.com/302c0de127488a9904c6b78c496c7d1dab7c7c00/content/shell/test_runner/web_test_interfaces.h
,
Aug 31
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
||||
►
Sign in to add a comment |
||||
Comment 1 by lukasza@chromium.org
, Jul 18 2017