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

Issue 745091 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 733314



Sign in to add a comment

Stop filtering IPC messages coming from swapped-out frames.

Project Member Reported by lukasza@chromium.org, Jul 18 2017

Issue description

Initial 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
 
Blocking: 733314
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Comment 3 by creis@chromium.org, Jul 24 2017

Cc: lukasza@chromium.org
Components: UI>Browser>Navigation
Labels: M-62 OS-All
Owner: nasko@chromium.org
Status: Started (was: Available)
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?)

Comment 4 by creis@chromium.org, 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

Comment 5 by nasko@chromium.org, Jul 25 2017

Owner: ----
Status: Available (was: Started)
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.
Project Member

Comment 6 by bugdroid1@chromium.org, 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

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()
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).
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


Project Member

Comment 10 by bugdroid1@chromium.org, 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

Project Member

Comment 11 by sheriffbot@chromium.org, Aug 31

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
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