Revert "Revert "Print with dead subframes for pdf composition""

This reverts commit 2093bb8f.

Reason for revert: The last flaky test shown in crbug.com/817068 is before the fix https://chromium.googlesource.com/chromium/src.git/+/788c11139b869d1fc5b82cbbd238af2fd543535e. According to flaky dashboard, there is no test failure from these browser tests after the fix landed. So it should be good to stay.

Original change's description:
> Revert "Print with dead subframes for pdf composition"
> 
> This reverts commit d20c0123.
> 
> Reason for revert: The SubframeUnavailableBeforePrint test added in this CL is flaky (see crbug.com/817068).
> 
> Original change's description:
> > Print with dead subframes for pdf composition
> >
> > Handle two cases with dead subframes for pdf composition:
> > -- When a web page has a dead subframe prior to printing, we need to
> > detect the liveness of the subframe, and avoid requesting printing for
> > such frame;
> > -- If after we request printing a subframe, the subframe dies, we add
> > monitoring for render frame's closed event to check whether it is one
> > of our pending ones. If so, notify pdf compositor service about that.
> >
> > We add an interface in pdf compositor service to be notified about
> > the frame's unavailability for either of the above cases.
> >
> > In this CL, we also add a map to record the subframes that are already
> > printed and use it to avoid printing the same ones repeatedly.
> >
> > BUG=814086
> >
> > Change-Id: Ibd69dd21a6498a5c2784dfe892bc5803e84fa6f0
> > Reviewed-on: https://chromium-review.googlesource.com/932018
> > Commit-Queue: Wei Li <weili@chromium.org>
> > Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> > Reviewed-by: Lei Zhang <thestig@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#539291}
> 
> TBR=dcheng@chromium.org,thestig@chromium.org,weili@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> TBR=dcheng@chromium.org
> 
> Bug: 814086
> Change-Id: I2062517c3f955523618be23a52ad507c4da3de90
> Reviewed-on: https://chromium-review.googlesource.com/941201
> Commit-Queue: Colin Blundell <blundell@chromium.org>
> Reviewed-by: Colin Blundell <blundell@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#539818}

TBR=dcheng@chromium.org,thestig@chromium.org,weili@chromium.org,blundell@chromium.org

Change-Id: Iab653dd94f117956059bf950a4c54fd133270c07
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 814086
Reviewed-on: https://chromium-review.googlesource.com/941501Reviewed-by: Wei Li <weili@chromium.org>
Commit-Queue: Wei Li <weili@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539843}

chrome/browser/printing/print_browsertest.cc

@@ -102,6 +102,33 @@ class TestPrintFrameContentMsgFilter : public content::BrowserMessageFilter {
   base::RepeatingClosure msg_callback_;
 };
 
+class KillPrintFrameContentMsgFilter : public content::BrowserMessageFilter {
+ public:
+  explicit KillPrintFrameContentMsgFilter(content::RenderProcessHost* rph)
+      : content::BrowserMessageFilter(PrintMsgStart), rph_(rph) {}
+
+  bool OnMessageReceived(const IPC::Message& message) override {
+    // Only handle PrintHostMsg_DidPrintFrameContent message.
+    bool handled = true;
+    IPC_BEGIN_MESSAGE_MAP(KillPrintFrameContentMsgFilter, message)
+      IPC_MESSAGE_HANDLER(PrintHostMsg_DidPrintFrameContent, KillRenderProcess)
+      IPC_MESSAGE_UNHANDLED(handled = false)
+    IPC_END_MESSAGE_MAP()
+    return handled;
+  }
+
+ private:
+  ~KillPrintFrameContentMsgFilter() override {}
+
+  void KillRenderProcess(int document_cookie,
+                         const PrintHostMsg_DidPrintContent_Params& param) {
+    base::ScopedAllowBaseSyncPrimitivesForTesting allow_sync_primitives;
+    rph_->Shutdown(0, true);
+  }
+
+  content::RenderProcessHost* rph_;
+};
+
 }  // namespace
 
 class PrintBrowserTest : public InProcessBrowserTest {
@@ -171,6 +198,17 @@ class PrintBrowserTest : public InProcessBrowserTest {
   std::unique_ptr<base::RunLoop> run_loop_;
 };
 
+class SitePerProcessPrintBrowserTest : public PrintBrowserTest {
+ public:
+  SitePerProcessPrintBrowserTest() {}
+  ~SitePerProcessPrintBrowserTest() override {}
+
+  // content::BrowserTestBase
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    content::IsolateAllSitesForTesting(command_line);
+  }
+};
+
 class IsolateOriginsPrintBrowserTest : public PrintBrowserTest {
  public:
   static constexpr char kIsolatedSite[] = "b.com";
@@ -342,6 +380,58 @@ IN_PROC_BROWSER_TEST_F(PrintBrowserTest, PrintSubframeABA) {
   WaitUntilMessagesReceived();
 }
 
+// Printing a web page with a dead subframe for site per process should succeed.
+// This test passes whenever the print preview is rendered. This should not be
+// a timed out test which indicates the print preview hung.
+IN_PROC_BROWSER_TEST_F(SitePerProcessPrintBrowserTest,
+                       SubframeUnavailableBeforePrint) {
+  ASSERT_TRUE(embedded_test_server()->Started());
+  GURL url(
+      embedded_test_server()->GetURL("/printing/content_with_iframe.html"));
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  content::WebContents* original_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_EQ(2u, original_contents->GetAllFrames().size());
+  content::RenderFrameHost* test_frame = original_contents->GetAllFrames()[1];
+  ASSERT_TRUE(test_frame);
+  ASSERT_TRUE(test_frame->IsRenderFrameLive());
+  // Wait for the renderer to be down.
+  content::RenderProcessHostWatcher render_process_watcher(
+      test_frame->GetProcess(),
+      content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  // Shutdown the subframe.
+  ASSERT_TRUE(test_frame->GetProcess()->Shutdown(0, false));
+  render_process_watcher.Wait();
+  ASSERT_FALSE(test_frame->IsRenderFrameLive());
+
+  PrintAndWaitUntilPreviewIsReady(/*print_only_selection=*/false);
+}
+
+// If a subframe dies during printing, the page printing should still succeed.
+// This test passes whenever the print preview is rendered. This should not be
+// a timed out test which indicates the print preview hung.
+IN_PROC_BROWSER_TEST_F(SitePerProcessPrintBrowserTest,
+                       SubframeUnavailableDuringPrint) {
+  ASSERT_TRUE(embedded_test_server()->Started());
+  GURL url(
+      embedded_test_server()->GetURL("/printing/content_with_iframe.html"));
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  content::WebContents* original_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_EQ(2u, original_contents->GetAllFrames().size());
+  content::RenderFrameHost* subframe = original_contents->GetAllFrames()[1];
+  ASSERT_TRUE(subframe);
+  auto* subframe_rph = subframe->GetProcess();
+
+  auto filter =
+      base::MakeRefCounted<KillPrintFrameContentMsgFilter>(subframe_rph);
+  subframe_rph->AddFilter(filter.get());
+
+  PrintAndWaitUntilPreviewIsReady(/*print_only_selection=*/false);
+}
+
 // Printing preview a web page with an iframe from an isolated origin.
 // This test passes whenever the print preview is rendered. This should not be
 // a timed out test which indicates the print preview hung or crash.

components/printing/browser/print_composite_client.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/memory/shared_memory_handle.h"
+#include "base/stl_util.h"
 #include "components/printing/common/print_messages.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
@@ -47,6 +48,22 @@ bool PrintCompositeClient::OnMessageReceived(
   return handled;
 }
 
+void PrintCompositeClient::RenderFrameDeleted(
+    content::RenderFrameHost* render_frame_host) {
+  auto frame_guid = GenerateFrameGuid(render_frame_host->GetProcess()->GetID(),
+                                      render_frame_host->GetRoutingID());
+  auto iter = pending_subframe_cookies_.find(frame_guid);
+  if (iter != pending_subframe_cookies_.end()) {
+    // When a subframe we are expecting is deleted, we should notify pdf
+    // compositor service.
+    for (auto doc_cookie : iter->second) {
+      auto& compositor = GetCompositeRequest(doc_cookie);
+      compositor->NotifyUnavailableSubframe(frame_guid);
+    }
+    pending_subframe_cookies_.erase(iter);
+  }
+}
+
 void PrintCompositeClient::OnDidPrintFrameContent(
     content::RenderFrameHost* render_frame_host,
     int document_cookie,
@@ -55,7 +72,6 @@ void PrintCompositeClient::OnDidPrintFrameContent(
   // is done here. Most of it will be directly forwarded to pdf compositor
   // service.
   auto& compositor = GetCompositeRequest(document_cookie);
-  DCHECK(compositor.is_bound());
 
   mojo::ScopedSharedBufferHandle buffer_handle = mojo::WrapSharedMemoryHandle(
       params.metafile_data_handle, params.data_size,
@@ -66,6 +82,12 @@ void PrintCompositeClient::OnDidPrintFrameContent(
       frame_guid, std::move(buffer_handle),
       ConvertContentInfoMap(web_contents(), render_frame_host,
                             params.subframe_content_info));
+
+  // Update our internal states about this frame.
+  pending_subframe_cookies_[frame_guid].erase(document_cookie);
+  if (pending_subframe_cookies_[frame_guid].empty())
+    pending_subframe_cookies_.erase(frame_guid);
+  printed_subframes_[document_cookie].insert(frame_guid);
 }
 
 void PrintCompositeClient::PrintCrossProcessSubframe(
@@ -75,9 +97,33 @@ void PrintCompositeClient::PrintCrossProcessSubframe(
   PrintMsg_PrintFrame_Params params;
   params.printable_area = rect;
   params.document_cookie = document_cookie;
-  // Send the request to the destination frame.
-  subframe_host->Send(
-      new PrintMsg_PrintFrameContent(subframe_host->GetRoutingID(), params));
+  uint64_t frame_guid = GenerateFrameGuid(subframe_host->GetProcess()->GetID(),
+                                          subframe_host->GetRoutingID());
+  if (subframe_host->IsRenderFrameLive()) {
+    auto subframe_iter = printed_subframes_.find(document_cookie);
+    if (subframe_iter != printed_subframes_.end() &&
+        base::ContainsKey(subframe_iter->second, frame_guid)) {
+      // If this frame is already printed, no need to print again.
+      return;
+    }
+
+    auto cookie_iter = pending_subframe_cookies_.find(frame_guid);
+    if (cookie_iter != pending_subframe_cookies_.end() &&
+        base::ContainsKey(cookie_iter->second, document_cookie)) {
+      // If this frame is being printed, no need to print again.
+      return;
+    }
+
+    // Send the request to the destination frame.
+    subframe_host->Send(
+        new PrintMsg_PrintFrameContent(subframe_host->GetRoutingID(), params));
+    pending_subframe_cookies_[frame_guid].insert(document_cookie);
+  } else {
+    // When the subframe is dead, no need to send message,
+    // just notify the service.
+    auto& compositor = GetCompositeRequest(document_cookie);
+    compositor->NotifyUnavailableSubframe(frame_guid);
+  }
 }
 
 void PrintCompositeClient::DoCompositePageToPdf(
@@ -149,6 +195,8 @@ void PrintCompositeClient::OnDidCompositeDocumentToPdf(
     printing::mojom::PdfCompositor::Status status,
     mojo::ScopedSharedBufferHandle handle) {
   RemoveCompositeRequest(document_cookie);
+  // Clear all stored printed subframes.
+  printed_subframes_.erase(document_cookie);
   std::move(callback).Run(status, std::move(handle));
 }
 
@@ -179,8 +227,10 @@ ContentToFrameMap PrintCompositeClient::ConvertContentInfoMap(
 
 mojom::PdfCompositorPtr& PrintCompositeClient::GetCompositeRequest(int cookie) {
   auto iter = compositor_map_.find(cookie);
-  if (iter != compositor_map_.end())
+  if (iter != compositor_map_.end()) {
+    DCHECK(iter->second.is_bound());
     return iter->second;
+  }
 
   auto iterator =
       compositor_map_.emplace(cookie, CreateCompositeRequest()).first;

components/printing/browser/print_composite_client.h

@@ -8,6 +8,7 @@
 #include <map>
 #include <memory>
 
+#include "base/containers/flat_set.h"
 #include "base/optional.h"
 #include "components/printing/service/public/cpp/pdf_service_mojo_types.h"
 #include "components/printing/service/public/interfaces/pdf_compositor.mojom.h"
@@ -35,6 +36,7 @@ class PrintCompositeClient
   // content::WebContentsObserver
   bool OnMessageReceived(const IPC::Message& message,
                          content::RenderFrameHost* render_frame_host) override;
+  void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
 
   // IPC message handler.
   void OnDidPrintFrameContent(
@@ -123,6 +125,14 @@ class PrintCompositeClient
   // requests.
   std::map<int, mojom::PdfCompositorPtr> compositor_map_;
 
+  // Stores the mapping between render frame's global unique id and document
+  // cookies that requested such frame.
+  std::map<uint64_t, base::flat_set<int>> pending_subframe_cookies_;
+
+  // Stores the mapping between document cookie and all the printed subframes
+  // for that document.
+  std::map<int, base::flat_set<uint64_t>> printed_subframes_;
+
   DISALLOW_COPY_AND_ASSIGN(PrintCompositeClient);
 };
 

components/printing/service/pdf_compositor_impl.cc

@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "base/memory/shared_memory_handle.h"
+#include "base/stl_util.h"
 #include "components/printing/service/public/cpp/pdf_service_mojo_types.h"
 #include "components/printing/service/public/cpp/pdf_service_mojo_utils.h"
 #include "mojo/public/cpp/system/platform_handle.h"
@@ -26,6 +27,21 @@ PdfCompositorImpl::PdfCompositorImpl(
 
 PdfCompositorImpl::~PdfCompositorImpl() = default;
 
+void PdfCompositorImpl::NotifyUnavailableSubframe(uint64_t frame_guid) {
+  // Add this frame into the map.
+  DCHECK(!base::ContainsKey(frame_info_map_, frame_guid));
+  auto& frame_info =
+      frame_info_map_.emplace(frame_guid, std::make_unique<FrameInfo>())
+          .first->second;
+  frame_info->composited = true;
+  // Set content to be nullptr so it will be replaced by an empty picture during
+  // deserialization of its parent.
+  frame_info->content = nullptr;
+
+  // Update the requests in case any of them might be waiting for this frame.
+  UpdateRequestsWithSubframeInfo(frame_guid, std::vector<uint64_t>());
+}
+
 void PdfCompositorImpl::AddSubframeContent(
     uint64_t frame_guid,
     mojo::ScopedSharedBufferHandle serialized_content,
@@ -56,6 +72,33 @@ void PdfCompositorImpl::AddSubframeContent(
       pending_subframes.push_back(subframe_guid);
   }
 
+  // Update the requests in case any of them is waiting for this frame.
+  UpdateRequestsWithSubframeInfo(frame_guid, pending_subframes);
+}
+
+void PdfCompositorImpl::CompositePageToPdf(
+    uint64_t frame_guid,
+    uint32_t page_num,
+    mojo::ScopedSharedBufferHandle serialized_content,
+    const ContentToFrameMap& subframe_content_map,
+    mojom::PdfCompositor::CompositePageToPdfCallback callback) {
+  HandleCompositionRequest(frame_guid, page_num, std::move(serialized_content),
+                           subframe_content_map, std::move(callback));
+}
+
+void PdfCompositorImpl::CompositeDocumentToPdf(
+    uint64_t frame_guid,
+    mojo::ScopedSharedBufferHandle serialized_content,
+    const ContentToFrameMap& subframe_content_map,
+    mojom::PdfCompositor::CompositeDocumentToPdfCallback callback) {
+  HandleCompositionRequest(frame_guid, base::nullopt,
+                           std::move(serialized_content), subframe_content_map,
+                           std::move(callback));
+}
+
+void PdfCompositorImpl::UpdateRequestsWithSubframeInfo(
+    uint64_t frame_guid,
+    const std::vector<uint64_t>& pending_subframes) {
   // Check for each request's pending list.
   for (auto it = requests_.begin(); it != requests_.end();) {
     auto& request = *it;
@@ -81,26 +124,6 @@ void PdfCompositorImpl::AddSubframeContent(
   }
 }
 
-void PdfCompositorImpl::CompositePageToPdf(
-    uint64_t frame_guid,
-    uint32_t page_num,
-    mojo::ScopedSharedBufferHandle serialized_content,
-    const ContentToFrameMap& subframe_content_map,
-    mojom::PdfCompositor::CompositePageToPdfCallback callback) {
-  HandleCompositionRequest(frame_guid, page_num, std::move(serialized_content),
-                           subframe_content_map, std::move(callback));
-}
-
-void PdfCompositorImpl::CompositeDocumentToPdf(
-    uint64_t frame_guid,
-    mojo::ScopedSharedBufferHandle serialized_content,
-    const ContentToFrameMap& subframe_content_map,
-    mojom::PdfCompositor::CompositeDocumentToPdfCallback callback) {
-  HandleCompositionRequest(frame_guid, base::nullopt,
-                           std::move(serialized_content), subframe_content_map,
-                           std::move(callback));
-}
-
 bool PdfCompositorImpl::IsReadyToComposite(
     uint64_t frame_guid,
     const ContentToFrameMap& subframe_content_map,

components/printing/service/pdf_compositor_impl.h

@@ -33,6 +33,7 @@ class PdfCompositorImpl : public mojom::PdfCompositor {
   ~PdfCompositorImpl() override;
 
   // mojom::PdfCompositor
+  void NotifyUnavailableSubframe(uint64_t frame_guid) override;
   void AddSubframeContent(
       uint64_t frame_guid,
       mojo::ScopedSharedBufferHandle serialized_content,
@@ -127,6 +128,12 @@ class PdfCompositorImpl : public mojom::PdfCompositor {
     CompositeToPdfCallback callback;
   };
 
+  // Check whether any request is waiting for the specific subframe, if so,
+  // update its dependecy with the subframe's pending child frames.
+  void UpdateRequestsWithSubframeInfo(
+      uint64_t frame_guid,
+      const std::vector<uint64_t>& pending_subframes);
+
   // Check whether the frame with a list of subframe content is ready to
   // composite. If not, return all unavailable frames' ids in
   // |pending_subframes|.

components/printing/service/pdf_compositor_impl_unittest.cc

@@ -324,4 +324,25 @@ TEST_F(PdfCompositorImplTest, MultiRequestsDepOrder) {
                           std::move(subframe_content_map));
 }
 
+TEST_F(PdfCompositorImplTest, NotifyUnavailableSubframe) {
+  MockPdfCompositorImpl impl;
+  // Page 0 with frame 3 has content 1, which refers to frame 8.
+  // When the content is not available, the request is not fulfilled.
+  ContentToFrameMap subframe_content_map;
+  subframe_content_map[1u] = 8u;
+  EXPECT_CALL(impl, OnFulfillRequest(testing::_, testing::_)).Times(0);
+  impl.CompositePageToPdf(
+      3u, 0, mojo::SharedBufferHandle::Create(10),
+      std::move(subframe_content_map),
+      base::BindOnce(&PdfCompositorImplTest::OnCompositeToPdfCallback,
+                     base::Unretained(this)));
+  testing::Mock::VerifyAndClearExpectations(&impl);
+
+  // Notifies that frame 8's unavailable, the previous request should be
+  // fulfilled.
+  EXPECT_CALL(impl, OnFulfillRequest(3u, 0)).Times(1);
+  impl.NotifyUnavailableSubframe(8u);
+  testing::Mock::VerifyAndClearExpectations(&impl);
+}
+
 }  // namespace printing

components/printing/service/public/interfaces/pdf_compositor.mojom

@@ -15,6 +15,12 @@ interface PdfCompositor {
     COMPOSTING_FAILURE,
   };
 
+  // Notifies that a subframe is unavailable, such as the render frame process
+  // hosting it crashed or terminated. The subframe will be composited with no
+  // content in the composited result.
+  // |frame_guid| is this subframe's global unique id.
+  NotifyUnavailableSubframe(uint64 frame_guid);
+
   // Add the content of a subframe for composition.
   // |frame_guid| is this subframe's global unique id.
   // |serialized_content| points to a buffer of a serialized Skia picture which

printing/common/pdf_metafile_utils.cc

@@ -77,7 +77,7 @@ sk_sp<SkPicture> DeserializeOopPicture(const void* data,
 
   auto* context = reinterpret_cast<DeserializationContext*>(ctx);
   auto iter = context->find(pic_id);
-  if (iter == context->end()) {
+  if (iter == context->end() || !iter->second) {
     // When we don't have the out-of-process picture available, we return
     // an empty picture. Returning a nullptr will cause the deserialization
     // crash.