Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cherry-pick fix for 1231134 from chromium #30637

Merged
merged 3 commits into from Aug 31, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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();