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

chrome/test/chromedriver/window_commands.cc

Index: chrome/test/chromedriver/window_commands.cc
diff --git a/chrome/test/chromedriver/window_commands.cc b/chrome/test/chromedriver/window_commands.cc
index 786f185b01ace9da3dc4ac7cd33df406425325fd..fd7513652af1c80c2b8ebb2929cb261cdfaee04f 100644
--- a/chrome/test/chromedriver/window_commands.cc
+++ b/chrome/test/chromedriver/window_commands.cc
@@ -203,6 +203,7 @@ Status ExecuteWindowCommand(
     Session* session,
     const base::DictionaryValue& params,
     scoped_ptr<base::Value>* value) {
+  Timeout timeout;

[samuong, 2016/02/20 00:11:44]

I might have misread how this works, but does timeout.deadline_ == 0 here?

   WebView* web_view = NULL;
   Status status = session->GetTargetWindow(&web_view);
   if (status.IsError())
@@ -233,19 +234,24 @@ Status ExecuteWindowCommand(
     }
 
     nav_status = web_view->WaitForPendingNavigations(
-        session->GetCurrentFrameId(), session->page_load_timeout, true);
+        session->GetCurrentFrameId(),
+        Timeout(session->page_load_timeout, &timeout),

[samuong, 2016/02/20 00:11:44]

...and if so, does that mean that the timeout duration here will also be 0,
since min(session->page_load_timeout, timeout.duration_) == timeout.duration_ ==
0?

[Alexander Semashko, 2016/02/20 00:38:40]

No, the outer timout's duration has effect only if it is set. The first call to
WaitForPendingNavigations will always be allowed to wait for
session->page_load_timeout.

[samuong, 2016/02/22 23:55:35]

Ah right, I misread the way TimeTicks::is_null() works. So the first call to
WaitForPendingNavigations will use session->page_load_timeout, since
timeout->deadline_ is null. This shouldn't cause any problem.

+        true);
     if (nav_status.IsError())
       return nav_status;
 
-    status = command.Run(session, web_view, params, value);
-    if (status.code() == kNoSuchExecutionContext) {
+    status = command.Run(session, web_view, params, value, &timeout);

[samuong, 2016/02/22 23:55:35]

...however the call to command.Run here may or may not set the timeout duration,
depending on which command we're dealing with. In the case of ExecuteGet, it
will set the duration, and if ExecuteGet fails, it'll be retried. So on the
second attempt, we call ExecuteGet again, and this will in turn call
timeout.SetDuration for a second time?

I guess this won't cause an issue since it gets set to the same duration (unless
we're in a debug build and the DCHECKs are enabled...)

[Alexander Semashko, 2016/02/24 12:51:20]

Good catch! I've changed the code so that it's ok to call SetDuration many times
with the same duration.

+    if (status.code() == kNoSuchExecutionContext ||
+        status.code() == kTimeout) {
+      // If the command timed out, let WaitForPendingNavigations cancel
+      // the navigation if there is one.
       continue;
     } else if (status.IsError()) {
       // If the command failed while a new page or frame started loading, retry
       // the command after the pending navigation has completed.
       bool is_pending = false;
       nav_status = web_view->IsPendingNavigation(session->GetCurrentFrameId(),
-                                                 &is_pending);
+                                                 &timeout, &is_pending);
       if (nav_status.IsError())
         return nav_status;
       else if (is_pending)
@@ -255,7 +261,9 @@ Status ExecuteWindowCommand(
   }
 
   nav_status = web_view->WaitForPendingNavigations(
-      session->GetCurrentFrameId(), session->page_load_timeout, true);
+      session->GetCurrentFrameId(),
+      Timeout(session->page_load_timeout, &timeout),
+      true);
 
   if (status.IsOk() && nav_status.IsError() &&
       nav_status.code() != kUnexpectedAlertOpen)
@@ -269,11 +277,13 @@ Status ExecuteGet(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
+  timeout->SetDuration(session->page_load_timeout);

[samuong, 2016/02/19 20:18:58]

Is there a need to have a series of subtask timeouts if the "outer" timeout has
the same duration?

[Alexander Semashko, 2016/02/19 23:38:59]

Why not? Anyway, I could'n conceive a better (yet simple) way for
ExecuteWindowCommand to set a timeout specific for a particular command, if
there is one. And it's easy to imagine having a long timeout for e.g. executing
scripts and a small timeout for page loads (for example, testing a web service
in local network with heavy server-side processing), in such a case a hung
navigation is a reason to abort as soon as possible.

[samuong, 2016/02/20 00:11:44]

If the outer and inner timeouts have the same duration, but the outer one has an
earlier start time, doesn't that make the inner sub-task timeout redundant?

[Alexander Semashko, 2016/02/20 00:38:40]

Yeah, it is, only if they have the same duration. But it's different if the
outer one has a long duration. Right now the outer timeout is set only in
ExecuteGet and happen to be the same that ExecuteWindowCommand uses for
preparation. After a while this may change.

[samuong, 2016/02/22 23:55:35]

OK, it's fine if this is the same for now but you want to make it easy to make
them different later.

   std::string url;
   if (!params.GetString("url", &url))
     return Status(kUnknownError, "'url' must be a string");
-  Status status = web_view->Load(url);
+  Status status = web_view->Load(url, timeout);
   if (status.IsError())
     return status;
   session->SwitchToTopFrame();
@@ -284,7 +294,8 @@ Status ExecuteExecuteScript(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::string script;
   if (!params.GetString("script", &script))
     return Status(kUnknownError, "'script' must be a string");
@@ -308,7 +319,8 @@ Status ExecuteExecuteAsyncScript(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::string script;
   if (!params.GetString("script", &script))
     return Status(kUnknownError, "'script' must be a string");
@@ -325,7 +337,8 @@ Status ExecuteSwitchToFrame(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   const base::Value* id;
   if (!params.Get("id", &id))
     return Status(kUnknownError, "missing 'id'");
@@ -396,7 +409,8 @@ Status ExecuteSwitchToParentFrame(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   session->SwitchToParentFrame();
   return Status(kOk);
 }
@@ -405,7 +419,8 @@ Status ExecuteGetTitle(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   const char kGetTitleScript[] = "function() {  return document.title;}";
   base::ListValue args;
   return web_view->CallFunction(std::string(), kGetTitleScript, args, value);
@@ -415,7 +430,8 @@ Status ExecuteGetPageSource(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   const char kGetPageSource[] =
       "function() {"
       "  return new XMLSerializer().serializeToString(document);"
@@ -430,7 +446,8 @@ Status ExecuteFindElement(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return FindElement(interval_ms, true, NULL, session, web_view, params, value);
 }
 
@@ -439,7 +456,8 @@ Status ExecuteFindElements(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return FindElement(
       interval_ms, false, NULL, session, web_view, params, value);
 }
@@ -448,7 +466,8 @@ Status ExecuteGetCurrentUrl(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::string url;
   Status status = GetUrl(web_view, std::string(), &url);
   if (status.IsError())
@@ -483,7 +502,8 @@ Status ExecuteGoBack(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   Status status = web_view->TraverseHistory(-1);
   if (status.IsError())
     return status;
@@ -495,7 +515,8 @@ Status ExecuteGoForward(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   Status status = web_view->TraverseHistory(1);
   if (status.IsError())
     return status;
@@ -507,7 +528,8 @@ Status ExecuteRefresh(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   Status status = web_view->Reload();
   if (status.IsError())
     return status;
@@ -519,7 +541,8 @@ Status ExecuteMouseMoveTo(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::string element_id;
   bool has_element = params.GetString("element", &element_id);
   int x_offset = 0;
@@ -557,7 +580,8 @@ Status ExecuteMouseClick(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   MouseButton button;
   Status status = GetMouseButton(params, &button);
   if (status.IsError())
@@ -578,7 +602,8 @@ Status ExecuteMouseButtonDown(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   MouseButton button;
   Status status = GetMouseButton(params, &button);
   if (status.IsError())
@@ -595,7 +620,8 @@ Status ExecuteMouseButtonUp(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   MouseButton button;
   Status status = GetMouseButton(params, &button);
   if (status.IsError())
@@ -612,7 +638,8 @@ Status ExecuteMouseDoubleClick(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   MouseButton button;
   Status status = GetMouseButton(params, &button);
   if (status.IsError())
@@ -633,7 +660,8 @@ Status ExecuteTouchDown(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return ExecuteTouchEvent(session, web_view, kTouchStart, params);
 }
 
@@ -641,7 +669,8 @@ Status ExecuteTouchUp(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return ExecuteTouchEvent(session, web_view, kTouchEnd, params);
 }
 
@@ -649,7 +678,8 @@ Status ExecuteTouchMove(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return ExecuteTouchEvent(session, web_view, kTouchMove, params);
 }
 
@@ -657,7 +687,8 @@ Status ExecuteTouchScroll(
     Session *session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   if (session->chrome->GetBrowserInfo()->build_no < 2286) {
     // TODO(samuong): remove this once we stop supporting M41.
     return Status(kUnknownCommand, "Touch scroll action requires Chrome 42+");
@@ -684,7 +715,8 @@ Status ExecuteTouchPinch(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   if (session->chrome->GetBrowserInfo()->build_no < 2286) {
     // TODO(samuong): remove this once we stop supporting M41.
     return Status(kUnknownCommand, "Pinch action requires Chrome 42+");
@@ -704,7 +736,8 @@ Status ExecuteGetActiveElement(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return GetActiveElement(session, web_view, value);
 }
 
@@ -712,7 +745,8 @@ Status ExecuteSendKeysToActiveElement(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   const base::ListValue* key_list;
   if (!params.GetList("value", &key_list))
     return Status(kUnknownError, "'value' must be a list");
@@ -724,7 +758,8 @@ Status ExecuteGetAppCacheStatus(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return web_view->EvaluateScript(
       session->GetCurrentFrameId(),
       "applicationCache.status",
@@ -735,7 +770,8 @@ Status ExecuteIsBrowserOnline(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return web_view->EvaluateScript(
       session->GetCurrentFrameId(),
       "navigator.onLine",
@@ -747,7 +783,8 @@ Status ExecuteGetStorageItem(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::string key;
   if (!params.GetString("key", &key))
     return Status(kUnknownError, "'key' must be a string");
@@ -765,7 +802,8 @@ Status ExecuteGetStorageKeys(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   const char script[] =
       "var keys = [];"
       "for (var key in %s) {"
@@ -783,7 +821,8 @@ Status ExecuteSetStorageItem(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::string key;
   if (!params.GetString("key", &key))
     return Status(kUnknownError, "'key' must be a string");
@@ -805,7 +844,8 @@ Status ExecuteRemoveStorageItem(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::string key;
   if (!params.GetString("key", &key))
     return Status(kUnknownError, "'key' must be a string");
@@ -823,7 +863,8 @@ Status ExecuteClearStorage(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return web_view->EvaluateScript(
       session->GetCurrentFrameId(),
       base::StringPrintf("%s.clear()", storage),
@@ -835,7 +876,8 @@ Status ExecuteGetStorageSize(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return web_view->EvaluateScript(
       session->GetCurrentFrameId(),
       base::StringPrintf("%s.length", storage),
@@ -846,7 +888,8 @@ Status ExecuteScreenshot(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   Status status = session->chrome->ActivateWebView(web_view->GetId());
   if (status.IsError())
     return status;
@@ -878,7 +921,8 @@ Status ExecuteGetCookies(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::list<Cookie> cookies;
   Status status = GetVisibleCookies(web_view, &cookies);
   if (status.IsError())
@@ -896,7 +940,8 @@ Status ExecuteAddCookie(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   const base::DictionaryValue* cookie;
   if (!params.GetDictionary("cookie", &cookie))
     return Status(kUnknownError, "missing 'cookie'");
@@ -911,7 +956,8 @@ Status ExecuteDeleteCookie(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::string name;
   if (!params.GetString("name", &name))
     return Status(kUnknownError, "missing 'name'");
@@ -928,7 +974,8 @@ Status ExecuteDeleteAllCookies(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::list<Cookie> cookies;
   Status status = GetVisibleCookies(web_view, &cookies);
   if (status.IsError())
@@ -956,7 +1003,8 @@ Status ExecuteSetLocation(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   const base::DictionaryValue* location = NULL;
   Geoposition geoposition;
   if (!params.GetDictionary("location", &location) ||
@@ -982,7 +1030,8 @@ Status ExecuteSetNetworkConditions(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   std::string network_name;
   const base::DictionaryValue* conditions = NULL;
   scoped_ptr<NetworkConditions> network_conditions(new NetworkConditions());
@@ -1042,7 +1091,8 @@ Status ExecuteDeleteNetworkConditions(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   // Chrome does not have any command to stop overriding network conditions, so
   // we just override the network conditions with the "No throttling" preset.
   NetworkConditions network_conditions;
@@ -1065,6 +1115,7 @@ Status ExecuteTakeHeapSnapshot(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
-    scoped_ptr<base::Value>* value) {
+    scoped_ptr<base::Value>* value,
+    Timeout* timeout) {
   return web_view->TakeHeapSnapshot(value);
 }