Revert "Don't make a LayerTreeFrameSink for a non-visible RenderWidget."

This reverts commit 68d8dbc0.

Reason for revert: Looks like it makes PrerenderBrowserTest.PrerenderInfiniteLoop according to FindIt. 

../../chrome/browser/prerender/prerender_browsertest.cc:1721: Failure
Expected equality of these values:
  1U
    Which is: 1
  GetLinkPrerenderCount()
    Which is: 2

Original change's description:
> Don't make a LayerTreeFrameSink for a non-visible RenderWidget.
> 
> Importantly, non-visible RenderWidgets include swapped out RenderWidgets
> which are zombies, without a frame, and which should not be used.
> 
> This can happen if the RenderWidget swap out races with the posted task
> from the compositor to make a LayerTreeFrameSink. When swapping out, the
> compositor would be marked as not-visible, which would stop it from
> making such requests, but one could already be in flight.
> 
> In the other case, honoring the request for a non-visible compositor is
> more benign, but we can delay it until the compositor is actually
> visible again. This means if a context is lost, backgrounded tabs would
> not all attempt to reconnect to the Gpu process at once.
> 
> This is a better followup for the hacky
> https://chromium-review.googlesource.com/c/chromium/src/+/1292711 which
> was meant only for merge to a release branch. It reverts that change as
> part of this one.
> 
> R=​piman@chromium.org
> 
> Change-Id: I058bdc37482d04bb86d65a7377b7520dad8573e7
> Bug: 896836, 419087
> Reviewed-on: https://chromium-review.googlesource.com/c/1292714
> Commit-Queue: danakj <danakj@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Antoine Labour <piman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#606655}

TBR=danakj@chromium.org,dcheng@chromium.org,piman@chromium.org

Change-Id: I65238aba0abdb6c9192e9f3cb3691f8dd89b3ea5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 896836, 419087, 903696
Reviewed-on: https://chromium-review.googlesource.com/c/1329003Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606807}

content/renderer/gpu/layer_tree_view.cc

@@ -637,31 +637,11 @@ void LayerTreeView::RecordWheelAndTouchScrollingCount(
 }
 
 void LayerTreeView::RequestNewLayerTreeFrameSink() {
-  // When the compositor is not visible it would not request a
-  // LayerTreeFrameSink so this is a race where it requested one then became
-  // not-visible. In that case, we can wait for it to become visible again
-  // before replying.
-  //
-  // This deals with an insidious race when the RenderWidget is swapped-out
-  // after this task was posted from the compositor. When swapped-out the
-  // compositor is stopped by making it not visible, and the RenderWidget (our
-  // delegate) is a zombie which can not be used (https://crbug.com/894899).
-  // Eventually the RenderWidget/LayerTreeView will not exist at all in this
-  // case (https://crbug.com/419087). So this handles the case for now since the
-  // compositor is not visible, and we can avoid using the RenderWidget until
-  // it marks the compositor visible again, indicating that it is valid to use
-  // the RenderWidget again.
-  //
-  // If there is no compositor thread, then this is a test-only path where
-  // composite is controlled directly by blink, and visibility is not
-  // considered. We don't expect blink to ever try composite on a swapped-out
-  // RenderWidget, which would be a bug, but the race condition can't happen
-  // in the single-thread case since this isn't a posted task.
-  if (!layer_tree_host_->IsVisible() && !!compositor_thread_) {
-    layer_tree_frame_sink_request_failed_while_invisible_ = true;
+  // If the host is closing, then no more compositing is possible.  This
+  // prevents shutdown races between handling the close message and
+  // the CreateLayerTreeFrameSink task.
+  if (delegate_->IsClosing())
     return;
-  }
-
   delegate_->RequestNewLayerTreeFrameSink(base::BindOnce(
       &LayerTreeView::SetLayerTreeFrameSink, weak_factory_.GetWeakPtr()));
 }

content/renderer/gpu/layer_tree_view_delegate.h

@@ -63,6 +63,9 @@ class LayerTreeViewDelegate {
   // or committing a frame (at the same time Tracing measurements are taken).
   virtual void RecordEndOfFrameMetrics(base::TimeTicks frame_begin_time) = 0;
 
+  // Indicates whether the LayerTreeView is about to close.
+  virtual bool IsClosing() const = 0;
+
   // Requests that the client schedule a composite now, and calculate
   // appropriate delay for potential future frame.
   virtual void RequestScheduleAnimation() = 0;

content/renderer/input/widget_input_handler_impl.cc

@@ -31,7 +31,7 @@ void RunClosureIfNotSwappedOut(base::WeakPtr<RenderWidget> render_widget,
   // Input messages must not be processed if the RenderWidget is in swapped out
   // or closing state.
   if (!render_widget || render_widget->is_swapped_out() ||
-      render_widget->is_closing()) {
+      render_widget->IsClosing()) {
     return;
   }
   std::move(closure).Run();

content/renderer/input/widget_input_handler_manager.cc

@@ -410,7 +410,7 @@ void WidgetInputHandlerManager::HandleInputEvent(
     const ui::LatencyInfo& latency,
     mojom::WidgetInputHandler::DispatchEventCallback callback) {
   if (!render_widget_ || render_widget_->is_swapped_out() ||
-      render_widget_->is_closing()) {
+      render_widget_->IsClosing()) {
     if (callback) {
       std::move(callback).Run(InputEventAckSource::MAIN_THREAD, latency,
                               INPUT_EVENT_ACK_STATE_NOT_CONSUMED, base::nullopt,

content/renderer/render_frame_impl.cc

@@ -2216,16 +2216,8 @@ void RenderFrameImpl::OnSwapOut(
       this, proxy_routing_id, replicated_frame_state.scope);
 
   // Swap out and stop sending any IPC messages that are not ACKs.
-  if (is_main_frame_) {
-    // The RenderWidget isn't actually closed here because we might need to use
-    // it again. It can't be destroyed and recreated later as it is part of
-    // the |render_view_|, which must be kept alive. So instead just stop the
-    // compositor.
-    // TODO(crbug.com/419087): The RenderWidget should be destroyed as a result
-    // of this (main) frame going away, then we won't have to do this.
-    render_view_->GetWidget()->StopCompositor();
-    render_view_->GetWidget()->SetSwappedOut(true);
-  }
+  if (is_main_frame_)
+    render_view_->SetSwappedOut(true);
 
   RenderViewImpl* render_view = render_view_;
   bool is_main_frame = is_main_frame_;
@@ -5896,15 +5888,8 @@ bool RenderFrameImpl::SwapIn() {
   if (is_main_frame_) {
     CHECK(!render_view_->main_render_frame_);
     render_view_->main_render_frame_ = this;
-    if (render_view_->GetWidget()->is_swapped_out()) {
-      render_view_->GetWidget()->SetSwappedOut(false);
-      // In OnSwapOut() the RenderWidget's compositor was stopped instead of
-      // deleting the RenderWidget. So here we can start it again. If the
-      // |render_view_|'s RenderWidget started swapped in, it may already be
-      // swapped in here, so this does nothing.
-      // TODO(crbug.com/419087): The RenderWidget should be newly created here,
-      // then we won't have to do this.
-      render_view_->GetWidget()->StartCompositor();
+    if (render_view_->is_swapped_out()) {
+      render_view_->SetSwappedOut(false);
     }
     render_view_->UpdateWebViewWithDeviceScaleFactor();
   }

content/renderer/render_view_impl.cc

@@ -1518,10 +1518,22 @@ void RenderViewImpl::AttachWebFrameWidget(blink::WebFrameWidget* frame_widget) {
   // The previous WebFrameWidget must already be detached by CloseForFrame().
   DCHECK(!frame_widget_);
   frame_widget_ = frame_widget;
+  // In CloseForFrame() the RenderWidget's compositor was stopped instead of
+  // deleting the RenderWidget. So here we can start it again. For the first
+  // main frame, it would already be started by default so this does nothing.
+  // TODO(crbug.com/419087): The RenderWidget should be newly created here along
+  // with the WebFrameWidget, then we won't have to do this.
+  GetWidget()->StartCompositor();
 }
 
 void RenderViewImpl::DetachWebFrameWidget() {
   DCHECK(frame_widget_);
+  // The RenderWidget isn't actually closed here because we might need to use it
+  // again since it can't be recreated as it is part of |this|. So instead just
+  // stop the compositor.
+  // TODO(crbug.com/419087): The RenderWidget should be destroyed here along
+  // with the WebFrameWidget, then we won't have to do this.
+  GetWidget()->StopCompositor();
   frame_widget_->Close();
   frame_widget_ = nullptr;
 }

content/renderer/render_widget.cc

@@ -488,6 +488,12 @@ void RenderWidget::CloseForFrame() {
   OnClose();
 }
 
+void RenderWidget::SetSwappedOut(bool is_swapped_out) {
+  // We should only toggle between states.
+  DCHECK(is_swapped_out_ != is_swapped_out);
+  is_swapped_out_ = is_swapped_out;
+}
+
 void RenderWidget::Init(ShowCallback show_callback, WebWidget* web_widget) {
   DCHECK(!webwidget_internal_);
   DCHECK_NE(routing_id_, MSG_ROUTING_NONE);
@@ -668,15 +674,9 @@ void RenderWidget::OnClose() {
   if (routing_id_ != MSG_ROUTING_NONE) {
     RenderThread::Get()->RemoveRoute(routing_id_);
     g_routing_id_widget_map.Get().erase(routing_id_);
-    if (RenderThreadImpl::current()) {
-      // RenderWidgets may be hidden when they are closed. If we were previously
-      // hidden, we are being counted as such in RenderThreadImpl. Thus we
-      // remove that count here by calling WidgetRestored() even though we're
-      // clearly not becoming visible here.
-      if (is_hidden_)
-        RenderThreadImpl::current()->WidgetRestored();
+    SetHidden(false);
+    if (RenderThreadImpl::current())
       RenderThreadImpl::current()->WidgetDestroyed();
-    }
   }
 
   if (for_oopif_) {
@@ -805,8 +805,6 @@ void RenderWidget::OnWasHidden() {
 void RenderWidget::OnWasShown(base::TimeTicks show_request_timestamp,
                               bool was_evicted) {
   TRACE_EVENT0("renderer", "RenderWidget::OnWasShown");
-  DCHECK(!is_swapped_out_);
-
   // During shutdown we can just ignore this message.
   if (!GetWebWidget())
     return;
@@ -968,17 +966,10 @@ void RenderWidget::BeginMainFrame(base::TimeTicks frame_time) {
 
 void RenderWidget::RequestNewLayerTreeFrameSink(
     LayerTreeFrameSinkCallback callback) {
+  DCHECK(GetWebWidget());
   // For widgets that are never visible, we don't start the compositor, so we
   // never get a request for a cc::LayerTreeFrameSink.
   DCHECK(!compositor_never_visible_);
-  // Swapped out RenderWidgets should not be doing any compositing.
-  DCHECK(!is_swapped_out_);
-
-  if (is_closing()) {
-    // In this case, we drop the request which means the compositor waits
-    // forever, which is fine since we're going to destroy it.
-    return;
-  }
 
   // TODO(jonross): have this generated by the LayerTreeFrameSink itself, which
   // would then handle binding.
@@ -1027,6 +1018,19 @@ void RenderWidget::DidCompletePageScaleAnimation() {
     owner_delegate_->DidCompletePageScaleAnimationForWidget();
 }
 
+bool RenderWidget::IsClosing() const {
+  // The |host_will_close_this_| is an optimization when we know that a
+  // WidgetMsg_Close IPC will be coming, though it has not arrived yet.
+  // The |closing_| state means we have received this IPC and are in the
+  // process of closing/destroying the RenderWidget.
+
+  // TODO(ajwong): Once RenderViewImpl and RenderWidget are split, attempt to
+  // combine two states so the shutdown logic is cleaner.
+  //
+  // http://crbug.com/545684
+  return host_will_close_this_ || closing_;
+}
+
 void RenderWidget::RequestScheduleAnimation() {
   if (owner_delegate_) {
     owner_delegate_->RequestScheduleAnimationForWidget();
@@ -1519,6 +1523,7 @@ void RenderWidget::Show(WebNavigationPolicy policy) {
 
 LayerTreeView* RenderWidget::InitializeLayerTreeView() {
   TRACE_EVENT0("blink", "RenderWidget::InitializeLayerTreeView");
+  DCHECK(!IsClosing());
 
   layer_tree_view_ = std::make_unique<LayerTreeView>(
       this, compositor_deps_->GetCompositorMainThreadTaskRunner(),

content/renderer/render_widget.h

@@ -225,23 +225,10 @@ class CONTENT_EXPORT RenderWidget
   // Sets whether this RenderWidget has been swapped out to be displayed by
   // a RenderWidget in a different process.  If so, no new IPC messages will be
   // sent (only ACKs) and the process is free to exit when there are no other
-  // active RenderWidgets. The RenderWidget is not used for compositing as there
-  // is no WebWidget that should display content when swapped out.
-  void SetSwappedOut(bool is_swapped_out) { is_swapped_out_ = is_swapped_out; }
+  // active RenderWidgets.
+  void SetSwappedOut(bool is_swapped_out);
   bool is_swapped_out() const { return is_swapped_out_; }
 
-  // This is true once a Close IPC has been received. The actual action of
-  // closing must be done on another stack frame, in case the IPC receipt
-  // is in a nested message loop and will unwind back up to javascript (from
-  // plugins). So this will be true between those two things, to avoid work
-  // when the RenderWidget will be closed.
-  // Additionally, as an optimization, this is true after we know the renderer
-  // has asked the browser to close this RenderWidget.
-  //
-  // TODO(crbug.com/545684): Once RenderViewImpl and RenderWidget are split,
-  // attempt to combine two states so the shutdown logic is cleaner.
-  bool is_closing() const { return host_will_close_this_ || closing_; }
-
   // Manage edit commands to be used for the next keyboard event.
   const EditCommands& edit_commands() const { return edit_commands_; }
   void SetEditCommandForNextKeyEvent(const std::string& name,
@@ -279,6 +266,7 @@ class CONTENT_EXPORT RenderWidget
   void DidCommitCompositorFrame() override;
   void DidCompletePageScaleAnimation() override;
   void RecordEndOfFrameMetrics(base::TimeTicks frame_begin_time) override;
+  bool IsClosing() const override;
   void RequestScheduleAnimation() override;
   void UpdateVisualState() override;
   void WillBeginCompositorFrame() override;

content/test/stub_layer_tree_view_delegate.cc

@@ -17,6 +17,10 @@ void StubLayerTreeViewDelegate::RequestNewLayerTreeFrameSink(
   std::move(callback).Run(nullptr);
 }
 
+bool StubLayerTreeViewDelegate::IsClosing() const {
+  return false;
+}
+
 std::unique_ptr<cc::SwapPromise>
 StubLayerTreeViewDelegate::RequestCopyOfOutputForLayoutTest(
     std::unique_ptr<viz::CopyOutputRequest> request) {

content/test/stub_layer_tree_view_delegate.h

@@ -26,6 +26,7 @@ class StubLayerTreeViewDelegate : public LayerTreeViewDelegate {
   void DidCommitAndDrawCompositorFrame() override {}
   void DidCommitCompositorFrame() override {}
   void DidCompletePageScaleAnimation() override {}
+  bool IsClosing() const override;
   void RequestScheduleAnimation() override {}
   void UpdateVisualState() override {}
   void WillBeginCompositorFrame() override {}

third_party/blink/renderer/core/exported/web_view_impl.cc

@@ -2817,6 +2817,10 @@ void WebViewImpl::ShowContextMenu(WebMenuSourceType source_type) {
 
 WebURL WebViewImpl::GetURLForDebugTrace() {
   WebFrame* main_frame = MainFrame();
+  // TODO(crbug.com/896836): Avoid a crash in minimal way for merge. But we'll
+  // avoid it properly in a followup.
+  if (!main_frame)
+    return {};
   if (main_frame->IsWebLocalFrame())
     return main_frame->ToWebLocalFrame()->GetDocument().Url();
   return {};