Skip to content

Commit

Permalink
chore: cherry-pick fix for 1231134 from chromium (#30637)
Browse files Browse the repository at this point in the history
* chore: cherry-pick fix for 1231134 from chromium

* chore: update patches

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
  • Loading branch information
zcbenz and patchup[bot] committed Aug 31, 2021
1 parent dcb311d commit a55bc6a
Show file tree
Hide file tree
Showing 2 changed files with 164 additions and 0 deletions.
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -125,5 +125,6 @@ cherry-pick-ac9dc1235e28.patch
cherry-pick-4ce2abc17078.patch
cherry-pick-e2123a8e0943.patch
cherry-pick-1227933.patch
cherry-pick-1231134.patch
cherry-pick-1233564.patch
cherry-pick-1234009.patch
163 changes: 163 additions & 0 deletions patches/chromium/cherry-pick-1231134.patch
@@ -0,0 +1,163 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Lei Zhang <thestig@chromium.org>
Date: Tue, 10 Aug 2021 21:38:36 +0000
Subject: Do more class validity checks in PrintViewManagerBase.

PrintViewManagerBase runs a nested loop. In some situations,
PrintViewManagerBase and related classes like PrintViewManager and
PrintPreviewHandler can get deleted while the nested loop is running.
When this happens, the nested loop exists to a PrintViewManagerBase
that is no longer valid.

Use base::WeakPtrs liberally to check for this condition and exit
safely.

(cherry picked from commit a2cb1fb333d2faacb2fe1380f8d2621b5ee6af7e)

Bug: 1231134
Change-Id: I21ec131574331ce973d22594c11e70088147e149
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3057880
Reviewed-by: Alan Screen <awscreen@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#906269}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3086110
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4515@{#2024}
Cr-Branched-From: 488fc70865ddaa05324ac00a54a6eb783b4bc41c-refs/heads/master@{#885287}

diff --git a/chrome/browser/printing/print_view_manager.cc b/chrome/browser/printing/print_view_manager.cc
index 26271f6689f1f887b82333d281f26012d2752d63..e73d0972b1b6d6db1fbb3825791e5b90146c2c5e 100644
--- a/chrome/browser/printing/print_view_manager.cc
+++ b/chrome/browser/printing/print_view_manager.cc
@@ -89,7 +89,11 @@ bool PrintViewManager::PrintForSystemDialogNow(
DCHECK(!on_print_dialog_shown_callback_);
on_print_dialog_shown_callback_ = std::move(dialog_shown_callback);
is_switching_to_system_dialog_ = true;
+
+ auto weak_this = weak_factory_.GetWeakPtr();
DisconnectFromCurrentPrintJob();
+ if (!weak_this)
+ return false;

// Don't print / print preview crashed tabs.
if (IsCrashed())
diff --git a/chrome/browser/printing/print_view_manager.h b/chrome/browser/printing/print_view_manager.h
index 8b2f150a1e6a042898cba14c971e1f80d04116ca..b5cba8a2dfb9021527e4cc5569635770e85949b3 100644
--- a/chrome/browser/printing/print_view_manager.h
+++ b/chrome/browser/printing/print_view_manager.h
@@ -128,6 +128,11 @@ class PrintViewManager : public PrintViewManagerBase,

WEB_CONTENTS_USER_DATA_KEY_DECL();

+ // Keep this last so that all weak pointers will be invalidated at the
+ // beginning of destruction. Note that PrintViewManagerBase has its own
+ // base::WeakPtrFactory as well, but PrintViewManager should use this one.
+ base::WeakPtrFactory<PrintViewManager> weak_factory_{this};
+
DISALLOW_COPY_AND_ASSIGN(PrintViewManager);
};

diff --git a/chrome/browser/printing/print_view_manager_base.cc b/chrome/browser/printing/print_view_manager_base.cc
index 3b6e1d2609af952fa825688833387518cafdb352..3ddd927654e0134c28a7f73c6ec30b0c1ffe5c49 100644
--- a/chrome/browser/printing/print_view_manager_base.cc
+++ b/chrome/browser/printing/print_view_manager_base.cc
@@ -370,7 +370,10 @@ bool PrintViewManagerBase::PrintNow(content::RenderFrameHost* rfh,
bool silent,
base::Value settings,
CompletionCallback callback) {
+ auto weak_this = weak_ptr_factory_.GetWeakPtr();
DisconnectFromCurrentPrintJob();
+ if (!weak_this)
+ return false;

// Don't print / print preview crashed tabs.
if (IsCrashed())
@@ -850,6 +853,8 @@ bool PrintViewManagerBase::RenderAllMissingPagesNow() {
// or in DidPrintDocument(). The check is done in
// ShouldQuitFromInnerMessageLoop().
// BLOCKS until all the pages are received. (Need to enable recursive task)
+ // WARNING: Do not do any work after RunInnerMessageLoop() returns, as `this`
+ // may have gone away.
if (!RunInnerMessageLoop()) {
// This function is always called from DisconnectFromCurrentPrintJob() so we
// know that the job will be stopped/canceled in any case.
@@ -876,8 +881,11 @@ bool PrintViewManagerBase::CreateNewPrintJob(
DCHECK(query);

if (callback_.is_null()) {
+ auto weak_this = weak_ptr_factory_.GetWeakPtr();
// Disconnect the current |print_job_| only when calling window.print()
DisconnectFromCurrentPrintJob();
+ if (!weak_this)
+ return false;
}

// We can't print if there is no renderer.
@@ -906,7 +914,10 @@ bool PrintViewManagerBase::CreateNewPrintJob(
void PrintViewManagerBase::DisconnectFromCurrentPrintJob() {
// Make sure all the necessary rendered page are done. Don't bother with the
// return value.
+ auto weak_this = weak_ptr_factory_.GetWeakPtr();
bool result = RenderAllMissingPagesNow();
+ if (!weak_this)
+ return;

// Verify that assertion.
if (print_job_ && print_job_->document() &&
@@ -988,7 +999,10 @@ bool PrintViewManagerBase::RunInnerMessageLoop() {

quit_inner_loop_ = run_loop.QuitClosure();

+ auto weak_this = weak_ptr_factory_.GetWeakPtr();
run_loop.Run();
+ if (!weak_this)
+ return false;

// If the inner-loop quit closure is still set then we timed out.
bool success = !quit_inner_loop_;
diff --git a/chrome/browser/printing/print_view_manager_base.h b/chrome/browser/printing/print_view_manager_base.h
index ccb9808bdb334a78ed7b64dd3030caff52055ad6..b2ad5c1010b233e038cad9e2b5e39f3c0027d63e 100644
--- a/chrome/browser/printing/print_view_manager_base.h
+++ b/chrome/browser/printing/print_view_manager_base.h
@@ -122,6 +122,8 @@ class PrintViewManagerBase : public content::NotificationObserver,

// Makes sure the current print_job_ has all its data before continuing, and
// disconnect from it.
+ // WARNING: `this` may not be alive after DisconnectFromCurrentPrintJob()
+ // returns.
void DisconnectFromCurrentPrintJob();

// Manages the low-level talk to the printer.
@@ -168,6 +170,7 @@ class PrintViewManagerBase : public content::NotificationObserver,
// Requests the RenderView to render all the missing pages for the print job.
// No-op if no print job is pending. Returns true if at least one page has
// been requested to the renderer.
+ // WARNING: `this` may not be alive after RenderAllMissingPagesNow() returns.
bool RenderAllMissingPagesNow();

// Checks that synchronization is correct with |print_job_| based on |cookie|.
@@ -201,6 +204,7 @@ class PrintViewManagerBase : public content::NotificationObserver,
// while the blocking inner message loop is running. This is useful in cases
// where the RenderView is about to be destroyed while a printing job isn't
// finished.
+ // WARNING: `this` may not be alive after RunInnerMessageLoop() returns.
bool RunInnerMessageLoop();

// In the case of Scripted Printing, where the renderer is controlling the
diff --git a/chrome/browser/ui/webui/print_preview/print_preview_handler.cc b/chrome/browser/ui/webui/print_preview/print_preview_handler.cc
index 82a2ac9bc0e5d32438a6ec6bd500cae7da8739fe..fe8a580e2fc5dbb74a57bf8488888a9696ce0007 100644
--- a/chrome/browser/ui/webui/print_preview/print_preview_handler.cc
+++ b/chrome/browser/ui/webui/print_preview/print_preview_handler.cc
@@ -726,9 +726,12 @@ void PrintPreviewHandler::HandleShowSystemDialog(
if (!initiator)
return;

+ auto weak_this = weak_factory_.GetWeakPtr();
auto* print_view_manager = PrintViewManager::FromWebContents(initiator);
print_view_manager->PrintForSystemDialogNow(base::BindOnce(
&PrintPreviewHandler::ClosePreviewDialog, weak_factory_.GetWeakPtr()));
+ if (!weak_this)
+ return;

// Cancel the pending preview request if exists.
print_preview_ui()->OnCancelPendingPreviewRequest();

0 comments on commit a55bc6a

Please sign in to comment.