Don't clear pending NavigationParams until didStopLoading.

chrome/browser/printing/print_preview_dialog_controller_browsertest.cc

Index: chrome/browser/printing/print_preview_dialog_controller_browsertest.cc
diff --git a/chrome/browser/printing/print_preview_dialog_controller_browsertest.cc b/chrome/browser/printing/print_preview_dialog_controller_browsertest.cc
index d45a166f09f5c4f5f9231a77d04dbf8f48d50e60..09a67ed3bbd8a1a3b8f0fc9001599822c9b28bdb 100644
--- a/chrome/browser/printing/print_preview_dialog_controller_browsertest.cc
+++ b/chrome/browser/printing/print_preview_dialog_controller_browsertest.cc
@@ -57,6 +57,8 @@ class RequestPrintPreviewObserver : public WebContentsObserver {
     IPC_BEGIN_MESSAGE_MAP(RequestPrintPreviewObserver, message)
       IPC_MESSAGE_HANDLER(PrintHostMsg_RequestPrintPreview,
                           OnRequestPrintPreview)
+      IPC_MESSAGE_HANDLER(PrintHostMsg_SetupScriptedPrintPreview,
+                          OnSetupScriptedPrintPreview)
       IPC_MESSAGE_UNHANDLED(break)
     IPC_END_MESSAGE_MAP()
     return false;  // Report not handled so the real handler receives it.
@@ -67,6 +69,10 @@ class RequestPrintPreviewObserver : public WebContentsObserver {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, quit_closure_);
   }
 
+  void OnSetupScriptedPrintPreview() {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, quit_closure_);
+  }
+
   base::Closure quit_closure_;
 
   DISALLOW_COPY_AND_ASSIGN(RequestPrintPreviewObserver);
@@ -96,21 +102,60 @@ class PrintPreviewDialogClonedObserver : public WebContentsObserver {
   DISALLOW_COPY_AND_ASSIGN(PrintPreviewDialogClonedObserver);
 };
 
+class PrintPreviewDialogStartedObserver : public WebContentsObserver {
+ public:
+  explicit PrintPreviewDialogStartedObserver(WebContents* dialog)
+      : WebContentsObserver(dialog), waiting_(false), dialog_started_(false) {}
+  ~PrintPreviewDialogStartedObserver() override {}
+
+  bool dialog_started() const { return dialog_started_; }
+
+  void WaitForDialogStarted() {
+    waiting_ = true;
+    run_loop_.Run();
+  }
+
+ private:
+  // content::WebContentsObserver implementation.
+  void RenderViewReady() override {
+    dialog_started_ = true;
+    if (waiting_)
+      run_loop_.Quit();
+  }
+
+  bool waiting_;
+  bool dialog_started_;
+  base::RunLoop run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrintPreviewDialogStartedObserver);
+};
+
 class PrintPreviewDialogDestroyedObserver : public WebContentsObserver {
  public:
   explicit PrintPreviewDialogDestroyedObserver(WebContents* dialog)
       : WebContentsObserver(dialog),
-        dialog_destroyed_(false) {
-  }
+        waiting_(false),
+        dialog_destroyed_(false) {}
   ~PrintPreviewDialogDestroyedObserver() override {}
 
   bool dialog_destroyed() const { return dialog_destroyed_; }
 
+  void WaitForDialogDestroyed() {
+    waiting_ = true;
+    run_loop_.Run();
+  }
+
  private:
   // content::WebContentsObserver implementation.
-  void WebContentsDestroyed() override { dialog_destroyed_ = true; }
+  void WebContentsDestroyed() override {
+    dialog_destroyed_ = true;
+    if (waiting_)
+      run_loop_.Quit();
+  }
 
+  bool waiting_;
   bool dialog_destroyed_;
+  base::RunLoop run_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(PrintPreviewDialogDestroyedObserver);
 };
@@ -167,6 +212,14 @@ class PrintPreviewDialogControllerBrowserTest : public InProcessBrowserTest {
     run_loop.Run();
   }
 
+  void PrintPreviewViaScript(base::StringPiece script) {
+    base::RunLoop run_loop;
+    request_preview_dialog_observer()->set_quit_closure(run_loop.QuitClosure());
+    initiator()->GetMainFrame()->ExecuteJavaScriptForTests(
+        base::ASCIIToUTF16(script));
+    run_loop.Run();
+  }
+
   WebContents* GetPrintPreviewDialog() {
     printing::PrintPreviewDialogController* dialog_controller =
         printing::PrintPreviewDialogController::GetInstance();
@@ -239,6 +292,49 @@ IN_PROC_BROWSER_TEST_F(PrintPreviewDialogControllerBrowserTest,
   EXPECT_TRUE(new_preview_dialog);
 }
 
+// Ensure that back/forward navigations with ScopedPageLoadDeferrers (e.g.,
+// during printing) end up committing to the correct NavigationEntry.
+// See https://crbug.com/626838.
+IN_PROC_BROWSER_TEST_F(PrintPreviewDialogControllerBrowserTest,
+                       HistoryNavigateDuringPrint) {

[Avi (use Gerrit), 2016/07/16 02:29:13]

You're aware that this new test is red on the rel_ng trybots?

[Lei Zhang, 2016/07/16 03:34:12]

Doh. I can help take a look. Maybe take over the CL if needed.

[Charlie Reis, 2016/07/20 21:32:26]

Any ideas?  It turns out ScopedPageLoadDeferrer is being used in some higher
severity bugs, so we should get this landed soon.

Thanks!

+  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
+  ui_test_utils::NavigateToURL(browser(), GURL("data:text/html,page1"));
+  ui_test_utils::NavigateToURL(browser(), GURL("data:text/html,page2"));
+  EXPECT_EQ(2, tab->GetController().GetEntryCount());
+  EXPECT_EQ(1, tab->GetController().GetLastCommittedEntryIndex());
+
+  // Go back via script, but before that gets a chance to commit, show a print
+  // dialog with a nested message loop and ScopedPageLoadDeferrer.
+  PrintPreviewViaScript("history.back(); print();");
+
+  // Get the preview dialog for the initiator tab.
+  WebContents* preview_dialog = GetPrintPreviewDialog();
+
+  // Check a new print preview dialog got created.
+  ASSERT_TRUE(preview_dialog);
+  ASSERT_NE(initiator(), preview_dialog);
+
+  if (!preview_dialog->GetRenderProcessHost()->IsReady()) {
+    PrintPreviewDialogStartedObserver started_observer(preview_dialog);
+    started_observer.WaitForDialogStarted();
+    ASSERT_TRUE(started_observer.dialog_started());
+  }
+
+  // Dismiss the dialog.
+  PrintPreviewDialogDestroyedObserver destroyed_observer(preview_dialog);
+  ASSERT_TRUE(preview_dialog->GetRenderProcessHost()->Shutdown(0, true));
+  if (!destroyed_observer.dialog_destroyed()) {
+    destroyed_observer.WaitForDialogDestroyed();
+    ASSERT_TRUE(destroyed_observer.dialog_destroyed());
+  }
+  ASSERT_FALSE(GetPrintPreviewDialog());
+
+  // The back navigation should put us back on the first entry.
+  content::WaitForLoadStop(tab);
+  EXPECT_EQ(2, tab->GetController().GetEntryCount());
+  EXPECT_EQ(0, tab->GetController().GetLastCommittedEntryIndex());
+}
+
 // Test to verify that after reloading the initiator, it creates a new print
 // preview dialog.
 IN_PROC_BROWSER_TEST_F(PrintPreviewDialogControllerBrowserTest,