Skip to content

Commit

Permalink
[M90-LTS] Do more class validity checks in PrintViewManagerBase.
Browse files Browse the repository at this point in the history
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 a2cb1fb)

Bug: 1231134
Change-Id: I21ec131574331ce973d22594c11e70088147e149
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3057880
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/+/3100154
Reviewed-by: Artem Sumaneev <asumaneev@google.com>
Owners-Override: Artem Sumaneev <asumaneev@google.com>
Commit-Queue: Roger Felipe Zanoni da Silva <rzanoni@google.com>
Cr-Commit-Position: refs/branch-heads/4430@{#1574}
Cr-Branched-From: e5ce7dc-refs/heads/master@{#857950}
  • Loading branch information
leizleiz authored and Chromium LUCI CQ committed Aug 23, 2021
1 parent 916bf13 commit f4db569
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 0 deletions.
4 changes: 4 additions & 0 deletions chrome/browser/printing/print_view_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/printing/print_view_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};

Expand Down
14 changes: 14 additions & 0 deletions chrome/browser/printing/print_view_manager_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,10 @@ PrintViewManagerBase::~PrintViewManagerBase() {
}

bool PrintViewManagerBase::PrintNow(content::RenderFrameHost* rfh) {
auto weak_this = weak_ptr_factory_.GetWeakPtr();
DisconnectFromCurrentPrintJob();
if (!weak_this)
return false;

// Don't print / print preview crashed tabs.
if (IsCrashed())
Expand Down Expand Up @@ -823,6 +826,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.
Expand All @@ -849,7 +854,10 @@ bool PrintViewManagerBase::CreateNewPrintJob(
DCHECK(query);

// Disconnect the current |print_job_|.
auto weak_this = weak_ptr_factory_.GetWeakPtr();
DisconnectFromCurrentPrintJob();
if (!weak_this)
return false;

// We can't print if there is no renderer.
if (!web_contents()->GetMainFrame()->GetRenderViewHost() ||
Expand Down Expand Up @@ -879,7 +887,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() &&
Expand Down Expand Up @@ -953,7 +964,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_;
Expand Down
4 changes: 4 additions & 0 deletions chrome/browser/printing/print_view_manager_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,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.
Expand Down Expand Up @@ -178,6 +180,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|.
Expand Down Expand Up @@ -211,6 +214,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -750,9 +750,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();
Expand Down

0 comments on commit f4db569

Please sign in to comment.