[chromedriver] Fix race where we might not wait for page load after navigating.

Change the NavigationTracker to assume the page is loading after Page.navigate.
BUG=chromedriver:347
R=chrisgao@chromium.org, craigdh@chromium.org

Review URL: https://codereview.chromium.org/15772009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@202651 0039d316-1c4b-4281-b951-d872f2087c98

chrome/chrome_tests.gypi

@@ -814,6 +814,7 @@
         'test/chromedriver/chrome/devtools_client.h',
         'test/chromedriver/chrome/devtools_client_impl.cc',
         'test/chromedriver/chrome/devtools_client_impl.h',
+        'test/chromedriver/chrome/devtools_event_listener.cc',
         'test/chromedriver/chrome/devtools_event_listener.h',
         'test/chromedriver/chrome/devtools_http_client.cc',
         'test/chromedriver/chrome/devtools_http_client.h',

chrome/test/chromedriver/chrome/devtools_client_impl.cc

@@ -178,7 +178,8 @@ Status DevToolsClientImpl::HandleEventsUntil(
   return Status(kOk);
 }
 
-DevToolsClientImpl::ResponseInfo::ResponseInfo() : state(kWaiting) {}
+DevToolsClientImpl::ResponseInfo::ResponseInfo(const std::string& method)
+    : state(kWaiting), method(method) {}
 
 DevToolsClientImpl::ResponseInfo::~ResponseInfo() {}
 
@@ -200,7 +201,8 @@ Status DevToolsClientImpl::SendCommandInternal(
   if (!socket_->Send(message))
     return Status(kDisconnected, "unable to send message to renderer");
 
-  linked_ptr<ResponseInfo> response_info = make_linked_ptr(new ResponseInfo());
+  linked_ptr<ResponseInfo> response_info =
+      make_linked_ptr(new ResponseInfo(method));
   response_info_map_[command_id] = response_info;
   while (response_info->state == kWaiting) {
     Status status = ProcessNextMessage(command_id);
@@ -229,6 +231,9 @@ Status DevToolsClientImpl::ProcessNextMessage(int expected_id) {
   if (status.IsError())
     return status;
   status = EnsureListenersNotifiedOfEvent();
+  if (status.IsError())
+    return status;
+  status = EnsureListenersNotifiedOfCommandResponse();
   if (status.IsError())
     return status;
 
@@ -300,15 +305,25 @@ Status DevToolsClientImpl::ProcessCommandResponse(
   linked_ptr<ResponseInfo> response_info = response_info_map_[response.id];
   if (response_info->state == kReceived)
     return Status(kUnknownError, "received multiple command responses");
+
   if (response_info->state == kIgnored) {
     response_info_map_.erase(response.id);
-    return Status(kOk);
+  } else {
+    response_info->state = kReceived;
+    response_info->response.id = response.id;
+    response_info->response.error = response.error;
+    if (response.result)
+      response_info->response.result.reset(response.result->DeepCopy());
+  }
+
+  if (response.result) {
+    unnotified_cmd_response_listeners_ = listeners_;
+    unnotified_cmd_response_info_ = response_info;
+    Status status = EnsureListenersNotifiedOfCommandResponse();
+    unnotified_cmd_response_info_.reset();
+    if (status.IsError())
+      return status;
   }
-  response_info->state = kReceived;
-  response_info->response.id = response.id;
-  response_info->response.error = response.error;
-  if (response.result)
-    response_info->response.result.reset(response.result->DeepCopy());
   return Status(kOk);
 }
 
@@ -333,6 +348,19 @@ Status DevToolsClientImpl::EnsureListenersNotifiedOfEvent() {
   return Status(kOk);
 }
 
+Status DevToolsClientImpl::EnsureListenersNotifiedOfCommandResponse() {
+  while (unnotified_cmd_response_listeners_.size()) {
+    DevToolsEventListener* listener =
+        unnotified_cmd_response_listeners_.front();
+    unnotified_cmd_response_listeners_.pop_front();
+    Status status =
+        listener->OnCommandSuccess(this, unnotified_cmd_response_info_->method);
+    if (status.IsError())
+      return status;
+  }
+  return Status(kOk);
+}
+
 namespace internal {
 
 bool ParseInspectorMessage(

chrome/test/chromedriver/chrome/devtools_client_impl.h

@@ -103,10 +103,11 @@ class DevToolsClientImpl : public DevToolsClient {
     kReceived
   };
   struct ResponseInfo {
-    ResponseInfo();
+    explicit ResponseInfo(const std::string& method);
     ~ResponseInfo();
 
     ResponseState state;
+    std::string method;
     internal::InspectorCommandResponse response;
   };
   typedef std::map<int, linked_ptr<ResponseInfo> > ResponseInfoMap;
@@ -121,6 +122,7 @@ class DevToolsClientImpl : public DevToolsClient {
       const internal::InspectorCommandResponse& response);
   Status EnsureListenersNotifiedOfConnect();
   Status EnsureListenersNotifiedOfEvent();
+  Status EnsureListenersNotifiedOfCommandResponse();
 
   scoped_ptr<SyncWebSocket> socket_;
   GURL url_;
@@ -132,6 +134,8 @@ class DevToolsClientImpl : public DevToolsClient {
   std::list<DevToolsEventListener*> unnotified_connect_listeners_;
   std::list<DevToolsEventListener*> unnotified_event_listeners_;
   const internal::InspectorEvent* unnotified_event_;
+  std::list<DevToolsEventListener*> unnotified_cmd_response_listeners_;
+  linked_ptr<ResponseInfo> unnotified_cmd_response_info_;
   ResponseInfoMap response_info_map_;
   int next_id_;
   int stack_count_;

chrome/test/chromedriver/chrome/devtools_client_impl_unittest.cc

@@ -1072,3 +1072,54 @@ TEST(DevToolsClientImpl, CorrectlyDeterminesWhichIsBlockedByAlert) {
   msgs.push_back("{\"id\": 6, \"result\": {}}");
   ASSERT_EQ(kOk, client.HandleReceivedEvents().code());
 }
+
+namespace {
+
+class MockCommandListener : public DevToolsEventListener {
+ public:
+  MockCommandListener() {}
+  virtual ~MockCommandListener() {}
+
+  virtual void OnEvent(DevToolsClient* client,
+                       const std::string& method,
+                       const base::DictionaryValue& params) OVERRIDE {
+    msgs_.push_back(method);
+  }
+
+  virtual Status OnCommandSuccess(DevToolsClient* client,
+                                  const std::string& method) OVERRIDE {
+    msgs_.push_back(method);
+    if (!callback_.is_null())
+      callback_.Run(client);
+    return Status(kOk);
+  }
+
+  base::Callback<void(DevToolsClient*)> callback_;
+  std::list<std::string> msgs_;
+};
+
+void HandleReceivedEvents(DevToolsClient* client) {
+  EXPECT_EQ(kOk, client->HandleReceivedEvents().code());
+}
+
+}  // namespace
+
+TEST(DevToolsClientImpl, ReceivesCommandResponse) {
+  std::list<std::string> msgs;
+  SyncWebSocketFactory factory = base::Bind(&CreateMockSyncWebSocket6, &msgs);
+  Logger logger;
+  DevToolsClientImpl client(
+      factory, "http://url", "id", base::Bind(&CloserFunc), &logger);
+  MockCommandListener listener1;
+  listener1.callback_ = base::Bind(&HandleReceivedEvents);
+  MockCommandListener listener2;
+  client.AddListener(&listener1);
+  client.AddListener(&listener2);
+  msgs.push_back("{\"id\": 1, \"result\": {}}");
+  msgs.push_back("{\"method\": \"event\", \"params\": {}}");
+  base::DictionaryValue params;
+  ASSERT_EQ(kOk, client.SendCommand("cmd", params).code());
+  ASSERT_EQ(2u, listener2.msgs_.size());
+  ASSERT_EQ("cmd", listener2.msgs_.front());
+  ASSERT_EQ("event", listener2.msgs_.back());
+}

chrome/test/chromedriver/chrome/devtools_event_listener.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/test/chromedriver/chrome/devtools_event_listener.h"
+
+#include "chrome/test/chromedriver/chrome/status.h"
+
+DevToolsEventListener::~DevToolsEventListener() {}
+
+Status DevToolsEventListener::OnConnected(DevToolsClient* client) {
+  return Status(kOk);
+}
+
+void DevToolsEventListener::OnEvent(DevToolsClient* client,
+                                    const std::string& method,
+                                    const base::DictionaryValue& params) {}
+
+Status DevToolsEventListener::OnCommandSuccess(
+    DevToolsClient* client,
+    const std::string& method) {
+  return Status(kOk);
+}

chrome/test/chromedriver/chrome/devtools_event_listener.h

@@ -14,16 +14,23 @@ class DictionaryValue;
 class DevToolsClient;
 class Status;
 
-// Listens to WebKit Inspector events and DevTools debugger connection.
+// Receives notification of incoming Blink Inspector messages and connection
+// to the DevTools server.
 class DevToolsEventListener {
  public:
-  virtual ~DevToolsEventListener() {}
+  virtual ~DevToolsEventListener();
 
-  virtual Status OnConnected(DevToolsClient* client) = 0;
+  // Called when a connection is made to the DevTools server.
+  virtual Status OnConnected(DevToolsClient* client);
 
+  // Called when an event is received.
   virtual void OnEvent(DevToolsClient* client,
                        const std::string& method,
-                       const base::DictionaryValue& params) = 0;
+                       const base::DictionaryValue& params);
+
+  // Called when a command success response is received.
+  virtual Status OnCommandSuccess(DevToolsClient* client,
+                                  const std::string& method);
 };
 
 #endif  // CHROME_TEST_CHROMEDRIVER_CHROME_DEVTOOLS_EVENT_LISTENER_H_

chrome/test/chromedriver/chrome/navigation_tracker.cc

@@ -126,3 +126,10 @@ void NavigationTracker::OnEvent(DevToolsClient* client,
     scheduled_frame_set_.clear();
   }
 }
+
+Status NavigationTracker::OnCommandSuccess(DevToolsClient* client,
+                                           const std::string& method) {
+  if (method == "Page.navigate")
+    loading_state_ = kLoading;
+  return Status(kOk);
+}

chrome/test/chromedriver/chrome/navigation_tracker.h

@@ -42,6 +42,8 @@ class NavigationTracker : public DevToolsEventListener {
   virtual void OnEvent(DevToolsClient* client,
                        const std::string& method,
                        const base::DictionaryValue& params) OVERRIDE;
+  virtual Status OnCommandSuccess(DevToolsClient* client,
+                                  const std::string& method) OVERRIDE;
 
  private:
   DevToolsClient* client_;

chrome/test/chromedriver/chrome/web_view_impl.cc

@@ -8,6 +8,7 @@
 #include "base/files/file_path.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
+#include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/threading/platform_thread.h"
 #include "base/time.h"
@@ -111,6 +112,10 @@ DevToolsClient* WebViewImpl::GetDevToolsClient() {
 }
 
 Status WebViewImpl::Load(const std::string& url) {
+  // Javascript URLs will cause a hang while waiting for the page to stop
+  // loading, so just disallow.
+  if (StartsWithASCII(url, "javascript:", false))
+    return Status(kUnknownError, "unsupported protocol");
   base::DictionaryValue params;
   params.SetString("url", url);
   return client_->SendCommand("Page.navigate", params);