Narrow the scope of page pause when printing. (try 2)

In the current implementation, sometimes the scope of a page's "pause"
is too wide compared to the definition in the spec [1], and sometimes it
does not pause at all. So narrow the scope to conform to the spec, and
apply it consistently to all possible ways users can print.

This is based on an earlier CL [2] which only did this for printing with
the system dialog, but not for Print Preview.

The first attempt at landing this CL [3] got reverted [4] because it
caused bf_cache_browser_tests failures. These tests did not run by
default as part of the CQ, which is how [3] passed the CQ. To fix this
issue, CL [5] landed to make the test less flaky.

Another bug [6] uncovered by the original CL [3] is a bit harder to
solve. The issue here is the case where PrintRenderFrameHelper tries to
unpause during blink::WebLocalFrameImpl teardown, and it is happening
too late. To address this, introduce blink::WebPrintClient, so the
WebLocalFrameImpl can tell the WebPrintClient (PrintRenderFrameHelper,
really) that it will be destroyed. Then PrintRenderFrameHelper can
unpause at the correct time.

[1] https://html.spec.whatwg.org/C/#printing-steps
[2] https://crrev.com/c/1669250
[3] https://crrev.com/797120
[4] https://crrev.com/797234
[5] https://crrev.com/799501
[6] https://crbug.com/1115475

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android-bfcache-rel
CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux-bfcache-rel

Bug: 956832
Change-Id: Ie328d90094f1b282126d51425200f8810819c3c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2353886Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799772}

components/paint_preview/renderer/paint_preview_recorder_impl.cc

@@ -159,7 +159,7 @@ void PaintPreviewRecorderImpl::CapturePaintPreviewInternal(
   }
 
   // Warm up paint for an out-of-lifecycle paint phase.
-  frame->DispatchBeforePrintEvent();
+  frame->DispatchBeforePrintEvent(/*print_client=*/nullptr);
 
   DCHECK_EQ(is_main_frame_, params->is_main_frame);
   // Default to using the clip rect.

components/printing/renderer/print_render_frame_helper.h

@@ -27,6 +27,7 @@
 #include "printing/common/metafile_utils.h"
 #include "printing/mojom/print.mojom-forward.h"
 #include "third_party/blink/public/web/web_node.h"
+#include "third_party/blink/public/web/web_print_client.h"
 #include "third_party/blink/public/web/web_print_params.h"
 #include "ui/gfx/geometry/size.h"
 
@@ -50,8 +51,9 @@ class DictionaryValue;
 
 namespace blink {
 class WebLocalFrame;
+class WebScopedPagePauser;
 class WebView;
-}
+}  // namespace blink
 
 namespace content {
 class AXTreeSnapshotter;
@@ -90,7 +92,8 @@ class FrameReference {
 // RenderView. We plan on making print asynchronous and that will require
 // copying the DOM of the document and creating a new WebView with the contents.
 class PrintRenderFrameHelper
-    : public content::RenderFrameObserver,
+    : public blink::WebPrintClient,
+      public content::RenderFrameObserver,
       public content::RenderFrameObserverTracker<PrintRenderFrameHelper>,
       public mojom::PrintRenderFrame {
  public:
@@ -213,7 +216,10 @@ class PrintRenderFrameHelper
     const base::WeakPtr<PrintRenderFrameHelper> weak_this_;
   };
 
-  // RenderFrameObserver implementation.
+  // blink::WebPrintClient:
+  void WillBeDestroyed() override;
+
+  // RenderFrameObserver:
   void OnDestruct() override;
   void DidStartNavigation(
       const GURL& url,
@@ -293,7 +299,8 @@ class PrintRenderFrameHelper
   // WARNING: |this| may be gone after this method returns.
   void Print(blink::WebLocalFrame* frame,
              const blink::WebNode& node,
-             PrintRequestType print_request_type);
+             PrintRequestType print_request_type,
+             std::unique_ptr<blink::WebScopedPagePauser> pauser);
 
   // Notification when printing is done - signal tear-down/free resources.
   void DidFinishPrinting(PrintingResult result);
@@ -475,6 +482,17 @@ class PrintRenderFrameHelper
     void InitWithFrame(blink::WebLocalFrame* web_frame);
     void InitWithNode(const blink::WebNode& web_node);
 
+    // Manual control of pausing/unpausing for special situations.
+    bool IsPaused() const;
+    void Pause();
+    std::unique_ptr<blink::WebScopedPagePauser> TakePauser();
+
+    // Dispatchs onbeforeprint/onafterprint events. Use these instead of calling
+    // the WebLocalFrame version on source_frame().
+    void DispatchBeforePrintEvent(
+        base::WeakPtr<PrintRenderFrameHelper> weak_this);
+    void DispatchAfterPrintEvent();
+
     // Does bookkeeping at the beginning of print preview.
     void OnPrintPreview();
 
@@ -560,6 +578,10 @@ class PrintRenderFrameHelper
 
     std::unique_ptr<PrepareFrameAndViewForPrint> prep_frame_view_;
 
+    // Manages when to pause between onbeforeprint and onafterprint events.
+    // https://html.spec.whatwg.org/C/#printing-steps
+    std::unique_ptr<blink::WebScopedPagePauser> pauser_;
+
     // The typefaces encountered in the content during document serialization.
     ContentProxySet typeface_content_info_;
 

third_party/blink/public/BUILD.gn

@@ -367,6 +367,7 @@ source_set("blink_headers") {
     "web/web_plugin_script_forbidden_scope.h",
     "web/web_popup_menu_info.h",
     "web/web_prerenderer_client.h",
+    "web/web_print_client.h",
     "web/web_print_page_description.h",
     "web/web_print_params.h",
     "web/web_print_preset_options.h",

third_party/blink/public/web/DEPS

@@ -6,6 +6,7 @@ include_rules = [
     "+base/macros.h",
     "+base/memory/ref_counted.h",
     "+base/memory/scoped_refptr.h",
+    "+base/memory/weak_ptr.h",
     "+base/strings",
     "+base/time/time.h",
     "+base/threading/thread_checker.h",

third_party/blink/public/web/web_local_frame.h

@@ -10,6 +10,7 @@
 
 #include "base/callback.h"
 #include "base/i18n/rtl.h"
+#include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/unguessable_token.h"
 #include "services/network/public/mojom/fetch_api.mojom-shared.h"
@@ -68,6 +69,7 @@ class WebFrameWidget;
 class WebInputMethodController;
 class WebPerformance;
 class WebPlugin;
+class WebPrintClient;
 class WebRange;
 class WebScriptExecutionCallback;
 class WebSpellCheckPanelHostClient;
@@ -650,7 +652,9 @@ class WebLocalFrame : public WebFrame {
   // Dispatch |beforeprint| event, and execute event handlers. They might detach
   // this frame from the owner WebView.
   // This function should be called before pairs of PrintBegin() and PrintEnd().
-  virtual void DispatchBeforePrintEvent() = 0;
+  // |print_client| is an optional weak pointer to the caller.
+  virtual void DispatchBeforePrintEvent(
+      base::WeakPtr<WebPrintClient> print_client) = 0;
 
   // Get the plugin to print, if any. The |constrain_to_node| parameter is the
   // same as the one for PrintBegin() below.

third_party/blink/public/web/web_print_client.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_PUBLIC_WEB_WEB_PRINT_CLIENT_H_
+#define THIRD_PARTY_BLINK_PUBLIC_WEB_WEB_PRINT_CLIENT_H_
+
+namespace blink {
+
+class WebPrintClient {
+ public:
+  // Let the client know it will be destroyed soon, to give it a chance to do
+  // any necessary cleanup work.
+  virtual void WillBeDestroyed() {}
+
+ protected:
+  virtual ~WebPrintClient() = default;
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_PUBLIC_WEB_WEB_PRINT_CLIENT_H_

third_party/blink/renderer/core/frame/web_local_frame_impl.cc

@@ -129,6 +129,7 @@
 #include "third_party/blink/public/web/web_node.h"
 #include "third_party/blink/public/web/web_performance.h"
 #include "third_party/blink/public/web/web_plugin.h"
+#include "third_party/blink/public/web/web_print_client.h"
 #include "third_party/blink/public/web/web_print_page_description.h"
 #include "third_party/blink/public/web/web_print_params.h"
 #include "third_party/blink/public/web/web_print_preset_options.h"
@@ -674,6 +675,7 @@ void WebLocalFrameImpl::Close() {
 
   if (print_context_)
     PrintEnd();
+  print_client_.reset();
 #if DCHECK_IS_ON()
   is_in_printing_ = false;
 #endif
@@ -1530,7 +1532,8 @@ WebPlugin* WebLocalFrameImpl::FocusedPluginIfInputMethodSupported() {
   return nullptr;
 }
 
-void WebLocalFrameImpl::DispatchBeforePrintEvent() {
+void WebLocalFrameImpl::DispatchBeforePrintEvent(
+    base::WeakPtr<WebPrintClient> print_client) {
 #if DCHECK_IS_ON()
   DCHECK(!is_in_printing_) << "DispatchAfterPrintEvent() should have been "
                               "called after the previous "
@@ -1538,6 +1541,8 @@ void WebLocalFrameImpl::DispatchBeforePrintEvent() {
   is_in_printing_ = true;
 #endif
 
+  print_client_ = print_client;
+
   // Disable BackForwardCache when printing API is used for now. When the page
   // navigates with BackForwardCache, we currently do not close the printing
   // popup properly.
@@ -1556,6 +1561,8 @@ void WebLocalFrameImpl::DispatchAfterPrintEvent() {
   is_in_printing_ = false;
 #endif
 
+  print_client_.reset();
+
   if (View())
     DispatchPrintEventRecursively(event_type_names::kAfterprint);
 }
@@ -2324,6 +2331,8 @@ void WebLocalFrameImpl::WillBeDetached() {
     dev_tools_agent_->WillBeDestroyed();
   if (find_in_page_)
     find_in_page_->Dispose();
+  if (print_client_)
+    print_client_->WillBeDestroyed();
 }
 
 void WebLocalFrameImpl::WillDetachParent() {

third_party/blink/renderer/core/frame/web_local_frame_impl.h

@@ -273,7 +273,8 @@ class CORE_EXPORT WebLocalFrameImpl final
   WebSize DocumentSize() const override;
   bool HasVisibleContent() const override;
   WebRect VisibleContentRect() const override;
-  void DispatchBeforePrintEvent() override;
+  void DispatchBeforePrintEvent(
+      base::WeakPtr<WebPrintClient> print_client) override;
   WebPlugin* GetPluginToPrint(const WebNode& constrain_to_node) override;
   int PrintBegin(const WebPrintParams&,
                  const WebNode& constrain_to_node) override;
@@ -518,6 +519,10 @@ class CORE_EXPORT WebLocalFrameImpl final
 
   Member<FindInPage> find_in_page_;
 
+  // Optional weak pointer to the WebPrintClient that initiated printing. Only
+  // valid when |is_in_printing_| is true.
+  base::WeakPtr<WebPrintClient> print_client_;
+
   // Valid between calls to BeginPrint() and EndPrint(). Containts the print
   // information. Is used by PrintPage().
   Member<ChromePrintContext> print_context_;

third_party/blink/renderer/core/page/chrome_client.cc

@@ -298,11 +298,6 @@ bool ChromeClient::Print(LocalFrame* frame) {
     return false;
   }
 
-  // Suspend pages in case the client method runs a new event loop that would
-  // otherwise cause the load to continue while we're in the middle of
-  // executing JavaScript.
-  ScopedPagePauser pauser;
-
   PrintDelegate(frame);
   return true;
 }