[chromedriver] Apply page load timeout to slow cross-process navigations

chrome/test/chromedriver/chrome/navigation_tracker.cc

Index: chrome/test/chromedriver/chrome/navigation_tracker.cc
diff --git a/chrome/test/chromedriver/chrome/navigation_tracker.cc b/chrome/test/chromedriver/chrome/navigation_tracker.cc
index a0f26a86a525ec483138e69b9d79b75f31f2e61a..74c392ed62438b501c8b63a6332935baf9efd748 100644
--- a/chrome/test/chromedriver/chrome/navigation_tracker.cc
+++ b/chrome/test/chromedriver/chrome/navigation_tracker.cc
@@ -43,7 +43,13 @@ NavigationTracker::NavigationTracker(DevToolsClient* client,
 
 NavigationTracker::~NavigationTracker() {}
 
+Status MakeNavigationCheckFailedStatus(Status command_status) {
+  return Status(command_status.code() == kTimeout ? kTimeout : kUnknownError,
+                "cannot determine loading status", command_status);
+}

[samuong, 2016/04/13 03:59:30]

I'm not sure we want to return a kTimeout status here. If we do so, test clients
will throw a timeout exception, which could be interpreted as a script timeout
or page load timeout. But these timeouts that you're catching are related to
DevTools command timeouts, which would indicate that something's gone wrong in
the browser/renderer. Shouldn't we continue to return kUnknownError here?

[Alexander Semashko, 2016/04/13 15:51:35]

It may as well indicate a real timeout when browser is functioning properly
(remember that during cross-process navigations all devtools responses are
throttled until the navigation commits/aborts), and the primary goal of this CL
is to handle these timeouts. We must return kTimeout to
WebViewImpl::WaitForPendingNavigations so that it attempts to stop the
navigation. Otherwise, the navigation remains pending, which means we're unable
to command the browser.

[samuong, 2016/04/15 04:23:55]

OK, sounds good.

+
 Status NavigationTracker::IsPendingNavigation(const std::string& frame_id,
+                                              const Timeout* timeout,
                                               bool* is_pending) {
   if (!IsExpectingFrameLoadingEvents()) {
     // Some DevTools commands (e.g. Input.dispatchMouseEvent) are handled in the
@@ -54,8 +60,8 @@ Status NavigationTracker::IsPendingNavigation(const std::string& frame_id,
     base::DictionaryValue params;
     params.SetString("expression", "1");
     scoped_ptr<base::DictionaryValue> result;
-    Status status = client_->SendCommandAndGetResult(
-        "Runtime.evaluate", params, &result);
+    Status status = client_->SendCommandAndGetResultWithTimeout(
+        "Runtime.evaluate", params, timeout, &result);
     int value = 0;
     if (status.code() == kDisconnected) {
       // If we receive a kDisconnected status code from Runtime.evaluate, don't
@@ -66,7 +72,7 @@ Status NavigationTracker::IsPendingNavigation(const std::string& frame_id,
     } else if (status.IsError() ||
                !result->GetInteger("result.value", &value) ||
                value != 1) {
-      return Status(kUnknownError, "cannot determine loading status", status);
+      return MakeNavigationCheckFailedStatus(status);
     }
   }
 
@@ -76,11 +82,11 @@ Status NavigationTracker::IsPendingNavigation(const std::string& frame_id,
     // for the new window. In such case, the baseURL will be empty.
     base::DictionaryValue empty_params;
     scoped_ptr<base::DictionaryValue> result;
-    Status status = client_->SendCommandAndGetResult(
-        "DOM.getDocument", empty_params, &result);
+    Status status = client_->SendCommandAndGetResultWithTimeout(
+        "DOM.getDocument", empty_params, timeout, &result);
     std::string base_url;
     if (status.IsError() || !result->GetString("root.baseURL", &base_url))
-      return Status(kUnknownError, "cannot determine loading status", status);
+      return MakeNavigationCheckFailedStatus(status);
     if (base_url.empty()) {
       *is_pending = true;
       loading_state_ = kLoading;
@@ -107,10 +113,10 @@ Status NavigationTracker::IsPendingNavigation(const std::string& frame_id,
        "}";
     base::DictionaryValue params;
     params.SetString("expression", kStartLoadingIfMainFrameNotLoading);
-    status = client_->SendCommandAndGetResult(
-        "Runtime.evaluate", params, &result);
+    status = client_->SendCommandAndGetResultWithTimeout(
+        "Runtime.evaluate", params, timeout, &result);
     if (status.IsError())
-      return Status(kUnknownError, "cannot determine loading status", status);
+      return MakeNavigationCheckFailedStatus(status);
 
     // Between the time the JavaScript is evaluated and
     // SendCommandAndGetResult returns, OnEvent may have received info about