[DevUI DFM] Fix crash-on-install caused by stale navigation states.

Previously, if a failed DevUI DFM install attempt (leading to the error
page) is followed by a successful install, then the following problems
arise:
(A) chrome.send() is unavailable for the loaded DevUI page.
(B) Addition refreshes trigger CHECK() and crashes browser.
It seems that between showing the error page and showing the DevUI page
(after successful install), some stale state persisted so that the
active instance of NavigationRequest has bindings() holding a value
without BINDINGS_POLICY_WEB_UI, so that chrome.send() would not load,
leading to (A). Then (B) is triggered as an after effect.

This CL is a work-around for the bug for the DevUI DFM. On initial
DevUI DFM install, instead of resuming navigation to the intended
DevUI page, we visit a small shim page that self-refreshes, so that
stale states are cleared.

This is not a full fix, since state staleness problem (and crash on
refresh) still exists for error pages (in history or other tabs) other
than the one that successfully triggers DevUI DFM install.

Bug: 1046159
Change-Id: I775491ee165e7d2a809d93d334f93ae4bbf72cb6
Fixed: 1046159
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2028363
Commit-Queue: Samuel Huang <huangs@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736836}

chrome/browser/dev_ui/android/dev_ui_loader_error_page.cc

@@ -13,6 +13,10 @@
 
 namespace dev_ui {
 
+std::string BuildInitialInstallRedirectPageHtml() {
+  return "<script>window.location.reload()</script>";
+}
+
 std::string BuildErrorPageHtml() {
   ui::TemplateReplacements replacements;
   replacements["h1"] =

chrome/browser/dev_ui/android/dev_ui_loader_error_page.h

@@ -9,6 +9,8 @@
 
 namespace dev_ui {
 
+std::string BuildInitialInstallRedirectPageHtml();
+
 std::string BuildErrorPageHtml();
 
 }  // namespace dev_ui

chrome/browser/dev_ui/android/dev_ui_loader_throttle.cc

@@ -133,7 +133,14 @@ DevUiLoaderThrottle::WillStartRequest() {
 void DevUiLoaderThrottle::OnDevUiDfmInstallWithStatus(bool success) {
   if (success) {
     dev_ui::DevUiModuleProvider::GetInstance()->LoadModule();
-    Resume();
+
+    // Fix for http://crbug.com/1046159. Normally Resume() should be called.
+    // However, if refresh originates from the error page, then some stale state
+    // would persist in NavigationRequest::bindings() that prevents
+    // chrome.send() from working. So instead, we add a shim page to perform a
+    // reload by JavaScript.
+    std::string html = BuildInitialInstallRedirectPageHtml();
+    CancelDeferredNavigation({CANCEL, net::OK, html});
   } else {
     std::string html = BuildErrorPageHtml();
     CancelDeferredNavigation({CANCEL, net::ERR_CONNECTION_FAILED, html});

chrome/browser/dev_ui/android/dev_ui_loader_throttle_unittest.cc

@@ -32,6 +32,17 @@ const char* const kDevUiUrls[] = {
     "chrome://bluetooth-internals/path?query#frag",
 };
 
+// Heuristically checks whether |html| is the redirect page HTML, on successful
+// install.
+bool MatchesRedirectPageHtml(const std::string& html) {
+  return html.find("window.location.reload()") != std::string::npos;
+}
+
+// Heuristically checks whether |html| is the install error page HTML.
+bool MatchesErrorPageHtml(const std::string& html) {
+  return html.find("<div id=\"failure-message\">") != std::string::npos;
+}
+
 /******** MockDevUiModuleProvider ********/
 
 // Mock DevUiModuleProvider that overrides the main module install / load
@@ -107,6 +118,11 @@ class TestDevUiLoaderThrottle : public DevUiLoaderThrottle {
     cancel_result = std::move(result);
   }
 
+  std::string GetCancelResult() {
+    EXPECT_TRUE(cancel_result.error_page_content().has_value());
+    return cancel_result.error_page_content().value();
+  }
+
   bool called_resume = false;
   bool called_cancel = false;
   content::NavigationThrottle::ThrottleCheckResult cancel_result;
@@ -224,8 +240,8 @@ TEST_F(DevUiLoaderThrottleTest, InstallSuccess) {
 
     // Simulate actual install (success)
     mock_provider_.SimulateAsyncInstall(true);  // !
-    EXPECT_TRUE(throttle->called_resume);
-    EXPECT_FALSE(throttle->called_cancel);
+    EXPECT_FALSE(throttle->called_resume);
+    EXPECT_TRUE(throttle->called_cancel);
     EXPECT_TRUE(mock_provider_.GetIsInstalled());
     EXPECT_TRUE(mock_provider_.GetIsLoaded());
   }
@@ -251,10 +267,12 @@ TEST_F(DevUiLoaderThrottleTest, InstallQueued) {
 
   // Simulate actual install.
   mock_provider_.SimulateAsyncInstall(true);  // !
-  EXPECT_TRUE(throttle1->called_resume);
-  EXPECT_FALSE(throttle1->called_cancel);
-  EXPECT_TRUE(throttle2->called_resume);
-  EXPECT_FALSE(throttle2->called_cancel);
+  EXPECT_FALSE(throttle1->called_resume);
+  EXPECT_TRUE(throttle1->called_cancel);
+  EXPECT_TRUE(MatchesRedirectPageHtml(throttle1->GetCancelResult()));
+  EXPECT_FALSE(throttle2->called_resume);
+  EXPECT_TRUE(throttle2->called_cancel);
+  EXPECT_TRUE(MatchesRedirectPageHtml(throttle2->GetCancelResult()));
   EXPECT_TRUE(mock_provider_.GetIsInstalled());
   EXPECT_TRUE(mock_provider_.GetIsLoaded());
 }
@@ -275,8 +293,9 @@ TEST_F(DevUiLoaderThrottleTest, InstallRedundant) {
 
   // Simulate actual install.
   mock_provider_.SimulateAsyncInstall(true);  // !
-  EXPECT_TRUE(throttle1->called_resume);
-  EXPECT_FALSE(throttle1->called_cancel);
+  EXPECT_FALSE(throttle1->called_resume);
+  EXPECT_TRUE(throttle1->called_cancel);
+  EXPECT_TRUE(MatchesRedirectPageHtml(throttle1->GetCancelResult()));
   EXPECT_TRUE(mock_provider_.GetIsInstalled());
   EXPECT_TRUE(mock_provider_.GetIsLoaded());
 
@@ -312,16 +331,13 @@ TEST_F(DevUiLoaderThrottleTest, InstallFailure) {
     mock_provider_.SimulateAsyncInstall(false);  // !
     EXPECT_FALSE(throttle->called_resume);
     EXPECT_TRUE(throttle->called_cancel);
-    EXPECT_FALSE(mock_provider_.GetIsInstalled());
-    EXPECT_FALSE(mock_provider_.GetIsLoaded());
     EXPECT_EQ(content::NavigationThrottle::CANCEL,
               throttle->cancel_result.action());
     EXPECT_EQ(net::ERR_CONNECTION_FAILED,
               throttle->cancel_result.net_error_code());
-    EXPECT_TRUE(throttle->cancel_result.error_page_content().has_value());
-    std::string page_content =
-        throttle->cancel_result.error_page_content().value();
-    EXPECT_NE(std::string::npos, page_content.find("<!DOCTYPE html"));
+    EXPECT_TRUE(MatchesErrorPageHtml(throttle->GetCancelResult()));
+    EXPECT_FALSE(mock_provider_.GetIsInstalled());
+    EXPECT_FALSE(mock_provider_.GetIsLoaded());
   }
 }