New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 419087
issue 907176



Sign in to add a comment
link

Issue 908582: View/Page and Widget have separate visibility signals but are connected

Reported by danakj@chromium.org, Nov 26 Project Member

Issue description

Frame RenderWidgets should derive visibility from the same signal as the Page.

Non-Frame RenderWidgets do not (they have their own page/world) so they need their own visibility signal. This should be a separate IPC.

This supports the idea of having different "RenderWidgets" for frames vs not, but that's a big idea.

It looks like these 2 separate signals causes flaky DCHECKs where the RenderWidget gets visible first with an IPC, this starts the compositor, its making a frame, but the Page does not get its visibility signal yet.

It seems really wrong to be producing a main frame when the Page is not visible, and hopefully code does not change its output based on it.
 

Comment 1 by danakj@chromium.org, Nov 26

Blocking: 907176

Comment 2 by danakj@chromium.org, Nov 26

Prior to https://chromium-review.googlesource.com/c/chromium/src/+/1320191 I guess the main frame RenderWidget's *compositor* was deriving this from the Page visibility.

RenderView would be notified (at some point after RenderWidget was made visible), which would update the Page, and the compositor.

For sub frames RenderWidget would be notified (requires it to be notified after the RenderView though) and then tell RenderFrames which would update the RenderWidget's compositor.

Comment 3 by danakj@chromium.org, Nov 26

Alternatively, what's the point of RenderViewImpl::OnPageWasShown/Hidden? Why not do that when the main frame's RenderWidget changes.

Comment 4 by danakj@chromium.org, Nov 26

The browser side looks like this:

void WebContentsImpl::WasShown() {
  controller_.SetActive(true);

  if (auto* view = GetRenderWidgetHostView()) {
    view->Show();
#if defined(OS_MACOSX)
    view->SetActive(true);
#endif
  }

  if (!ShowingInterstitialPage())
    SetVisibilityForChildViews(true);

  SendPageMessage(new PageMsg_WasShown(MSG_ROUTING_NONE));
  ...
}



void WebContentsImpl::WasHidden() {
  // If there are entities capturing screenshots or video (e.g. mirroring),
  // or in Picture-in-Picture mode, don't activate the "disable rendering"
  // optimization.
  if (!IsBeingCaptured() && !HasPictureInPictureVideo()) {
    // |GetRenderViewHost()| can be NULL if the user middle clicks a link to
    // open a tab in the background, then closes the tab before selecting it.
    // This is because closing the tab calls WebContentsImpl::Destroy(), which
    // removes the |GetRenderViewHost()|; then when we actually destroy the
    // window, OnWindowPosChanged() notices and calls WasHidden() (which
    // calls us).
    if (auto* view = GetRenderWidgetHostView())
      view->Hide();

    if (!ShowingInterstitialPage())
      SetVisibilityForChildViews(false);

    SendPageMessage(new PageMsg_WasHidden(MSG_ROUTING_NONE));
  }
  ...
}

The SetVisibilityForChildViews() walks the frame tree for each child RenderFrameHostImpl from the root, and does Show()/Hide() on the RenderWidgetHostView.

RenderWidgetHostView's Show/Hide does host()->WasShown() or host()->WasHidden() which is the RenderWidgetHostImpl, which does:

void RenderWidgetHostImpl::WasShown(bool record_presentation_time) {
  ...
  Send(new WidgetMsg_WasShown(
      routing_id_,
      record_presentation_time ? base::TimeTicks::Now() : base::TimeTicks(),
      view_->is_evicted()));
  ...
}

Or

void RenderWidgetHostImpl::WasHidden() {
  ...
  Send(new WidgetMsg_WasHidden(routing_id_));
  ...
}

Comment 5 by danakj@chromium.org, Nov 26

Note that in #2 I claimed subframes have to be made visible after the view/page, but that is not what the browser is doing.

I was confused because here: https://chromium-review.googlesource.com/c/chromium/src/+/1320191/6/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc#b1068

WebFrameWidgetImpl::SetIsAcceleratedCompositingActive(bool active) will when becoming visible (but not when becoming hidden...) show the compositor with the Page's visibility, not with the boolean being passed.

But then I noticed this was done when setting the root layer, not in response to the frame being shown/hidden.

So I think that subframe compositors where just never hidden ever before, that's cool.

Comment 6 by danakj@chromium.org, Nov 26

No, correction. The WebFrameWidgetImpl was setting visible when the root layer was set, but it also did

void WebFrameWidgetImpl::SetVisibilityState(
    mojom::PageVisibilityState visibility_state) {
  if (layer_tree_view_) {
    layer_tree_view_->SetVisible(visibility_state ==
                                 mojom::PageVisibilityState::kVisible);
  }
}

Which set the compositor to visible in response to the RenderWidget.

So this flakiness with the main frame happening while the Page isn't visible yet could have happened for sub frames before, and now it can happen for main frames too.

Comment 7 by danakj@chromium.org, Nov 26

Woa thats ambiguous as heck lmao.

Flakiness with the compositor's BeginMainFrame aka layout/paint happening while the Page isn't visible yet. This is now happening for the main frame's (as in blink::Frame) compositor/widget but could have before for subframes already. We just didn't have a test that did that so we didn't see it as flaky.

Comment 8 by danakj@chromium.org, Nov 26

So then I got to wondering but if this is WebContentsImpl telling the RenderView/Page to be visible/hidden... *which* one is it telling, who tells the proxys? Well it turns out SendPageMessage does that, it goes through RenderFrameHostManager and loops through the proxies sending it, some bonus to a "speculative RenderView" apparently, and on to the non-proxy RenderView.

Comment 9 by danakj@chromium.org, Nov 26

Blocking: 419087

Comment 10 by danakj@chromium.org, Nov 27

I was planning to try drop the page visibility IPC entirely.

But that's the one that goes and notifies the proxy Pages. I don't want to tie this into requiring a frozen main frame widget to stay around obviously..

So my next thought is I may try remove the RenderWidget message (for frame widgets) and DCHECK that, and then have each frame register its RenderWidget on the RenderView so the page notification can tell all the widgets.

Comment 11 by bugdroid1@chromium.org, Nov 29

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

commit a4ba7e504ded9354e2e333c3fd9119e63cbaf55a
Author: danakj <danakj@chromium.org>
Date: Thu Nov 29 23:04:31 2018

Make more explicit the RenderFrame override of visibility to kPrerender

It doesn't override to arbitrary things, so let the RenderViewImpl have
a better idea what is going on by only asking for a bool to use
kPrerender visibility instead of shown/hidden.

This will help us allow the RenderViewImpl to control the visibility
of the RenderWidgets.

R=dcheng@chromium.org

Change-Id: I8b4bc4a8fd969c1c2e6a4c90f1269a5d41b41ba6
Bug:  908582 
Reviewed-on: https://chromium-review.googlesource.com/c/1355773
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612422}
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/chrome/renderer/prerender/prerender_helper.cc
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/content/public/renderer/render_frame.h
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/content/public/renderer/render_view.h
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/content/renderer/render_frame_impl.h
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/content/renderer/render_view_impl.cc
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/content/renderer/render_view_impl.h
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a/third_party/blink/renderer/core/exported/web_view_impl.h

Comment 12 by danakj@chromium.org, Nov 30

Cc: danakj@chromium.org
 Issue 907176  has been merged into this issue.

Comment 13 by bugdroid1@chromium.org, Nov 30

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

commit e548e0768957d6932ff67456b0f90b84aba67fec
Author: Egor Pasko <pasko@chromium.org>
Date: Fri Nov 30 21:28:49 2018

Remove PrerenderBrowserTest.PrerenderVisibility

The test legitimately fails when "prerender" value gets removed from
visibilityState as it is done in http://crrev.com/c/1356120

Unblock this change by removing the test. Traditional prerenders should
no longer happen, while nostate-prefetch should not ever look at
visibility state.

TBR=mattcary@chromium.org

Bug:  908582 , 755921
Change-Id: I44da809f924b2730ac2b00fd85a1d1e484a3aacb
Reviewed-on: https://chromium-review.googlesource.com/c/1356939
Reviewed-by: Egor Pasko <pasko@chromium.org>
Commit-Queue: Egor Pasko <pasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612792}
[modify] https://crrev.com/e548e0768957d6932ff67456b0f90b84aba67fec/chrome/browser/prerender/prerender_browsertest.cc
[delete] https://crrev.com/104459dbd6e3bab6fea41c00beae46f23775f153/chrome/test/data/prerender/prerender_visibility.html
[delete] https://crrev.com/104459dbd6e3bab6fea41c00beae46f23775f153/chrome/test/data/prerender/prerender_visibility_shared.js

Comment 14 by bugdroid1@chromium.org, Nov 30

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7874491254982eea1ea5dfb254e24073412d4f96

commit 7874491254982eea1ea5dfb254e24073412d4f96
Author: danakj <danakj@chromium.org>
Date: Fri Nov 30 23:46:42 2018

Remove kPrerender visibility from the renderer.

Change all PageVisibilityState enums to be just booleans of hidden or
not in the renderer side code. Prerender state is not needed any more
there for correctness. The only places that check explictly for
prerender are to generate strings, or in RenderFrameImpl in order to
pass a bool along. We can remove the former and pass along the bool
more explicitly in the latter case since RenderFrameImpl can query if
prerendering is taking place for the frame.

There are a few places that check for == kVisible or != kVisible. For
the == kVisible, prerendering is only used while the frame is not
visible per the comment being deleted in prerender_helper.h. So this
should not change behaviour as prerender == hidden from this
perspective.

For places that check == kHidden or != kHidden it could be a new
behaviour when it would have been kPrerender before:

Document::DispatchUnloadEvents() would if !kHidden then dispatch
a visibility change event. This would have occured if going to
prerender during unload. Since prerender is for pre-loading this
seems okay to not happen anymore.

LocalFrameView::ShouldSetCursor() would return true during prerender.
This is part of event handling which is after the page is transitioned
out of prerender state, so it should be okay to return false during
prerender now.

This also removes "prerender" from layout tests. Layout tests set a
prerender visibility state, which would just set to hidden internally,
and verify it works like hidden. Since prerender no longer exists in
the renderer, these tests would just be testing hidden twice, and
we can remove the prerender bits.

R=avi@chromium.org, dcheng@chromium.org, pasko@chromium.org
TBR=avi

Change-Id: I914f47f49b41cbf8080ff6271c2896c1a5dbf569
Bug:  908582 
Reviewed-on: https://chromium-review.googlesource.com/c/1356120
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612849}
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/chrome/renderer/prerender/prerender_helper.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/components/plugins/renderer/webview_plugin.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/components/printing/renderer/print_render_frame_helper.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/browser/service_worker/service_worker_client_utils.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/public/renderer/render_frame.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/public/renderer/render_view.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/renderer/gpu/gpu_benchmarking_extension.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/renderer/loader/request_extra_data.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/renderer/loader/request_extra_data.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/renderer/render_frame_impl.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/renderer/render_view_impl.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/renderer/render_view_impl.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/shell/test_runner/test_runner_for_specific_view.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/content/shell/test_runner/web_view_test_proxy.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/extensions/renderer/scoped_web_frame.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/media/blink/webmediaplayer_impl_unittest.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/public/mojom/service_worker/service_worker_client.mojom
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/public/platform/modules/service_worker/web_service_worker_clients_info.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/public/web/web_local_frame_client.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/dom/document.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/exported/web_view_test.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/exported/worker_shadow_page.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/frame/frame_test_helpers.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/html/canvas/canvas_font_cache_test.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/loader/interactive_detector.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/loader/interactive_detector.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/page/page.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/page/page.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/page/page_visibility_state.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/page/page_visibility_state.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/svg/graphics/svg_image_test.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/timing/performance_navigation_timing.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/core/timing/performance_navigation_timing_test.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d_test.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/modules/clipboard/clipboard_promise.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/modules/keyboard/keyboard_layout.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/modules/nfc/nfc.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/modules/presentation/presentation_availability.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/modules/presentation/presentation_availability_test.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/modules/sensor/sensor_proxy.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/modules/service_worker/service_worker_window_client.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/modules/service_worker/service_worker_window_client.h
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/renderer/modules/wake_lock/screen_wake_lock_test.cc
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/web_tests/fast/dom/timer-throttling-hidden-page.html
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/web_tests/fast/events/page-visibility-prefixed-expected.txt
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/web_tests/fast/events/page-visibility-prefixed.html
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/web_tests/fast/events/page-visibility-transition-test-expected.txt
[modify] https://crrev.com/7874491254982eea1ea5dfb254e24073412d4f96/third_party/blink/web_tests/fast/events/page-visibility-transition-test.html

Comment 15 by danakj@chromium.org, Nov 30

So removing the WidgetMsg_WasHidden would be easy enough, and have RenderView notify them all. However WidgetMsg_WasShown has a couple parameters that are per-widget, so moving that to the view would need to pass a set of widget routing ids or something that it wants to record_presentation_time or notify it was evicted.

That's one idea.

Or I guess now I'm leaning to removing the PageMsg_WasShown/Hidden instead, however that is how proxy trees hear about visibility, since only one RenderView has a main frame RenderWidget which could most-obviously control the Page's visibility through the owner_delegate_.

However.. RenderFrameHostManager::CommitPending() also sends a PageMsg_WasShown to a RenderFrameProxyHost (for  https://crbug.com/638375 ).

  // Sets the speculative RenderFrameHost to be the active one. Called when the                                            
  // pending RenderFrameHost commits.                                                                                      
  void CommitPending();

The code:

  // 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) {
    if (!is_main_frame &&
        !render_frame_host_->render_view_host()->is_active()) {
      // Ensure that page visibility in the subframe's process is set to shown.                                            
      // This is important if the subframe is using a RenderView which                                                     
      // started out as active and later became swapped-out, which also updates                                            
      // page visibility to hidden.  Without updating page visibility the                                                  
      // subframe would not be able to generate compositor frames.  See                                                    
      //  https://crbug.com/638375 .                                                                                         
      //                                                                                                                   
      // TODO(alexmos,dcheng,lfg): This workaround should be cleaned up as part                                            
      // of the view/widget split.  We should decouple page visibility from                                                
      // widget visibility.                                                                                                
      RenderFrameProxyHost* proxy =
          frame_tree_node_->frame_tree()
              ->root()
              ->render_manager()
              ->GetRenderFrameProxyHost(render_frame_host_->GetSiteInstance());
      // The proxy should always exist since the RenderViewHost is not active.                                             
      proxy->Send(new PageMsg_WasShown(proxy->GetRoutingID()));
    }

    // In most cases, we need to show the new view.                                                                        
    render_frame_host_->GetView()->Show();
  }

So we send this if the RenderFrameHost has a RenderWidgetView (aka has a RenderWidgetHost aka is a local root aka has a RenderWidget)
And if it's not a main frame, so it's a subframe. If this is a main frame with a widget then the main frame isn't a proxy and the RenderView will presumably hear about it... somewhere else?

It also does this only if the render_frame_host_->render_view_host() is not active. This means the RenderView is swapped out (there is no main frame widget).

So between those two checks, this is a proxy child local root frame, in a tree without a local main frame... I think.

This message gets sent to the *main frame RenderFrameProxy* which forwards it to RenderViewImpl:

  // Forward Page IPCs to the RenderView.                                                                                  
  if ((IPC_MESSAGE_CLASS(msg) == PageMsgStart)) {
    if (render_view())
      return render_view()->OnMessageReceived(msg);
  
    return false;
  }

Reading through https://bugs.chromium.org/p/chromium/issues/detail?id=638375#c28

It refers to WebFrameWidgetImpl::SetVisibilityState() not updating Page visibility, but that code doesn't exist anymore.

What it claims happens is one call to RenderFrameHostManager::CommitPending() is hiding a main frame, so we hide the RenderWidgetView -> RenderWidget, which hides the Page...? I don't see that, it would just end up in RenderWidgetHostImpl::WasHidden() and Send a WidgetMsg_WasHidden, not a PageMsg_WasHidden..

It claims that hiding the render widget hides the WebViewFrameWidget which hides the WebViewImpl (without going through the RenderViewImpl at all, which would want to do other things!!!)

It's possible I have broken this in https://chromium-review.googlesource.com/c/1320191, and we're leaving the Page visible in this case now. Removing the Send() to the RenderFrameProxy doesn't break the test case in https://bugs.chromium.org/p/chromium/issues/detail?id=638375#c0 now.

So.. Then a future call to RenderFrameHostManager::CommitPending() for a subframe, while the RenderView is still around (because we don't drop it until the navigation completes fully and the new mainframe in some way eventually refers to a subframe back in this RenderView's tree??).

So then we go and Show() this child local root RenderWidget.

This explains the "this is a proxy child local root frame, in a tree without a local main frame" check, as it's trying to narrow down to this exact scenario. It's explainable from a navigation use case, more than it is from reading the code directly.

But when this Show() to the child local root happens, it doesn't show the RenderView (tho neither does to a main frame RenderWidget, but I guess it might have used to...).

So it seems to me like the Hide should be going to the (main frame) RenderWidget (optionally really, it will get frozen) but also more importantly to the RenderView !

And then that the Show should instead of going to this proxy main frame, which redirects to the RenderView, it could also go directly to the RenderView ??

Comment 16 by danakj@chromium.org, Dec 1

Cc: creis@chromium.org alex...@chromium.org
+folks from 638375 which I think I uh.. broke.. in a weird works but is always visible now way.

Up for question is why we mark Page as not visible when navigating away from it on the main frame, but I'll just accept we rly want to do that.

I confirmed with prints that when I follow that test case:

1. When I "open simple window" the main frame RenderWidget 0x107fe8c3fc00 from the original tab is hidden.
2. Also the RenderView 0x107fe8c3fc00 from the original tab is hidden.
-- this was the WebContentsImpl hidden path from hiding the tab, and is WAI --
3. When I navigate the new window back to http://csreis.github.io/tests/cross-site-iframes.html the RenderFrameHostManager hits the local main frame path and hides the main frame RenderWidget 0x107fe8f5e000 for the "simple window"
4. The RenderView and Page for the "simple window" *is not hidden*
5. Then when "go cross site" button is hit, the subframe RenderWidget 0x107fe8fbbc00 is hidden.

I commented out the `proxy->Send(new PageMsg_WasShown(proxy->GetRoutingID()))` call and it works fine, since at step 4 the RenderView/Page for the "simple window" was not hidden.

Comment 17 by danakj@chromium.org, Dec 1

Oh so it seems like all RenderView messages of PageMsg variety may be going through the main RenderFrameImpl/RenderFrameProxy, via RenderFrameHostManager, that's interesting.

Comment 18 by danakj@chromium.org, Dec 1

> Up for question is why we mark Page as not visible when navigating away from it on the main frame, but I'll just accept we rly want to do that.

Actually from reading the code this is not clear that it's really intended. The code says 

  // 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 comment says its hiding the RenderViewHost, but it's really calling hide on the RenderWidgetHost(View) aka hiding the RenderWidget. Why does it do this? I have a theory.

Because this old_render_frame_host is going away, this would detach the local Frame in the renderer. For a non-main frame this will drop the RenderWidget for it as well, if it was a local root.

But if it's a main frame, that RenderWidget can't go away (crbug.com/419087), so we will freeze it when the frame is detached.

But this code goes ahead and also marks it as not visible here first. Now was that done before freezing existed? It does seem appropriate to hide it when its going to be frozen (though weird that these are not just connected as one action - just have the freeze hide as well..)

But there is a bunch of browser-side stuff too in RenderWidgetHostView/RenderWidgetHostImpl for hiding/showing. So we probably really want *that* to run here, and the IPC to hide is a bit of a side effect.. is my theory.

So anyway our goal was to hide the main RenderWidget stuff here. But because that used to then hide the WebView/Page, when we reused the Page later, it had this stale hidden state, so we added the Send there to show it again.

Long story short it doesn't *seem* from reading this code that it is claiming the Page is supposed to be hidden, it was just working around that.

So now it won't be hidden, unless the tab was in the background when the action happened. If that was the case, then we'd actually want it to be not visible, and the Send(PageMsg_WasShown) here would be incorrect and have the subframe in the background tab sending frames.. oops.

So summary I think I just remove this Send(PageMsg_WasShown) now.

Comment 19 by danakj@chromium.org, Dec 1

Oh noting:

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

So this isn't a problem anymore already thanks to https://chromium-review.googlesource.com/c/1320191. And confirms we don't want the Page to be hidden as side effect of the main frame RenderWidget being hidden (when it's going to be swapped out/frozen....)

Comment 20 by danakj@chromium.org, Dec 1

Notably all these older code/comments say "we wish visibility signals for widget and page were disconnected", whereas this bug is "i wish they were connected".

If we're going to connect them, we probably want a RenderWidget changing its vis to change its RenderView (whether its a main frame widget or not), and *also when a RenderWidget is created* because the RenderView could be sitting there with no frames/widgets in it apparently!

Comment 21 by bugdroid1@chromium.org, Dec 1

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0018a29aefbefa1aee77a026a1a09ca90664212e

commit 0018a29aefbefa1aee77a026a1a09ca90664212e
Author: danakj <danakj@chromium.org>
Date: Sat Dec 01 01:03:43 2018

Move blink::mojom::PageVisibilityState to content::PageVisibilityState.

This moves the mojom enum to be a standard c++ enum class in
content/public/browser/. It's used by chrome/browser and
content/browser, as well as content/shell/test_runner, though the
latter one will be removed.

This enum is no longer used by the renderer code nor sent over IPC so
not need to be in blink or as a mojo enum.

R=avi@chromium.org, dcheng@chromium.org
TBR=dcheng

Change-Id: I18a81513332338ae36fe789ab4d289061335c4c3
Bug:  908582 
Reviewed-on: https://chromium-review.googlesource.com/c/1357507
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612883}
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/chrome/browser/after_startup_task_utils.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/chrome/browser/notifications/notification_permission_context.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/chrome/browser/push_messaging/push_messaging_notification_manager.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/components/plugins/renderer/DEPS
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/components/plugins/renderer/webview_plugin.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/components/printing/renderer/print_render_frame_helper.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/browser/frame_host/render_frame_host_impl_browsertest.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/browser/loader/navigation_url_loader_impl_unittest.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/browser/loader/navigation_url_loader_unittest.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/browser/loader/resource_dispatcher_host_unittest.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/browser/service_worker/service_worker_client_utils.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/common/content_param_traits_macros.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/common/service_worker/service_worker_types.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/public/browser/BUILD.gn
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/public/browser/content_browser_client.h
[add] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/public/browser/page_visibility_state.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/public/browser/render_frame_host.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/public/renderer/render_frame.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/renderer/loader/request_extra_data.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/renderer/render_frame_impl.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/content/renderer/render_view_impl.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/extensions/renderer/DEPS
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/extensions/renderer/scoped_web_frame.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/public/mojom/BUILD.gn
[delete] https://crrev.com/0e998e395bc349b41af06c4bcb8ec11b39f1fefc/third_party/blink/public/mojom/page/page_visibility_state.mojom
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/public/web/web_frame_widget.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/public/web/web_local_frame_client.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/public/web/web_view_client.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/exported/web_view_test.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/exported/worker_shadow_page.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/frame/frame_test_helpers.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/frame/web_view_frame_widget.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/html/canvas/canvas_font_cache_test.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/loader/interactive_detector.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/page/BUILD.gn
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/page/page.cc
[rename] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/page/page_hidden_state.cc
[rename] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/page/page_hidden_state.h
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/timing/performance_navigation_timing.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/core/timing/performance_navigation_timing_test.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d_test.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/canvas/offscreencanvas/offscreen_canvas_test.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/clipboard/clipboard_promise.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/keyboard/keyboard_layout.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/nfc/nfc.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/presentation/presentation_availability.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/presentation/presentation_availability_test.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/sensor/sensor_proxy.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/service_worker/service_worker_window_client.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/wake_lock/screen_wake_lock.cc
[modify] https://crrev.com/0018a29aefbefa1aee77a026a1a09ca90664212e/third_party/blink/renderer/modules/wake_lock/screen_wake_lock_test.cc

Comment 22 by danakj@chromium.org, Dec 1

And if we're not going to connect them, then we don't want to show the compositor until the page is shown, or we want to show the page before we show the widgets. The latter seems... pretty reasonable?

Comment 23 by bugdroid1@chromium.org, Dec 4

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

commit e80fd037f976848f06ecaab1e9460d28c8ee7302
Author: danakj <danakj@chromium.org>
Date: Tue Dec 04 17:39:26 2018

Re-enable SubframeVisibleAfterRenderViewBecomesSwappedOut on android.

Visibility has been moved around a bunch, and the Page is no longer
made non-visible when the main frame detaches from the tree and we
hide/freeze the RenderWidget.

Then if a subframe is added to the tree, its RenderWidget is shown
directly and when the root GraphicsLayer is attached, visibility of
the RenderWidget is not modified. Plus the Page's visibility is left as
correct anyhow.

R=alexmos@chromium.org

Change-Id: Iac795123a7c3ef80e4cf635b7d77cc66474b6597
Bug: 897709,  908582 
Reviewed-on: https://chromium-review.googlesource.com/c/1357635
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613585}
[modify] https://crrev.com/e80fd037f976848f06ecaab1e9460d28c8ee7302/content/browser/site_per_process_browsertest.cc

Comment 24 by danakj@chromium.org, Dec 4

Ok let's tl;dr this monster.

Previously RenderViewImpl inherited visibility from RenderWidget and this was a mess. Since we dropped all the prerender stuff in https://crrev.com/a4ba7e504ded9354e2e333c3fd9119e63cbaf55a that is no longer the case. And we removed compositor visiblity setting from WebViewImpl and do it from RenderWidget directly, so the RenderViewImpl message handler doesn't read or write to bits regarding widget visibility now.

As such we have 2 fully independent IPCs, but they create races since we show the widget then the Page, and its surprising to the renderer code to start making a compositor frame/painting when the Page isn't shown yet.

So I think the only thing left to do here is to show the Page before the widgets (we already hide it after so that side is good).

Comment 25 by bugdroid1@chromium.org, Dec 4

Project Member
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

Comment 26 by bugdroid1@chromium.org, Dec 5

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

commit 4aa277a93fcb486b8ce2d763d7d227a9bf8e2e57
Author: danakj <danakj@chromium.org>
Date: Wed Dec 05 16:21:24 2018

Shortcut through RenderFrameHost::GetView() instead of RenderViewHost.

This code goes through RenderViewHost to get the widget's view, which
is valid since it is a main frame, but we can more simply grab the
widget's view from the RenderFrameHost directly.

R=creis@chromium.org

Change-Id: I08d0496c19a5dfd161fe2842e9e3449ef88d3a91
Bug:  908582 
Reviewed-on: https://chromium-review.googlesource.com/c/1361783
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613988}
[modify] https://crrev.com/4aa277a93fcb486b8ce2d763d7d227a9bf8e2e57/content/browser/frame_host/render_frame_host_manager.cc

Comment 27 by bugdroid1@chromium.org, Dec 5

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

commit c1e71e4fbdaedd9593cec822fe070f561c27e58f
Author: danakj <danakj@chromium.org>
Date: Wed Dec 05 19:34:35 2018

Show the RenderView/Page before showing RenderWidgets.

RenderWidgets start making compositor frames when shown. If the Page
IPC is delayed then they may start trying to produce BeginMainFrames
on a non-visible Page which is confusing to blink and incorrect.

R=avi@chromium.org

Change-Id: If77b892d7d5a9d7606dff529519e7febf1e4440a
Bug:  908582 
Reviewed-on: https://chromium-review.googlesource.com/c/1361937
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614060}
[modify] https://crrev.com/c1e71e4fbdaedd9593cec822fe070f561c27e58f/content/browser/web_contents/web_contents_impl.cc

Comment 28 by danakj@chromium.org, Dec 5

Status: Fixed (was: Assigned)

Comment 29 by bugdroid1@chromium.org, Dec 7

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

commit cc8b1e563f106abfcd3e153bcaf03f3c6a272d81
Author: danakj <danakj@chromium.org>
Date: Fri Dec 07 01:07:02 2018

Update TODO about hiding the main frame RenderWidget when swapping out

Combine 2 TODOs into one explanation about why this is done for main
frames only (subframes don't need it since we drop the RenderWidget
when the frame is detached during SwapOut).

R=creis@chromium.org

Change-Id: Ib10c51401994f9a3a6d8270b45eb1006297ecb50
Bug:  908582 
Reviewed-on: https://chromium-review.googlesource.com/c/1362104
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614523}
[modify] https://crrev.com/cc8b1e563f106abfcd3e153bcaf03f3c6a272d81/content/browser/frame_host/render_frame_host_manager.cc

Sign in to add a comment