Fix Print Preview refresh

Previously, it was not possible to refresh Print Preview using dev
tools. However, it is now possible to do so, and as a result
renderer/compositor messages can return to a refreshed Print Preview
page that is not expecting any messages, leading to a renderer kill.

Since the preview ui id is used only for routing messages from the
PrintPreviewMessageHandler back to the appropriate PrintPreviewUI
instance, which then forwards them to the PrintPreviewHandler that
ultimately sends them to the Print Preview Web UI, it should not be
initialized until the web UI is ready to receive messages. It should
also be reset whenever the web UI is reloaded. This can be accomplished
by setting the preview ui id in OnJavascriptAllowed() and clearing it
in OnJavascriptDisallowed() or the PrintPreviewUI destructor, whichever
occurs first.

Refreshed Preview UIs will then have a different unique identifier
after each refresh, which ensures renderer messages intended for
previous instances of the web UI page will be dropped instead of being
received by the handler and causing a renderer kill.

Bug: 874744
Change-Id: Ia43aa9abb97e7952ec3761e5fb016f6513c1c982
Reviewed-on: https://chromium-review.googlesource.com/c/1302676Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603609}

chrome/browser/printing/print_preview_message_handler.cc

@@ -90,8 +90,8 @@ PrintPreviewUI* PrintPreviewMessageHandler::GetPrintPreviewUI(
     return nullptr;
   PrintPreviewUI* preview_ui =
       static_cast<PrintPreviewUI*>(dialog->GetWebUI()->GetController());
-  return preview_ui->GetIDForPrintPreviewUI() == preview_ui_id ? preview_ui
-                                                               : nullptr;
+  base::Optional<int32_t> id = preview_ui->GetIDForPrintPreviewUI();
+  return (id && *id == preview_ui_id) ? preview_ui : nullptr;
 }
 
 void PrintPreviewMessageHandler::OnRequestPrintPreview(

chrome/browser/ui/webui/print_preview/print_preview_handler.cc

@@ -628,6 +628,7 @@ void PrintPreviewHandler::RegisterMessages() {
 }
 
 void PrintPreviewHandler::OnJavascriptAllowed() {
+  print_preview_ui()->SetPreviewUIId();
   // Now that the UI is initialized, any future account changes will require
   // a printer list refresh.
   RegisterForGaiaCookieChanges();
@@ -637,6 +638,7 @@ void PrintPreviewHandler::OnJavascriptDisallowed() {
   // Normally the handler and print preview will be destroyed together, but
   // this is necessary for refresh or navigation from the chrome://print page.
   weak_factory_.InvalidateWeakPtrs();
+  print_preview_ui()->ClearPreviewUIId();
   preview_callbacks_.clear();
   preview_failures_.clear();
   UnregisterForGaiaCookieChanges();
@@ -773,7 +775,7 @@ void PrintPreviewHandler::HandleGetPreview(const base::ListValue* args) {
   // Add an additional key in order to identify |print_preview_ui| later on
   // when calling PrintPreviewUI::ShouldCancelRequest() on the IO thread.
   settings->SetInteger(printing::kPreviewUIID,
-                       print_preview_ui()->GetIDForPrintPreviewUI());
+                       print_preview_ui()->GetIDForPrintPreviewUI().value());
 
   // Increment request count.
   ++regenerate_preview_request_count_;

chrome/browser/ui/webui/print_preview/print_preview_ui.cc

@@ -584,17 +584,13 @@ PrintPreviewUI::PrintPreviewUI(content::WebUI* web_ui,
                                std::unique_ptr<PrintPreviewHandler> handler)
     : ConstrainedWebDialogUI(web_ui),
       initial_preview_start_time_(base::TimeTicks::Now()),
-      id_(g_print_preview_ui_id_map.Get().Add(this)),
       handler_(handler.get()) {
   web_ui->AddMessageHandler(std::move(handler));
-
-  g_print_preview_request_id_map.Get().Set(id_, -1);
 }
 
 PrintPreviewUI::PrintPreviewUI(content::WebUI* web_ui)
     : ConstrainedWebDialogUI(web_ui),
       initial_preview_start_time_(base::TimeTicks::Now()),
-      id_(g_print_preview_ui_id_map.Get().Add(this)),
       handler_(CreatePrintPreviewHandlers(web_ui)) {
   // Set up the chrome://print/ data source.
   Profile* profile = Profile::FromWebUI(web_ui);
@@ -611,31 +607,37 @@ PrintPreviewUI::PrintPreviewUI(content::WebUI* web_ui)
 
   // Set up the chrome://theme/ source.
   content::URLDataSource::Add(profile, std::make_unique<ThemeSource>(profile));
-
-  g_print_preview_request_id_map.Get().Set(id_, -1);
 }
 
 PrintPreviewUI::~PrintPreviewUI() {
-  PrintPreviewDataService::GetInstance()->RemoveEntry(id_);
-  g_print_preview_request_id_map.Get().Erase(id_);
-  g_print_preview_ui_id_map.Get().Remove(id_);
+  ClearPreviewUIId();
+}
+
+void PrintPreviewUI::ClearPreviewUIId() {
+  if (!id_)
+    return;
+
+  PrintPreviewDataService::GetInstance()->RemoveEntry(*id_);
+  g_print_preview_request_id_map.Get().Erase(*id_);
+  g_print_preview_ui_id_map.Get().Remove(*id_);
+  id_.reset();
 }
 
 void PrintPreviewUI::GetPrintPreviewDataForIndex(
     int index,
     scoped_refptr<base::RefCountedMemory>* data) const {
-  PrintPreviewDataService::GetInstance()->GetDataEntry(id_, index, data);
+  PrintPreviewDataService::GetInstance()->GetDataEntry(*id_, index, data);
 }
 
 void PrintPreviewUI::SetPrintPreviewDataForIndex(
     int index,
     scoped_refptr<base::RefCountedMemory> data) {
-  PrintPreviewDataService::GetInstance()->SetDataEntry(id_, index,
+  PrintPreviewDataService::GetInstance()->SetDataEntry(*id_, index,
                                                        std::move(data));
 }
 
 void PrintPreviewUI::ClearAllPreviewData() {
-  PrintPreviewDataService::GetInstance()->RemoveEntry(id_);
+  PrintPreviewDataService::GetInstance()->RemoveEntry(*id_);
 }
 
 void PrintPreviewUI::SetInitiatorTitle(
@@ -689,7 +691,7 @@ bool PrintPreviewUI::ShouldCancelRequest(const PrintHostMsg_PreviewIds& ids) {
   return ids.request_id != current_id;
 }
 
-int32_t PrintPreviewUI::GetIDForPrintPreviewUI() const {
+base::Optional<int32_t> PrintPreviewUI::GetIDForPrintPreviewUI() const {
   return id_;
 }
 
@@ -728,7 +730,7 @@ void PrintPreviewUI::OnPrintPreviewRequest(int request_id) {
     UMA_HISTOGRAM_TIMES("PrintPreview.InitializationTime",
                         base::TimeTicks::Now() - initial_preview_start_time_);
   }
-  g_print_preview_request_id_map.Get().Set(id_, request_id);
+  g_print_preview_request_id_map.Get().Set(*id_, request_id);
 }
 
 void PrintPreviewUI::OnDidStartPreview(
@@ -799,7 +801,7 @@ void PrintPreviewUI::OnDidPreviewPage(
 
   if (g_testing_delegate)
     g_testing_delegate->DidRenderPreviewPage(web_ui()->GetWebContents());
-  handler_->SendPagePreviewReady(page_number, id_, preview_request_id);
+  handler_->SendPagePreviewReady(page_number, *id_, preview_request_id);
 }
 
 void PrintPreviewUI::OnPreviewDataIsAvailable(
@@ -823,11 +825,11 @@ void PrintPreviewUI::OnPreviewDataIsAvailable(
   SetPrintPreviewDataForIndex(printing::COMPLETE_PREVIEW_DOCUMENT_INDEX,
                               std::move(data));
 
-  handler_->OnPrintPreviewReady(id_, preview_request_id);
+  handler_->OnPrintPreviewReady(*id_, preview_request_id);
 }
 
 void PrintPreviewUI::OnCancelPendingPreviewRequest() {
-  g_print_preview_request_id_map.Get().Set(id_, -1);
+  g_print_preview_request_id_map.Get().Set(*id_, -1);
 }
 
 void PrintPreviewUI::OnPrintPreviewFailed(int request_id) {
@@ -906,3 +908,9 @@ void PrintPreviewUI::SetPrintPreviewDataForIndexForTest(
 void PrintPreviewUI::ClearAllPreviewDataForTest() {
   ClearAllPreviewData();
 }
+
+void PrintPreviewUI::SetPreviewUIId() {
+  DCHECK(!id_);
+  id_ = g_print_preview_ui_id_map.Get().Add(this);
+  g_print_preview_request_id_map.Get().Set(*id_, -1);
+}

chrome/browser/ui/webui/print_preview/print_preview_ui.h

@@ -15,6 +15,7 @@
 #include "base/macros.h"
 #include "base/memory/read_only_shared_memory_region.h"
 #include "base/memory/ref_counted.h"
+#include "base/optional.h"
 #include "base/time/time.h"
 #include "chrome/browser/ui/webui/constrained_web_dialog_ui.h"
 #include "ui/gfx/geometry/rect.h"
@@ -94,7 +95,7 @@ class PrintPreviewUI : public ConstrainedWebDialogUI {
   static bool ShouldCancelRequest(const PrintHostMsg_PreviewIds& ids);
 
   // Returns an id to uniquely identify this PrintPreviewUI.
-  int32_t GetIDForPrintPreviewUI() const;
+  base::Optional<int32_t> GetIDForPrintPreviewUI() const;
 
   // Notifies the Web UI of a print preview request with |request_id|.
   virtual void OnPrintPreviewRequest(int request_id);
@@ -194,6 +195,14 @@ class PrintPreviewUI : public ConstrainedWebDialogUI {
   // See ClearAllPreviewData().
   void ClearAllPreviewDataForTest();
 
+  // Sets a new valid Print Preview UI ID for this instance. Called by
+  // PrintPreviewHandler in OnJavascriptAllowed().
+  void SetPreviewUIId();
+
+  // Clears the UI ID. Called by PrintPreviewHandler in
+  // OnJavascriptDisallowed().
+  void ClearPreviewUIId();
+
  protected:
   // Alternate constructor for tests
   PrintPreviewUI(content::WebUI* web_ui,
@@ -217,7 +226,7 @@ class PrintPreviewUI : public ConstrainedWebDialogUI {
 
   // The unique ID for this class instance. Stored here to avoid calling
   // GetIDForPrintPreviewUI() everywhere.
-  const int32_t id_;
+  base::Optional<int32_t> id_;
 
   // Weak pointer to the WebUI handler.
   PrintPreviewHandler* const handler_;

chrome/browser/ui/webui/print_preview/print_preview_ui_unittest.cc

@@ -87,6 +87,7 @@ TEST_F(PrintPreviewUIUnitTest, PrintPreviewData) {
   PrintPreviewUI* preview_ui = static_cast<PrintPreviewUI*>(
       preview_dialog->GetWebUI()->GetController());
   ASSERT_TRUE(preview_ui);
+  preview_ui->SetPreviewUIId();
 
   scoped_refptr<base::RefCountedMemory> data;
   preview_ui->GetPrintPreviewDataForIndex(
@@ -134,6 +135,7 @@ TEST_F(PrintPreviewUIUnitTest, PrintPreviewDraftPages) {
   PrintPreviewUI* preview_ui = static_cast<PrintPreviewUI*>(
       preview_dialog->GetWebUI()->GetController());
   ASSERT_TRUE(preview_ui);
+  preview_ui->SetPreviewUIId();
 
   scoped_refptr<base::RefCountedMemory> data;
   preview_ui->GetPrintPreviewDataForIndex(printing::FIRST_PAGE_INDEX, &data);
@@ -194,6 +196,7 @@ TEST_F(PrintPreviewUIUnitTest, ShouldCancelRequest) {
   PrintPreviewUI* preview_ui = static_cast<PrintPreviewUI*>(
       preview_dialog->GetWebUI()->GetController());
   ASSERT_TRUE(preview_ui);
+  preview_ui->SetPreviewUIId();
 
   // Test with invalid UI ID.
   const int32_t kInvalidId = -5;
@@ -201,7 +204,7 @@ TEST_F(PrintPreviewUIUnitTest, ShouldCancelRequest) {
 
   const int kFirstRequestId = 1000;
   const int kSecondRequestId = 1001;
-  const int32_t preview_id = preview_ui->GetIDForPrintPreviewUI();
+  const int32_t preview_id = preview_ui->GetIDForPrintPreviewUI().value();
 
   // Test with kFirstRequestId.
   preview_ui->OnPrintPreviewRequest(kFirstRequestId);