Also note that if after step 4, you click on "Go cross-site (complex page)" and then on "Go cross-site (simple page)" again, the frame keeps displaying the build page even after it was supposed to navigate away from it.  However, if you reload the whole page and then hit "Go cross-site (simple page)", it works correctly.

The behavior is similar.

Since both processes are in the same browsing instance, there's a proxy from one window to the other that already exists. There's also a swapped-out RenderView that gets reused when the subframe navigation happens. My guess is that there's a corner case because the RenderView is being reused.

I won't have time to debug this in the short term, since I need to work on the high dpi issues that are blocking, but I can take a look if no one else picks it up first.

Lucas, would you be able to look at this now that some of the other blocking issues have been resolved?  Alex and I are curious if it's the same bug we're seeing in  issue 487872  comments 4-5.

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

commit 7c3d7bbd510eda2bcfb971f6d166da26bcfe4222
Author: lfg <lfg@chromium.org>
Date: Fri Oct 21 22:17:27 2016

Do not hide the old RenderWidgetHostView when commiting a navigation.

This fixes an issue where the old RenderView is reused by a new remote
subframe. We shouldn't be leaking resources, because now we are already
hiding the unused RenderView in the renderer, when the local main frame
gets detached and the WebViewFrameWidget is closed.

BUG= 638375 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://chromiumcodereview.appspot.com/2415973002
Cr-Commit-Position: refs/heads/master@{#426913}

[modify] https://crrev.com/7c3d7bbd510eda2bcfb971f6d166da26bcfe4222/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/7c3d7bbd510eda2bcfb971f6d166da26bcfe4222/content/browser/site_per_process_browsertest.cc

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

commit 4b746a0ef62cc611f7b79b4cc6a2d7cebb982f3e
Author: lfg <lfg@chromium.org>
Date: Mon Oct 24 17:24:58 2016

Revert of Do not hide the old RenderWidgetHostView when commiting a navigation. (patchset #2 id:20001 of https://codereview.chromium.org/2415973002/ )

Reason for revert:
Caused a regression on Mac.

BUG= 658688 

Original issue's description:
> Do not hide the old RenderWidgetHostView when commiting a navigation.
>
> This fixes an issue where the old RenderView is reused by a new remote
> subframe. We shouldn't be leaking resources, because now we are already
> hiding the unused RenderView in the renderer, when the local main frame
> gets detached and the WebViewFrameWidget is closed.
>
> BUG= 638375 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Committed: https://crrev.com/7c3d7bbd510eda2bcfb971f6d166da26bcfe4222
> Cr-Commit-Position: refs/heads/master@{#426913}

TBR=creis@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 638375 

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

[modify] https://crrev.com/4b746a0ef62cc611f7b79b4cc6a2d7cebb982f3e/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/4b746a0ef62cc611f7b79b4cc6a2d7cebb982f3e/content/browser/site_per_process_browsertest.cc

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

commit 9431efdc7b0146ef74f75ce160f2ba343ca2d550
Author: lfg <lfg@chromium.org>
Date: Wed Dec 14 14:45:26 2016

Destroy the old RenderWidgetHostView when swapping out a main frame.

This fixes an issue where the old RenderView is reused by a new remote
subframe. We shouldn't be leaking resources, because now we are already
hiding the unused RenderView in the renderer, when the local main frame
gets detached and the WebViewFrameWidget is closed.

BUG= 638375 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/9431efdc7b0146ef74f75ce160f2ba343ca2d550/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/9431efdc7b0146ef74f75ce160f2ba343ca2d550/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/9431efdc7b0146ef74f75ce160f2ba343ca2d550/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/9431efdc7b0146ef74f75ce160f2ba343ca2d550/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/9431efdc7b0146ef74f75ce160f2ba343ca2d550/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/9431efdc7b0146ef74f75ce160f2ba343ca2d550/content/public/test/test_renderer_host.cc
[modify] https://crrev.com/9431efdc7b0146ef74f75ce160f2ba343ca2d550/content/public/test/test_renderer_host.h

Verified on today's canary.

Hooray!  I know this was a tricky one-- thanks for working through it.

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

commit 00cf9403c34aea924ffecd736f93e06421e10e15
Author: piman <piman@chromium.org>
Date: Mon Apr 03 17:09:20 2017

Revert "Destroy the old RenderWidgetHostView when swapping out a main frame."

It makes the assumption that a RenderWidget can talk to different
RenderWidgetHostView over its lifetime, but this assumption cannot be satisfied
by the code, and so this causes immediate problems (renderer hangs, tests
failures and flakiness) as well as fundamental consistency problems making further
changes to the code intractable.
Note: this is a manual revert because of conflicts

Original description:
>    Destroy the old RenderWidgetHostView when swapping out a main frame.
>
>    This fixes an issue where the old RenderView is reused by a new remote
>    subframe. We shouldn't be leaking resources, because now we are already
>    hiding the unused RenderView in the renderer, when the local main frame
>    gets detached and the WebViewFrameWidget is closed.
>
>    BUG= 638375 
>    CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
>    Review-Url: https://codereview.chromium.org/2496233003
>    Cr-Commit-Position: refs/heads/master@{#438516}

BUG= 701988 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/00cf9403c34aea924ffecd736f93e06421e10e15/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/00cf9403c34aea924ffecd736f93e06421e10e15/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/00cf9403c34aea924ffecd736f93e06421e10e15/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/00cf9403c34aea924ffecd736f93e06421e10e15/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/00cf9403c34aea924ffecd736f93e06421e10e15/content/browser/web_contents/web_contents_impl.cc

Since the fix for this bug was reverted, I thought it's appropriate to re-open this bug so it gets attention.

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

commit 204f85e5d72d7da5c76d6a38045304c4fb594fd0
Author: Antoine Labour <piman@chromium.org>
Date: Tue Apr 04 19:21:31 2017

Revert "Destroy the old RenderWidgetHostView when swapping out a main frame."

It makes the assumption that a RenderWidget can talk to different
RenderWidgetHostView over its lifetime, but this assumption cannot be satisfied
by the code, and so this causes immediate problems (renderer hangs, tests
failures and flakiness) as well as fundamental consistency problems making further
changes to the code intractable.
Note: this is a manual revert because of conflicts

Original description:
>    Destroy the old RenderWidgetHostView when swapping out a main frame.
>
>    This fixes an issue where the old RenderView is reused by a new remote
>    subframe. We shouldn't be leaking resources, because now we are already
>    hiding the unused RenderView in the renderer, when the local main frame
>    gets detached and the WebViewFrameWidget is closed.
>
>    BUG= 638375 
>    CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
>    Review-Url: https://codereview.chromium.org/2496233003
>    Cr-Commit-Position: refs/heads/master@{#438516}

BUG= 701988 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2786443003
Cr-Commit-Position: refs/heads/master@{#461459}
(cherry picked from commit 00cf9403c34aea924ffecd736f93e06421e10e15)

Review-Url: https://codereview.chromium.org/2798673002 .
Cr-Commit-Position: refs/branch-heads/3029@{#575}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/204f85e5d72d7da5c76d6a38045304c4fb594fd0/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/204f85e5d72d7da5c76d6a38045304c4fb594fd0/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/204f85e5d72d7da5c76d6a38045304c4fb594fd0/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/204f85e5d72d7da5c76d6a38045304c4fb594fd0/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/204f85e5d72d7da5c76d6a38045304c4fb594fd0/content/browser/web_contents/web_contents_impl.cc

Tested the issue on windows 7, Ubuntu 14.04 and Mac 10.12.3 using chrome version 58.0.3029.54 with the steps mentioned in comment #0.
Observed that The frame navigated and shows "csreis.github.io" after clicking "Go cross-site (simple page).
Please find the attached screen cast for the same.

Adding TE-Verified labels.

Thanks,

Deleted: 638375.mp4 565 KB

	638375.mp4 565 KB View Download

Re# 15: The repro steps in the video looks correct, I'm not sure why you don't see the issue on M58. I haven't tried M58, but I can reproduce it in today's canary.

 Issue 718869  has been merged into this issue.

 Issue 753314  has been merged into this issue.

In the test case described in https://bugs.chromium.org/p/chromium/issues/detail?id=753314, adding "rel=noopener" to the link works around the problem. For our purposes, I don't think we have any reason to make use of `window.opener` in links clicked in the iframe.

For people who know Chrome internals better, is "rel=noopener" likely to be a universal workaround for this problem or are there cases in which it probably won't work?

Yes, having rel=noopener should be a universal workaround to this problem, because it'll force the new window to be opened in a different browsing context (see WebContentsImpl::CreateNewWindow, in particular the fact that we create a new SiteInstace if params.opener_suppressed is true).

 Issue 793280  has been merged into this issue.

Let's try to revive this bug and get it fixed.  Let's start by just figuring out what's needed.

If I understand correctly, the ideal fix would be finishing the split of WebView and WebWidget / RenderView and RenderWidget?  Or are there shorter term changes that can help as well?

Note that one workaround is changing to a different tab and back, but this is still a fairly disruptive issue when it happens.

I think we should just fix the Widget/View stuff. The recent refactors that removed the layering between web/core made this a lot easier to complete now that what it was in the past.

Our current thought is that this depends on the view/widget split in issue 478281, as noted above.

There's a chance that we might be able to get this working along the way by identifying which bits of RenderView state are not updated in the case that it starts local, goes remote, and later is given an OOPIF.  There's a good chance that page visibility and other state isn't updated when the OOPIF is added, and we might be able to fix that.  (Comparing the state of RenderView when it's initially created for a remote main frame and an OOPIF against the state of a RenderView that starts local and goes remote would help spot what's missing here.)

Targeting M67 as a Site Isolation launch blocker.

Some updates after debugging this, mostly by looking at the difference between A doing a window.open() of B(A2) (which fails) and A directly navigating to B(A2) (which works).

I think the root cause here is actually Page::IsPageVisible getting out of sync when a RenderView transitions to swapped-out and is later looked up by a LocalFrame subframe when it starts painting.

More details:

When A window.opens B, the new window starts in A's SiteInstance, and when it transitions to B, in RFHM::CommitPending(), we hide the old widget, just before swapping out the old frame:

  // For top-level frames, also hide the old RenderViewHost's view.
  // TODO(creis): As long as show/hide are on RVH, we don't want to hide on
  // subframe navigations or we will interfere with the top-level frame.
  if (is_main_frame &&
      old_render_frame_host->render_view_host()->GetWidget()->GetView()) {
    old_render_frame_host->render_view_host()->GetWidget()->GetView()->Hide();
  }

This eventually sends ViewMsg_WasHidden to the top-level RenderWidget, which follows RenderWidget::OnWasHidden() -> RenderFrameImpl::WasHidden() -> WebViewFrameWidget::SetVisibilityState() -> WebViewImpl::SetVisibilityState() -> Page::SetVisibilityState(), which sets Page::visibility_state_ to hidden.

Then we swap out the old frame, which marks the RenderView as swapped-out.

Later, when B embeds the A2 subframe, we come back to process A and start a remote-to-local navigation.  When it commits, RFHM::CommitPending() will show the subframe's view here:

  // Show the new view (or a sad tab) if necessary.
  bool new_rfh_has_view = !!render_frame_host_->GetView();
  if (!delegate_->IsHidden() && new_rfh_has_view) {
    // In most cases, we need to show the new view.
    render_frame_host_->GetView()->Show();
  }

The WebContents obviously isn't hidden at this point, so the check passes.

This sends a ViewMsg_WasShown to the subframe's widget, which follows a similar path as WasHidden, but  unlike the WebViewFrameWidget case, WebFrameWidgetImpl::SetVisibilityState() does *not* update Page::SetVisibilityState().

Later, when the subframe attempts to start painting, we go down the wrong path on this line:

  layer_tree_view_->SetVisible(GetPage()->IsPageVisible());

where Page::IsPageVisible() is checked via this stack:

#2 0x7f75f76c67eb blink::WebFrameWidgetImpl::SetIsAcceleratedCompositingActive()
#3 0x7f75f76c7dad blink::WebFrameWidgetImpl::SetRootGraphicsLayer()
#4 0x7f75f7c457b2 blink::ChromeClientImpl::AttachRootGraphicsLayer()
#5 0x7f75f7d6a039 blink::PaintLayerCompositor::UpdateIfNeeded()
#6 0x7f75f7d6947f blink::PaintLayerCompositor::UpdateIfNeededRecursiveInternal()
#7 0x7f75f7d68fc5 blink::PaintLayerCompositor::UpdateIfNeededRecursive()
#8 0x7f75f766dcfe blink::LocalFrameView::UpdateLifecyclePhasesInternal()
#9 0x7f75f766d8e7 blink::LocalFrameView::UpdateAllLifecyclePhases()
#10 0x7f75f7c69e1e blink::PageAnimator::UpdateAllLifecyclePhases()
#11 0x7f75f76c52c3 blink::WebFrameWidgetImpl::UpdateLifecycle()
#12 0x7f75fcb72fc2 content::RenderWidget::UpdateVisualState()
#13 0x7f75fb2d0f75 cc::ProxyMain::BeginMainFrame()

So, we hide the LayerTreeView, because we've never updated the Page's visibility state to visible, even though the page is actually visible.

In contrast, when A directly navigates to B(A2), we create the swapped-out RenderView for B, but it's *not* hidden: RVHI::CreateRenderView() sets params.hidden to false:

  params->hidden = is_active_ ? GetWidget()->is_hidden()
                              : GetWidget()->delegate()->IsHidden(); // <-- here

... because the WebContents is not hidden.

So, I can of a few fix options:

1. It's not clear that we need to modify Page::visibility_state_ when a RV transitions to swapped-out.  It seems that should only happen if the WebContents becomes hidden.  We could find a way to avoid doing this, though I think the other work in RenderWidget::SetHidden(), etc. is probably needed.

2. We could have WebFrameWidgetImpl::SetVisibilityState() update Page's visibility as well, to match behavior in WebViewImpl::SetVisibilityState.  I'm not sure whether that's correct though: could separate widgets have different visibility from the page, and from each other, e.g. in A(B,B)?  lfg@, can you confirm?

3. We could simply update Page's visibility (when needed) before we show the subframe's view in CommitPending.  We already have PageMsg_WasShown which seems to do this, so we could just send that message to the B main frame's proxy in process A right before calling render_frame_host_->GetView()->Show().  (It's a bit of a hack to send a PageMsg to just one proxy, but we could think about how to make this cleaner.)  This seems like the easiest fix.

I've tried 2 and 3, and in practice, they both seem to fix this issue in practice, as well as the repro page in issue 810927.  Input events work fine too -- unlike after switching to another tab and coming back, in which case they often still don't work even though the subframe becomes shown (which probably happens thanks to PageMsg_WasShown being sent to same proxy as in option 3).  It's a bit less clear how to do option 1 properly, but just dropping the corresponding WasHidden IPC fixes things too.

1. I wonder if it's an artifact of the old swappedout:// implementation? And presumably we don't want to trigger any begin main frames if there are no local frames anyway and this was one way to do it....

2. We could do this, but yes, not all the web frame widgets will have the same visibility. One of the things that controls visibility of the widget is calculated based on style updates of the remote frame owner: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/RemoteFrameView.cpp?l=190&gs=kythe%253A%252F%252Fchromium%253Flang%253Dc%25252B%25252B%253Fpath%253Dsrc%252Fthird_party%252FWebKit%252FSource%252Fcore%252Fframe%252FRemoteFrameView.cpp%2523jpvAjWBjiUGpDhxk%25252F7ht2CibCIHx5TLgcD0RI%25252FShgxg%25253D&gsn=Hide&ct=xref_usages. So we would have to calculate if global visibility changed and update Page if needed. I wonder if it's possible to get this bug if a subframe is initially hidden by style and then later shown? That would be a reason to do it here instead.

3. I'm not sure if there are edge cases that this wouldn't handle.

1. We want to set the correct visibility state on the top-level RenderWidget, because it's necessary for the compositor to free resources when going into a swapped-out state.

2. Unfortunately, setting the Page's visibility when the frame's visibility change is incorrect, because the frame's visibility can change when setting 'display: none', but this won't change the page's visibility. Doing this will cause other bugs (e.g. triggering the visibilitychange even when it shouldn't).

3. This is kinda hacky, but I think it'll probably work. I can't think of any obvious bugs with this approach.

I'll also add another thing to consider:

4. We could try to fully decouple widget visibility and page visibility. They should be different concepts, and we will need to address this during the WebView/WebWidget split. Basically, we want WebFrameWidgetImpl::SetVisibilityState and WebViewFrameWidget::SetVisibilityState to do the same thing (hide the LayerTreeView). I don't fully remember why this wasn't possible when I initially implemented visibility events for OOPIFs, but that was in 2016, a lot has changed since then.


Overall, maybe option 3 is the best in the short term.

Hmm... OK I agree that #3 sounds the best as a short-term fix then, I forgot that page visibility changes also fire additional events.

Maybe we can try to poke at #4 after that.

Thanks for the additional details!  I'll put together a CL for option 3.  I'll verify that it also works with style changes on the iframe element (e.g., when it's initially hidden and later becomes visible).

I agree with pursuing option 4 after that - let's file a separate bug for that.

I've filed issue 822901 to follow up on option 4.

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

commit b13f850c7ed32867ae9fd93cfe3fe72b8cb4da0b
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Tue Mar 20 17:38:09 2018

Update page visibility for subframes using swapped-out RenderViews.

When a main frame transitions from a local to remote frame, the
RFHM::CommitPending() changes the corresponding RenderView from active
to swapped out, and it also sends a ViewMsg_WasHidden to the
corresponding RenderWidget, which updates visibility state on
blink::Page to hidden.

Later, if that swapped-out RenderView is used by a subframe (e.g.,
when A window.opens B(A2), that subframe is A2), BeginMainFrame for
the subframe would go down an incorrect path because it would think
that the Page is hidden, preventing the subframe from painting.

This CL ensures that before we show the subframe, we update Page
visibility in the subframe's process.

Bug:  638375 
Change-Id: I0893f0c6e58798b641ec8e76b28566a92fe2070f
Reviewed-on: https://chromium-review.googlesource.com/966988
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544425}
[modify] https://crrev.com/b13f850c7ed32867ae9fd93cfe3fe72b8cb4da0b/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/b13f850c7ed32867ae9fd93cfe3fe72b8cb4da0b/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/b13f850c7ed32867ae9fd93cfe3fe72b8cb4da0b/testing/buildbot/filters/viz.content_browsertests.filter

I've verified the fix (r544425) in Mac canary 67.0.3377.0.  It works for both the repro in this bug and the repro page in issue 810927.  Note that in this bug's repro, it's necessary to change step (3) to do a renderer-initiated navigation (e.g., open devtools and set location='http://csreis.github.io/tests/cross-site-iframes.html'), because of r540709.

Should it be marked as Fixed then, based on comment 35?

I'll leave it up to Alex, but I think we can mark this as fixed for now, given that we already created new issues for the follow-ups.

Agreed, I'll mark this as fixed.  I think the only work left here is merge-related.

Speaking of which, requesting merge of r544425 to M66.  It's been verified in canary per #35, and I've also checked the crashes, which don't show anything suspicious so far.  The fix is small and well-contained, and given that it's a blocker for site isolation launch, I think it'd be good to have it for our site isolation trials on M66.

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Re: Comment 39: The reverts are from an earlier attempt to fix this bug.  The fix in r544425 should be safe.

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

commit dc6be55154074cd7663c3cb5023ce5de153d662d
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Mar 22 17:04:02 2018

Remove test workaround for window.open in Android content shell.

This can be removed since  https://crbug.com/823493  has been fixed by
r544591.

Bug:  638375 
Change-Id: Ia07a35fa7725b7b38eeda495b05db35ad6fa6c13
Reviewed-on: https://chromium-review.googlesource.com/974473
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545119}
[modify] https://crrev.com/dc6be55154074cd7663c3cb5023ce5de153d662d/content/browser/site_per_process_browsertest.cc

Approving merge for M66. Branch:3359

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

commit a505653d196fcaf211e14e5f4b147d3f51f01105
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Mar 22 18:36:49 2018

Update page visibility for subframes using swapped-out RenderViews (Merge to M66)

When a main frame transitions from a local to remote frame, the
RFHM::CommitPending() changes the corresponding RenderView from active
to swapped out, and it also sends a ViewMsg_WasHidden to the
corresponding RenderWidget, which updates visibility state on
blink::Page to hidden.

Later, if that swapped-out RenderView is used by a subframe (e.g.,
when A window.opens B(A2), that subframe is A2), BeginMainFrame for
the subframe would go down an incorrect path because it would think
that the Page is hidden, preventing the subframe from painting.

This CL ensures that before we show the subframe, we update Page
visibility in the subframe's process.

TBR=alexmos@chromium.org

(cherry picked from commit b13f850c7ed32867ae9fd93cfe3fe72b8cb4da0b)

Bug:  638375 
Change-Id: I0893f0c6e58798b641ec8e76b28566a92fe2070f
Reviewed-on: https://chromium-review.googlesource.com/966988
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#544425}
Reviewed-on: https://chromium-review.googlesource.com/976334
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#381}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/a505653d196fcaf211e14e5f4b147d3f51f01105/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/a505653d196fcaf211e14e5f4b147d3f51f01105/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/a505653d196fcaf211e14e5f4b147d3f51f01105/testing/buildbot/filters/viz.content_browsertests.filter

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

commit 4fb927c087d54d5ff8993c2be15e72fe9c05f9b4
Author: danakj <danakj@chromium.org>
Date: Tue Dec 04 23:02:10 2018

Don't send PageMsg_WasShown from RenderFrameHostManager::CommitPending()

This was done to show the Page which was hidden accidentally when
hiding the main frame's RenderWidgetHost when detaching a main
frame. Detaching a main frame doesn't destroy the RenderWidgetHost
because the renderer's RenderWidget's lifetime is tied to
RenderView.

Now the Page is no longer hiding with the main frame RenderWidget
so this code does not need to show the Page when creating a subframe
in a tree that was empty but used to hold a local main frame.

Lots of investigation/details in  crbug.com/908582  from #15 to #19.

R=alexmos@chromium.org, creis@chromium.org

Change-Id: Ie03115c94883c2a19840f7b9e16fc4c94789ede2
Bug: 419087,  908582 ,  638375 
Reviewed-on: https://chromium-review.googlesource.com/c/1357629
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613740}
[modify] https://crrev.com/4fb927c087d54d5ff8993c2be15e72fe9c05f9b4/content/browser/frame_host/render_frame_host_manager.cc