Reland "Fix browser test EulaTest.LoadOffline"

This is a reland of 5fd0e6ed

Original change's description:
> Fix browser test EulaTest.LoadOffline
> 
> This commit improves the reliability of the EULA browser test
> EulaTest.LoadOffline. This test was disabled due to its flakiness
> in the past and is now enabled again.
> 
> Tested locally. Ran 100 tests under stress without any errors.
> 
> Bug: 1015948
> Change-Id: I59bc8612da9f5be526a2aa28d13ee70400dbb01e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1892775
> Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
> Commit-Queue: Renato Silva <rrsilva@google.com>
> Cr-Commit-Position: refs/heads/master@{#712516}

Bug: 1015948
Change-Id: I47bd386c589344f011b03bd6e24f50f08c3b42a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899862Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Commit-Queue: Renato Silva <rrsilva@google.com>
Cr-Commit-Position: refs/heads/master@{#716119}

chrome/browser/chromeos/login/eula_browsertest.cc

@@ -72,8 +72,10 @@ class WebContentsLoadFinishedWaiter : public content::WebContentsObserver {
   ~WebContentsLoadFinishedWaiter() override = default;
 
   void Wait() {
-    if (!web_contents()->IsLoading())
+    if (!web_contents()->IsLoading() &&
+        web_contents()->GetLastCommittedURL() != GURL::EmptyGURL()) {
       return;
+    }
 
     run_loop_ = std::make_unique<base::RunLoop>();
     run_loop_->Run();
@@ -107,26 +109,23 @@ class EulaTest : public OobeBaseTest {
   EulaTest() = default;
   ~EulaTest() override = default;
 
+  void SetUpOnMainThread() override {
+    // Retrieve the URL from the embedded test server and override EULA URL.
+    fake_eula_url_ =
+        embedded_test_server()->base_url().Resolve(kFakeOnlineEulaPath).spec();
+    EulaScreenHandler::set_eula_url_for_testing(fake_eula_url_.c_str());
+
+    OobeBaseTest::SetUpOnMainThread();
+  }
+
   // OobeBaseTest:
   void RegisterAdditionalRequestHandlers() override {
     embedded_test_server()->RegisterRequestHandler(
         base::Bind(&EulaTest::HandleRequest, base::Unretained(this)));
   }
 
-  void OverrideOnlineEulaUrl() {
-    // Override with the embedded test server's base url. Otherwise, the load
-    // would not hit the embedded test server.
-    const GURL fake_eula_url =
-        embedded_test_server()->base_url().Resolve(kFakeOnlineEulaPath);
-    test::OobeJS().Evaluate(
-        base::StringPrintf("loadTimeData.overrideValues({eulaOnlineUrl: '%s'});"
-                           "Oobe.updateLocalizedContent();",
-                           fake_eula_url.spec().c_str()));
-  }
-
   void ShowEulaScreen() {
     LoginDisplayHost::default_host()->StartWizard(EulaView::kScreenId);
-    OverrideOnlineEulaUrl();
     OobeScreenWaiter(EulaView::kScreenId).Wait();
   }
 
@@ -156,6 +155,18 @@ class EulaTest : public OobeBaseTest {
     return *frame_set.begin();
   }
 
+  // Wait for the fallback offline page (loaded as data url) to be loaded.
+  void WaitForLocalWebviewLoad() {
+    content::WebContents* eula_contents = FindEulaContents();
+    ASSERT_TRUE(eula_contents);
+
+    while (!eula_contents->GetLastCommittedURL().SchemeIs("data")) {
+      // Pump messages to avoid busy loop so that renderer could do some work.
+      base::RunLoop().RunUntilIdle();
+      WebContentsLoadFinishedWaiter(eula_contents).Wait();
+    }
+  }
+
   // Returns an Oobe JSChecker that sends 'click' events instead of 'tap'
   // events when interacting with UI elements.
   test::JSChecker NonPolymerOobeJS() {
@@ -228,7 +239,10 @@ class EulaTest : public OobeBaseTest {
     return std::move(http_response);
   }
 
-  bool allow_online_eula_ = false;
+  bool allow_online_eula_ = true;
+
+  // URL used for testing. Retrieved from the embedded server.
+  std::string fake_eula_url_;
 
   DISALLOW_COPY_AND_ASSIGN(EulaTest);
 };
@@ -247,19 +261,11 @@ IN_PROC_BROWSER_TEST_F(EulaTest, DISABLED_LoadOnline) {
 
 // Tests that offline version is shown when the online version is not
 // accessible.
-// Disable due to flaky crbug.com/1015948
-IN_PROC_BROWSER_TEST_F(EulaTest, DISABLED_LoadOffline) {
+IN_PROC_BROWSER_TEST_F(EulaTest, LoadOffline) {
   set_allow_online_eula(false);
   ShowEulaScreen();
 
-  content::WebContents* eula_contents = FindEulaContents();
-  ASSERT_TRUE(eula_contents);
-  // Wait for the fallback offline page (loaded as data url) to be loaded.
-  while (!eula_contents->GetLastCommittedURL().SchemeIs("data")) {
-    // Pump messages to avoid busy loop so that renderer could do some work.
-    base::RunLoop().RunUntilIdle();
-    WebContentsLoadFinishedWaiter(eula_contents).Wait();
-  }
+  WaitForLocalWebviewLoad();
 
   EXPECT_TRUE(test::GetWebViewContents({"oobe-eula-md", "crosEulaFrame"})
                   .find(kOfflineEULAWarning) != std::string::npos);

chrome/browser/resources/chromeos/login/oobe_screen_eula.js

@@ -17,88 +17,154 @@ login.createScreen('EulaScreen', 'eula', function() {
         '}'
   };
 
-  // A class to wrap online contents loading for a webview. A PendingLoad is
-  // constructed with a target webview, an url to load, a load timeout and an
-  // error callback. It attaches to a "pendingLoad" property of |webview| after
-  // starting the load by setting |webview|.src. If the load times out or fails,
-  // the error callback is invoked. If there exists a "pendingLoad" before the
-  // load, the existing one is stopped (note the error callback is not invoked
-  // in this case).
-  class PendingLoad {
-    constructor(webview, url, timeout, opt_errorCallback) {
+  // EulaLoader assists on the process of loading an URL into a webview.
+  // It listens for events from the webRequest API for the given URL and loads
+  // an offline version of the EULA in case of failure.
+  // Calling setURL() multiple times with the same URL while requests are being
+  // made won't affect current requests. Instead, it will mark the flag
+  // 'reloadRequested' for the given URL. The reload will be performed only if
+  // the current requests fail. This prevents webview-loadAbort events from
+  // being fired and unnecessary reloads.
+  class EulaLoader {
+    constructor(webview, timeout, load_offline_callback) {
       assert(webview.tagName === 'WEBVIEW');
-      assert(/^https?:\/\//.test(url));
+
+      // Do not create multiple loaders.
+      if (EulaLoader.instance_) {
+        return EulaLoader.instance_;
+      }
 
       this.webview_ = webview;
-      this.url_ = url;
       this.timeout_ = timeout;
-      this.errorCallback_ = opt_errorCallback;
-
+      this.isPerformingRequests_ = false;
+      this.reloadRequested_ = false;
+      this.loadOfflineCallback_ = load_offline_callback;
       this.loadTimer_ = 0;
-      this.boundOnLoadCompleted_ = this.onLoadCompleted_.bind(this);
-      this.boundOnLoadError_ = this.onLoadError_.bind(this);
-    }
 
-    start() {
-      if (this.webview_[PendingLoad.ATTACHED_PROPERTY_NAME])
-        this.webview_[PendingLoad.ATTACHED_PROPERTY_NAME].stop();
-      this.webview_[PendingLoad.ATTACHED_PROPERTY_NAME] = this;
+      // Add the CLEAR_ANCHORS_CONTENT_SCRIPT that will clear <a><\a> (anchors)
+      // in order to prevent any navigation in the webview itself.
+      webview.addContentScripts([{
+        name: 'clearAnchors',
+        matches: ['<all_urls>'],
+        js: CLEAR_ANCHORS_CONTENT_SCRIPT,
+      }]);
+      webview.addEventListener('contentload', () => {
+        webview.executeScript(CLEAR_ANCHORS_CONTENT_SCRIPT);
+      });
 
-      this.webview_.addEventListener('loadabort', this.boundOnLoadError_);
+      // Monitor webRequests API events
       this.webview_.request.onCompleted.addListener(
-          this.boundOnLoadCompleted_,
+          this.onCompleted_.bind(this),
           {urls: ['<all_urls>'], types: ['main_frame']});
       this.webview_.request.onErrorOccurred.addListener(
-          this.boundOnLoadError_,
+          this.onErrorOccurred_.bind(this),
           {urls: ['<all_urls>'], types: ['main_frame']});
-      this.loadTimer_ =
-          window.setTimeout(this.boundOnLoadError_, this.timeout_);
 
-      this.webview_.src = this.url_;
+      // The only instance of the EulaLoader.
+      EulaLoader.instance_ = this;
     }
 
-    stop() {
-      assert(this.webview_[PendingLoad.ATTACHED_PROPERTY_NAME] === this);
-      delete this.webview_[PendingLoad.ATTACHED_PROPERTY_NAME];
-
-      this.webview_.removeEventListener('loadabort', this.boundOnLoadError_);
-      this.webview_.request.onCompleted.removeListener(
-          this.boundOnLoadCompleted_);
-      this.webview_.request.onErrorOccurred.removeListener(
-          this.boundOnLoadError_);
+    // Clears the internal state of the EULA loader. Stops the timeout timer
+    // and prevents events from being handled.
+    clearInternalState() {
       window.clearTimeout(this.loadTimer_);
+      this.isPerformingRequests_ = false;
+      this.reloadRequested_ = false;
+    }
+
+    // Sets an URL to be loaded in the webview. If the URL is different from the
+    // previous one, it will be immediately loaded. If the URL is the same as
+    // the previous one, it will be reloaded. If requests are under way, the
+    // reload will be performed once the current requests are finished.
+    setUrl(url) {
+      assert(/^https?:\/\//.test(url));
+
+      if (url != this.url_) {
+        // Clear the internal state and start with a new URL.
+        this.clearInternalState();
+        this.url_ = url;
+        this.loadWithFallbackTimer();
+      } else {
+        // Same URL was requested again. Reload later if a request is under way.
+        if (this.isPerformingRequests_)
+          this.reloadRequested_ = true;
+        else
+          this.loadWithFallbackTimer();
+      }
     }
 
-    onLoadCompleted_(details) {
-      if (details.url != this.url_)
+    // This method only gets invoked if the webview webRequest API does not
+    // fire either 'onErrorOccurred' or 'onCompleted' before the timer runs out.
+    // See: https://developer.chrome.com/extensions/webRequest
+    onTimeoutError_() {
+      // Return if we are no longer monitoring requests. Sanity check.
+      if (!this.isPerformingRequests_)
         return;
 
-      // Http errors such as 4xx, 5xx hit here instead of 'onErrorOccurred'.
-      if (details.statusCode != 200) {
-        this.onLoadError_();
+      this.reloadIfRequestedOrLoadOffline();
+    }
+
+    // Loads the offline version of the EULA.
+    tryLoadOffline() {
+      this.clearInternalState();
+      if (this.loadOfflineCallback_)
+        this.loadOfflineCallback_();
+    }
+
+    // Only process events for the current URL and when performing requests.
+    shouldProcessEvent(details) {
+      return this.isPerformingRequests_ && (details.url === this.url_);
+    }
+
+    // webRequest API Event Handler for 'onErrorOccurred'
+    onErrorOccurred_(details) {
+      if (!this.shouldProcessEvent(details))
         return;
-      }
 
-      this.stop();
+      this.reloadIfRequestedOrLoadOffline();
     }
 
-    onLoadError_(opt_details) {
-      // A 'loadabort' or 'onErrorOccurred' event from a previous load could
-      // be invoked even though a new navigation has started. Filter out those
-      // by checking the url.
-      if (opt_details && opt_details.url != this.url_)
+    // webRequest API Event Handler for 'onCompleted'
+    onCompleted_(details) {
+      if (!this.shouldProcessEvent(details))
         return;
 
-      this.stop();
-      if (this.errorCallback_)
-        this.errorCallback_();
+      // Http errors such as 4xx, 5xx hit here instead of 'onErrorOccurred'.
+      if (details.statusCode != 200) {
+        // Not a successful request. Perform a reload if requested.
+        this.reloadIfRequestedOrLoadOffline();
+      } else {
+        // Success!
+        this.clearInternalState();
+      }
+    }
+
+    // Loads the URL into the webview and starts a timer.
+    loadWithFallbackTimer() {
+      // A request is being made
+      this.isPerformingRequests_ = true;
+
+      // Clear previous timer and perform a load.
+      window.clearTimeout(this.loadTimer_);
+      this.loadTimer_ =
+          window.setTimeout(this.onTimeoutError_.bind(this), this.timeout_);
+
+      if (this.webview_.src === this.url_)
+        this.webview_.reload();
+      else
+        this.webview_.src = this.url_;
+    }
+
+    // Tries to perform a reload if it was requested, otherwise load offline.
+    reloadIfRequestedOrLoadOffline() {
+      if (this.reloadRequested_) {
+        this.reloadRequested_ = false;
+        this.loadWithFallbackTimer();
+      } else {
+        this.tryLoadOffline();
+      }
     }
   }
-  /**
-   * A property name used to attach to a Webview.
-   * type{string}
-   */
-  PendingLoad.ATTACHED_PROPERTY_NAME = 'pendingLoad';
 
   return {
     /** @override */
@@ -185,15 +251,6 @@ login.createScreen('EulaScreen', 'eula', function() {
             webview, TERMS_URL, WebViewHelper.ContentType.HTML);
       };
 
-      webview.addContentScripts([{
-        name: 'clearAnchors',
-        matches: ['<all_urls>'],
-        js: CLEAR_ANCHORS_CONTENT_SCRIPT,
-      }]);
-      webview.addEventListener('contentload', () => {
-        webview.executeScript(CLEAR_ANCHORS_CONTENT_SCRIPT);
-      });
-
       var onlineEulaUrl = loadTimeData.getString('eulaOnlineUrl');
       if (!onlineEulaUrl) {
         loadBundledEula();
@@ -201,10 +258,10 @@ login.createScreen('EulaScreen', 'eula', function() {
       }
 
       // Load online Eula with a timeout to fallback to the offline version.
-      var pendingLoad = new PendingLoad(
-          webview, onlineEulaUrl, ONLINE_EULA_LOAD_TIMEOUT_IN_MS,
-          loadBundledEula);
-      pendingLoad.start();
+      // This won't construct multiple EulaLoaders. Single instance.
+      var eulaLoader = new EulaLoader(
+          webview, ONLINE_EULA_LOAD_TIMEOUT_IN_MS, loadBundledEula);
+      eulaLoader.setUrl(onlineEulaUrl);
     },
 
     /**

chrome/browser/ui/webui/chromeos/login/eula_screen_handler.cc

@@ -29,6 +29,8 @@ namespace chromeos {
 
 constexpr StaticOobeScreenId EulaView::kScreenId;
 
+const char* EulaScreenHandler::eula_url_for_testing_ = nullptr;
+
 EulaScreenHandler::EulaScreenHandler(JSCallsContainer* js_calls_container,
                                      CoreOobeView* core_oobe_view)
     : BaseScreenHandler(kScreenId, js_calls_container),
@@ -64,6 +66,15 @@ void EulaScreenHandler::Unbind() {
   BaseScreenHandler::SetBaseScreen(nullptr);
 }
 
+std::string EulaScreenHandler::GetEulaOnlineUrl() {
+  if (EulaScreenHandler::eula_url_for_testing_) {
+    return std::string(EulaScreenHandler::eula_url_for_testing_);
+  }
+
+  return base::StringPrintf(chrome::kOnlineEulaURLPath,
+                            g_browser_process->GetApplicationLocale().c_str());
+}
+
 void EulaScreenHandler::DeclareLocalizedValues(
     ::login::LocalizedValuesBuilder* builder) {
   builder->Add("eulaScreenAccessibleTitle", IDS_EULA_SCREEN_ACCESSIBLE_TITLE);
@@ -94,10 +105,8 @@ void EulaScreenHandler::DeclareLocalizedValues(
                 IDS_SHORT_PRODUCT_OS_NAME);
 #endif
 
-  builder->Add(
-      "eulaOnlineUrl",
-      base::StringPrintf(chrome::kOnlineEulaURLPath,
-                         g_browser_process->GetApplicationLocale().c_str()));
+  // Online URL to use. May be overridden by tests.
+  builder->Add("eulaOnlineUrl", GetEulaOnlineUrl());
 
   /* MD-OOBE */
   builder->Add("oobeEulaSectionTitle", IDS_OOBE_EULA_SECTION_TITLE);

chrome/browser/ui/webui/chromeos/login/eula_screen_handler.h

@@ -62,12 +62,20 @@ class EulaScreenHandler : public EulaView, public BaseScreenHandler {
   void GetAdditionalParameters(base::DictionaryValue* dict) override;
   void Initialize() override;
 
+  static void set_eula_url_for_testing(const char* eula_test_url) {
+    eula_url_for_testing_ = eula_test_url;
+  }
+
  private:
   // JS messages handlers.
   void HandleOnLearnMore();
   void HandleOnInstallationSettingsPopupOpened();
   void HandleUsageStatsEnabled(bool enabled);
 
+  // Determines the online URL to use.
+  std::string GetEulaOnlineUrl();
+  static const char* eula_url_for_testing_;
+
   void UpdateLocalizedValues(::login::SecureModuleUsed secure_module_used);
 
   EulaScreen* screen_ = nullptr;