Ensure the renderer can reply to ClosePage with an ACK while closing.

When a renderer has an unload handler (which can be caused by opening
devtools apparently), the browser sends a ClosePage IPC to the renderer
when it requests to be closed, and waits for ClosePage_ACK before
sending the WidgetMsg_Close IPC and closing the tab.

When sending the ClosePage_ACK was moved to RenderViewImpl, the Send()
method was changed when to stop sending IPCs. In RenderWidget it
checked RenderWidget::closing_. But in RenderViewImpl it checks
RenderWidget::is_closing(). These look the same but they were
different, causing the ClosePage_ACK to never be sent. This CL
rectifies the situation by making is_closing() simply return closing_,
which restores the previous behaviour and allows browser-renderer
interactions between the close request and the actual close action.

The is_closing() included the host_will_close_this_ variable, which
exists for a rather convoluted reason. When WebPagePopupImpl requests
to be closed through the RenderWidget, it immediately destroys its Page
object, and its main frame. That means the RenderWidget's WebWidget
(which is the WebPagePopupImpl) becomes crashy and invalid after
requesting to close. All the methods in the WebWidget API early out
when page_ is null to handle this scenario. Except one, the
GetURLForDebugTrace() method, which is called when making a
LayerTreeFrameSink.

To solve this problem in the past, rather than making this method early
out (or whatever other method it was at that time), we made
RenderWidget avoid making a LayerTreeFrameSink once a close has been
requested, to indirectly avoid using the WebWidget.

Now that we understand this area of code more fully, we can solve this
at the site of the problem in WebPagePopupImpl, and early out in
GetURLForDebugTrace() like we do in other WebWidget methods. That means
we don't need host_will_close_this_.

The mistake here was leaking the host_will_close_this_ state out of
RenderWidget and using it for more than it was intended. But since we
don't need the variable at all in its more narrow scope we remove it.

R=avi@chromium.org

Bug: 995123
Change-Id: Iaad83c7e3892908150672d9860a6db4307ffe432
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1814689Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#698531}

content/renderer/render_widget.cc

@@ -1273,11 +1273,10 @@ void RenderWidget::RequestNewLayerTreeFrameSink(
   // widgets for provisional frames do start their compositor.
   DCHECK(!is_undead_);
 
-  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.
+  // If we early out, we drop the request which means the compositor waits
+  // forever, which is fine since we're going to destroy it soon.
+  if (closing_)
     return;
-  }
 
   // TODO:(https://crbug.com/995981): If there is no WebWidget, then the
   // RenderWidget should also be destroyed, and this DCHECK should not be
@@ -1994,13 +1993,6 @@ void RenderWidget::ClosePopupWidgetSoon() {
 
 void RenderWidget::CloseWidgetSoon() {
   DCHECK(RenderThread::IsMainThread());
-  // Prevent compositor from setting up new IPC channels, since we know a
-  // WidgetMsg_Close is coming. We do this immediately, not in DoDeferredClose,
-  // as the caller (eg WebPagePopupImpl) may start tearing down things after
-  // calling this method, including detaching the frame from this RenderWidget.
-  // Then trying to make a LayerTreeFrameSink would crash.
-  // https://crbug.com/906340
-  host_will_close_this_ = true;
 
   // If a page calls window.close() twice, we'll end up here twice, but that's
   // OK.  It is safe to send multiple Close messages.

content/renderer/render_widget.h

@@ -318,12 +318,7 @@ class CONTENT_EXPORT RenderWidget
   // 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_; }
+  bool is_closing() const { return closing_; }
 
   // Manage edit commands to be used for the next keyboard event.
   const EditCommands& edit_commands() const { return edit_commands_; }
@@ -1013,9 +1008,6 @@ class CONTENT_EXPORT RenderWidget
   // be sent, except for a Close.
   bool closing_ = false;
 
-  // True if it is known that the host is in the process of being shut down.
-  bool host_will_close_this_ = false;
-
   // A RenderWidget is undead if it is the RenderWidget attached to the
   // RenderViewImpl for its main frame, but there is a proxy main frame in
   // RenderViewImpl's frame tree. Since proxy frames do not have content they

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

@@ -485,6 +485,8 @@ void WebPagePopupImpl::SetFocus(bool enable) {
 }
 
 WebURL WebPagePopupImpl::GetURLForDebugTrace() {
+  if (!page_)
+    return {};
   WebFrame* main_frame = web_view_->MainFrame();
   if (main_frame->IsWebLocalFrame())
     return main_frame->ToWebLocalFrame()->GetDocument().Url();

third_party/blink/renderer/core/exported/web_page_popup_impl.h

@@ -107,6 +107,11 @@ class CORE_EXPORT WebPagePopupImpl final : public WebPagePopup,
 
  private:
   // WebWidget implementation.
+  // NOTE: The WebWidget may still be used after requesting the popup to be
+  // closed and destroyed. But the Page and the MainFrame are destroyed
+  // immediately. So all methods (outside of initialization) that are part
+  // of the WebWidget need to check if close has already been initiated (they
+  // can do so by checking |page_|) and not crash! https://crbug.com/906340
   void SetAnimationHost(cc::AnimationHost*) override;
   void SetSuppressFrameRequestsWorkaroundFor704763Only(bool) final;
   void BeginFrame(base::TimeTicks last_frame_time,