Introduce a ChromeDriver automation version constant and a JSON request

for the client to fetch the server's version. If the server's version is
newer than the client's, warn the client and quit.

Additional small changes:
-Add /healthz callback that sends a 200 status, for checking if the server is up.
-Fix shutdown crash where the AutomationProxy is deleted on the wrong thread.
-Initialize logging correctly.
-Disable mongoose file serving capabilities by default
BUG=none
TEST=none

Review URL: http://codereview.chromium.org/6690060

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

14 files changed, 222 insertions, 58 deletions

diff --git a/chrome/test/automation/automation_json_requests.cc b/chrome/test/automation/automation_json_requests.cc
index 6fe72ad..8d7853d 100644
--- a/chrome/test/automation/automation_json_requests.cc
+++ b/chrome/test/automation/automation_json_requests.cc
@@ -472,3 +472,13 @@ bool SendWaitForAllTabsToStopLoadingJSONRequest(
   DictionaryValue reply_dict;
   return SendAutomationJSONRequest(sender, dict, &reply_dict);
 }
+
+bool SendGetChromeDriverAutomationVersion(
+    AutomationMessageSender* sender, int* version) {
+  DictionaryValue dict;
+  dict.SetString("command", "GetChromeDriverAutomationVersion");
+  DictionaryValue reply_dict;
+  if (!SendAutomationJSONRequest(sender, dict, &reply_dict))
+    return false;
+  return reply_dict.GetInteger("version", version);
+}
diff --git a/chrome/test/automation/automation_json_requests.h b/chrome/test/automation/automation_json_requests.h
index c8019e8..a08442e 100644
--- a/chrome/test/automation/automation_json_requests.h
+++ b/chrome/test/automation/automation_json_requests.h
@@ -243,4 +243,10 @@ bool SendNativeKeyEventJSONRequest(
 bool SendWaitForAllTabsToStopLoadingJSONRequest(
     AutomationMessageSender* sender) WARN_UNUSED_RESULT;
 
+// Requests the version of ChromeDriver automation supported by the automation
+// server. Returns true on success.
+bool SendGetChromeDriverAutomationVersion(
+    AutomationMessageSender* sender,
+    int* version) WARN_UNUSED_RESULT;
+
 #endif  // CHROME_TEST_AUTOMATION_AUTOMATION_JSON_REQUESTS_H_
diff --git a/chrome/test/webdriver/automation.cc b/chrome/test/webdriver/automation.cc
index b1fb662..020e52a 100644
--- a/chrome/test/webdriver/automation.cc
+++ b/chrome/test/webdriver/automation.cc
@@ -120,11 +120,10 @@ Automation::Automation() {}
 
 Automation::~Automation() {}
 
-void Automation::Init(const FilePath& user_browser_dir, bool* success) {
+void Automation::Init(const FilePath& user_browser_dir, ErrorCode* code) {
   FilePath browser_dir = user_browser_dir;
   if (browser_dir.empty() && !GetDefaultChromeExeDir(&browser_dir)) {
-    LOG(ERROR) << "Could not locate Chrome application directory";
-    *success = false;
+    *code = kBrowserCouldNotBeFound;
     return;
   }
 
@@ -149,7 +148,17 @@ void Automation::Init(const FilePath& user_browser_dir, bool* success) {
       true   // show_window
   };
   launcher_->LaunchBrowserAndServer(launch_props, true);
-  *success = launcher_->IsBrowserRunning();
+  if (!launcher_->IsBrowserRunning()) {
+    *code = kBrowserFailedToStart;
+    return;
+  }
+  int version = 0;
+  if (!SendGetChromeDriverAutomationVersion(automation(), &version) ||
+      version > automation::kChromeDriverAutomationVersion) {
+    *code = kIncompatibleBrowserVersion;
+    return;
+  }
+  *code = kSuccess;
 }
 
 void Automation::Terminate() {
@@ -394,6 +403,10 @@ void Automation::GetBrowserVersion(std::string* version) {
   *version = automation()->server_version();
 }
 
+void Automation::GetChromeDriverAutomationVersion(int* version, bool* success) {
+  *success = SendGetChromeDriverAutomationVersion(automation(), version);
+}
+
 void Automation::WaitForAllTabsToStopLoading(bool* success) {
   *success = SendWaitForAllTabsToStopLoadingJSONRequest(automation());
 }
diff --git a/chrome/test/webdriver/automation.h b/chrome/test/webdriver/automation.h
index d390447..ffd8dd4 100644
--- a/chrome/test/webdriver/automation.h
+++ b/chrome/test/webdriver/automation.h
@@ -13,6 +13,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/task.h"
 #include "chrome/common/automation_constants.h"
+#include "chrome/test/webdriver/error_codes.h"
 #include "ui/base/keycodes/keyboard_codes.h"
 
 class AutomationProxy;
@@ -42,7 +43,7 @@ class Automation {
 
   // Creates a browser, using the exe found in |browser_dir|. If |browser_dir|
   // is empty, it will search in all the default locations.
-  void Init(const FilePath& browser_dir, bool* success);
+  void Init(const FilePath& browser_dir, ErrorCode* code);
 
   // Terminates this session and disconnects its automation proxy. After
   // invoking this method, the Automation can safely be deleted.
@@ -116,6 +117,10 @@ class Automation {
   // Gets the version of the runing browser.
   void GetBrowserVersion(std::string* version);
 
+  // Gets the ChromeDriver automation version supported by the automation
+  // server.
+  void GetChromeDriverAutomationVersion(int* version, bool* success);
+
   // Waits for all tabs to stop loading.
   void WaitForAllTabsToStopLoading(bool* success);
 
diff --git a/chrome/test/webdriver/chromedriver_launcher.py b/chrome/test/webdriver/chromedriver_launcher.py
index 3390674..e4be1c8 100644
--- a/chrome/test/webdriver/chromedriver_launcher.py
+++ b/chrome/test/webdriver/chromedriver_launcher.py
@@ -41,9 +41,8 @@ class ChromeDriverLauncher:
         raise RuntimeError('ChromeDriver exe could not be found in its default '
                            'location. Searched in following directories: ' +
                            ', '.join(self.DefaultExeLocations()))
-    if self._root_path is None:
-      self._root_path = '.'
-    self._root_path = os.path.abspath(self._root_path)
+    if self._root_path is not None:
+      self._root_path = os.path.abspath(self._root_path)
     self._process = None
 
     if not os.path.exists(self._exe_path):
@@ -128,7 +127,9 @@ class ChromeDriverLauncher:
     if self._process is not None:
       self.Kill()
 
-    chromedriver_args = [self._exe_path, '--root=%s' % self._root_path]
+    chromedriver_args = [self._exe_path]
+    if self._root_path is not None:
+      chromedriver_args += ['--root=%s' % self._root_path]
     if self._port is not None:
       chromedriver_args += ['--port=%d' % self._port]
     if self._url_base is not None:
@@ -200,6 +201,3 @@ class ChromeDriverLauncher:
 
   def GetPort(self):
     return self._port
-
-  def __del__(self):
-    self.Kill()
diff --git a/chrome/test/webdriver/chromedriver_tests.py b/chrome/test/webdriver/chromedriver_tests.py
index e51e411..b92a04a 100755
--- a/chrome/test/webdriver/chromedriver_tests.py
+++ b/chrome/test/webdriver/chromedriver_tests.py
@@ -113,13 +113,13 @@ class BasicTest(unittest.TestCase):
   def tearDown(self):
     self._launcher.Kill()
 
-  def testShouldReturn404WhenSentAnUnknownCommandURL(self):
+  def testShouldReturn403WhenSentAnUnknownCommandURL(self):
     request_url = self._launcher.GetURL() + '/foo'
     try:
       SendRequest(request_url, method='GET')
-      self.fail('Should have raised a urllib.HTTPError for returned 404')
+      self.fail('Should have raised a urllib.HTTPError for returned 403')
     except urllib2.HTTPError, expected:
-      self.assertEquals(404, expected.code)
+      self.assertEquals(403, expected.code)
 
   def testShouldReturnHTTP405WhenSendingANonPostToTheSessionURL(self):
     request_url = self._launcher.GetURL() + '/session'
@@ -139,14 +139,20 @@ class BasicTest(unittest.TestCase):
       self.assertEquals(404, expected.code)
 
   def testShouldReturn204ForFaviconRequests(self):
-    # Disabled until new python bindings are pulled in.
-    return
     request_url = self._launcher.GetURL() + '/favicon.ico'
-    response = SendRequest(request_url, method='GET')
-    try:
-      self.assertEquals(204, response.code)
-    finally:
-      response.close()
+    # In python2.5, a 204 status code causes an exception.
+    if sys.version_info[0:2] == (2, 5):
+      try:
+        SendRequest(request_url, method='GET')
+        self.fail('Should have raised a urllib.HTTPError for returned 204')
+      except urllib2.HTTPError, expected:
+        self.assertEquals(204, expected.code)
+    else:
+      response = SendRequest(request_url, method='GET')
+      try:
+        self.assertEquals(204, response.code)
+      finally:
+        response.close()
 
   def testCanStartChromeDriverOnSpecificPort(self):
     launcher = ChromeDriverLauncher(port=9520)
@@ -156,6 +162,26 @@ class BasicTest(unittest.TestCase):
     launcher.Kill()
 
 
+class WebserverTest(unittest.TestCase):
+  """Tests the built-in ChromeDriver webserver."""
+
+  def testShouldNotServeFilesByDefault(self):
+    launcher = ChromeDriverLauncher()
+    try:
+      SendRequest(launcher.GetURL(), method='GET')
+      self.fail('Should have raised a urllib.HTTPError for returned 403')
+    except urllib2.HTTPError, expected:
+      self.assertEquals(403, expected.code)
+    finally:
+      launcher.Kill()
+
+  def testCanServeFiles(self):
+    launcher = ChromeDriverLauncher(root_path=os.path.dirname(__file__))
+    request_url = launcher.GetURL() + '/' + os.path.basename(__file__)
+    SendRequest(request_url, method='GET')
+    launcher.Kill()
+
+
 class NativeInputTest(unittest.TestCase):
   """Native input ChromeDriver tests."""
 
@@ -199,7 +225,7 @@ class CookieTest(unittest.TestCase):
   """Cookie test for the json webdriver protocol"""
 
   def setUp(self):
-    self._launcher = ChromeDriverLauncher()
+    self._launcher = ChromeDriverLauncher(root_path=os.path.dirname(__file__))
     self._driver = WebDriver(self._launcher.GetURL(),
                              DesiredCapabilities.CHROME)
 
diff --git a/chrome/test/webdriver/commands/create_session.cc b/chrome/test/webdriver/commands/create_session.cc
index 73bb97d..5c9e95b 100644
--- a/chrome/test/webdriver/commands/create_session.cc
+++ b/chrome/test/webdriver/commands/create_session.cc
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/file_path.h"
+#include "base/stringprintf.h"
 #include "base/values.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/common/chrome_constants.h"
@@ -17,6 +18,9 @@
 
 namespace webdriver {
 
+// The minimum supported version of Chrome for this version of ChromeDriver.
+const int kMinSupportedChromeVersion = 12;
+
 CreateSession::CreateSession(const std::vector<std::string>& path_segments,
                              const DictionaryValue* const parameters)
     : Command(path_segments, parameters) {}
@@ -30,10 +34,33 @@ void CreateSession::ExecutePost(Response* const response) {
 
   // Session manages its own liftime, so do not call delete.
   Session* session = new Session();
-  if (!session->Init(session_manager->chrome_dir())) {
+  ErrorCode code = session->Init(session_manager->chrome_dir());
+
+  if (code == kBrowserCouldNotBeFound) {
     SET_WEBDRIVER_ERROR(response,
-                        "Failed to initialize session",
-                        kInternalServerError);
+                        "Chrome could not be found.",
+                        kUnknownError);
+    return;
+  } else if (code == kBrowserFailedToStart) {
+    std::string error_msg = base::StringPrintf(
+        "Chrome could not be started successfully. "
+            "Please update ChromeDriver and ensure you are using Chrome %d+.",
+        kMinSupportedChromeVersion);
+    SET_WEBDRIVER_ERROR(response, error_msg, kUnknownError);
+    return;
+  } else if (code == kIncompatibleBrowserVersion) {
+    std::string error_msg = base::StringPrintf(
+        "Version of Chrome is incompatible with version of ChromeDriver. "
+            "Please update ChromeDriver and ensure you are using Chrome %d+.",
+        kMinSupportedChromeVersion);
+    SET_WEBDRIVER_ERROR(response, error_msg, kUnknownError);
+    return;
+  } else if (code != kSuccess) {
+    std::string error_msg = base::StringPrintf(
+        "Unknown error while initializing session. "
+            "Ensure ChromeDriver is up-to-date and Chrome is version %d+.",
+        kMinSupportedChromeVersion);
+    SET_WEBDRIVER_ERROR(response, error_msg, kUnknownError);
     return;
   }
 
diff --git a/chrome/test/webdriver/commands/mouse_commands.cc b/chrome/test/webdriver/commands/mouse_commands.cc
index a802a98..20eeff3 100644
--- a/chrome/test/webdriver/commands/mouse_commands.cc
+++ b/chrome/test/webdriver/commands/mouse_commands.cc
@@ -103,7 +103,7 @@ DragCommand::DragCommand(const std::vector<std::string>& path_segments,
 DragCommand::~DragCommand() {}
 
 bool DragCommand::Init(Response* const response) {
-  if (WebDriverCommand::Init(response)) {
+  if (WebElementCommand::Init(response)) {
     if (!GetIntegerParameter("x", &drag_x_) ||
         !GetIntegerParameter("y", &drag_y_)) {
       SET_WEBDRIVER_ERROR(response,
diff --git a/chrome/test/webdriver/dispatch.cc b/chrome/test/webdriver/dispatch.cc
index 6e0a531..29176c8 100644
--- a/chrome/test/webdriver/dispatch.cc
+++ b/chrome/test/webdriver/dispatch.cc
@@ -59,6 +59,15 @@ void Shutdown(struct mg_connection* connection,
   shutdown_event->Signal();
 }
 
+void SendStatus(struct mg_connection* connection,
+                const struct mg_request_info* request_info,
+                void* user_data) {
+  std::string response = "HTTP/1.1 200 OK\r\n"
+                         "Content-Length:2\r\n\r\n"
+                         "ok";
+  mg_write(connection, response.data(), response.length());
+}
+
 void SendNoContentResponse(struct mg_connection* connection,
                            const struct mg_request_info* request_info,
                            void* user_data) {
@@ -68,6 +77,12 @@ void SendNoContentResponse(struct mg_connection* connection,
   mg_write(connection, response.data(), response.length());
 }
 
+void SendForbidden(struct mg_connection* connection,
+                   const struct mg_request_info* request_info,
+                   void* user_data) {
+  mg_printf(connection, "HTTP/1.1 403 Forbidden\r\n\r\n");
+}
+
 void SendNotImplementedError(struct mg_connection* connection,
                              const struct mg_request_info* request_info,
                              void* user_data) {
@@ -268,9 +283,17 @@ void Dispatcher::AddShutdown(const std::string& pattern,
                       shutdown_event);
 }
 
+void Dispatcher::AddStatus(const std::string& pattern) {
+  mg_set_uri_callback(context_, (root_ + pattern).c_str(), &SendStatus, NULL);
+}
+
 void Dispatcher::SetNotImplemented(const std::string& pattern) {
   mg_set_uri_callback(context_, (root_ + pattern).c_str(),
                       &SendNotImplementedError, NULL);
 }
 
+void Dispatcher::ForbidAllOtherRequests() {
+  mg_set_uri_callback(context_, "*", &SendForbidden, NULL);
+}
+
 }  // namespace webdriver
diff --git a/chrome/test/webdriver/dispatch.h b/chrome/test/webdriver/dispatch.h
index b3ee57c..5d86a52 100644
--- a/chrome/test/webdriver/dispatch.h
+++ b/chrome/test/webdriver/dispatch.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -97,10 +97,20 @@ class Dispatcher {
   void AddShutdown(const std::string& pattern,
                    base::WaitableEvent* shutdown_event);
 
+  // Registers a callback for the given pattern that will return a simple
+  // "HTTP/1.1 200 OK" message with "ok" in the body. Used for checking the
+  // status of the server.
+  void AddStatus(const std::string& pattern);
+
   // Registers a callback that will always respond with a
   // "HTTP/1.1 501 Not Implemented" message.
   void SetNotImplemented(const std::string& pattern);
 
+  // Registers a callback that will respond for all other requests with a
+  // "HTTP/1.1 403 Forbidden" message. Should be called only after registering
+  // other callbacks.
+  void ForbidAllOtherRequests();
+
  private:
   struct mg_context* context_;
   const std::string root_;
diff --git a/chrome/test/webdriver/error_codes.h b/chrome/test/webdriver/error_codes.h
index ed5a83f..78060f8 100644
--- a/chrome/test/webdriver/error_codes.h
+++ b/chrome/test/webdriver/error_codes.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -23,6 +23,13 @@ enum ErrorCode {
   kNoSuchWindow = 23,
   kInvalidCookieDomain = 24,
   kUnableToSetCookie = 25,
+
+  // Non-standard error codes.
+  kBrowserCouldNotBeFound = 50,
+  kBrowserFailedToStart,
+  kIncompatibleBrowserVersion,
+
+  // HTTP status codes.
   kSeeOther = 303,
   kBadRequest = 400,
   kSessionNotFound = 404,
@@ -33,4 +40,3 @@ enum ErrorCode {
 }  // namespace webdriver
 
 #endif  // CHROME_TEST_WEBDRIVER_ERROR_CODES_H_
-
diff --git a/chrome/test/webdriver/server.cc b/chrome/test/webdriver/server.cc
index 22711f5..1824b3d 100644
--- a/chrome/test/webdriver/server.cc
+++ b/chrome/test/webdriver/server.cc
@@ -26,6 +26,7 @@
 #include "base/threading/platform_thread.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/test/webdriver/dispatch.h"
 #include "chrome/test/webdriver/error_codes.h"
 #include "chrome/test/webdriver/session_manager.h"
@@ -75,8 +76,10 @@ signal_handler(int sig_num) {
 namespace webdriver {
 
 void InitCallbacks(struct mg_context* ctx, Dispatcher* dispatcher,
-                   base::WaitableEvent* shutdown_event) {
+                   base::WaitableEvent* shutdown_event,
+                   bool forbid_other_requests) {
   dispatcher->AddShutdown("/shutdown", shutdown_event);
+  dispatcher->AddStatus("/healthz");
 
   dispatcher->Add<CreateSession>("/session");
 
@@ -140,12 +143,42 @@ void InitCallbacks(struct mg_context* ctx, Dispatcher* dispatcher,
   dispatcher->SetNotImplemented("/session/*/timeouts/async_script");
 
   // Since the /session/* is a wild card that would match the above URIs, this
-  // line MUST be the last registered URI with the server.
+  // line MUST be after all other webdriver command callbacks.
   dispatcher->Add<SessionWithID>("/session/*");
+
+  if (forbid_other_requests)
+    dispatcher->ForbidAllOtherRequests();
 }
 
 }  // namespace webdriver
 
+// Initializes logging for ChromeDriver.
+void InitChromeDriverLogging(const CommandLine& command_line) {
+  bool success = InitLogging(
+      FILE_PATH_LITERAL("chromedriver.log"),
+      logging::LOG_TO_BOTH_FILE_AND_SYSTEM_DEBUG_LOG,
+      logging::LOCK_LOG_FILE,
+      logging::DELETE_OLD_LOG_FILE,
+      logging::DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS);
+  if (!success) {
+    PLOG(ERROR) << "Unable to initialize logging";
+  }
+  logging::SetLogItems(false,  // enable_process_id
+                       false,  // enable_thread_id
+                       true,   // enable_timestamp
+                       false); // enable_tickcount
+  if (command_line.HasSwitch(switches::kLoggingLevel)) {
+    std::string log_level = command_line.GetSwitchValueASCII(
+        switches::kLoggingLevel);
+    int level = 0;
+    if (base::StringToInt(log_level, &level)) {
+      logging::SetMinLogLevel(level);
+    } else {
+      LOG(WARNING) << "Bad log level: " << log_level;
+    }
+  }
+}
+
 // Configures mongoose according to the given command line flags.
 // Returns true on success.
 bool SetMongooseOptions(struct mg_context* ctx,
@@ -186,6 +219,7 @@ int main(int argc, char *argv[]) {
   // built Chrome.
   chrome::RegisterPathProvider();
   TestTimeouts::Initialize();
+  InitChromeDriverLogging(*cmd_line);
 
   // Parse command line flags.
   std::string port = "9515";
@@ -194,8 +228,9 @@ int main(int argc, char *argv[]) {
   std::string url_base;
   if (cmd_line->HasSwitch("port"))
     port = cmd_line->GetSwitchValueASCII("port");
-  // By default, mongoose serves files from the current working directory. The
-  // 'root' flag allows the user to specify a different location to serve from.
+  // The 'root' flag allows the user to specify a location to serve files from.
+  // If it is not given, a callback will be registered to forbid all file
+  // requests.
   if (cmd_line->HasSwitch("root"))
     root = cmd_line->GetSwitchValueASCII("root");
   if (cmd_line->HasSwitch("chrome-dir"))
@@ -233,7 +268,7 @@ int main(int argc, char *argv[]) {
   }
 
   webdriver::Dispatcher dispatcher(ctx, url_base);
-  webdriver::InitCallbacks(ctx, &dispatcher, &shutdown_event);
+  webdriver::InitCallbacks(ctx, &dispatcher, &shutdown_event, root.empty());
 
   // The tests depend on parsing the first line ChromeDriver outputs,
   // so all other logging should happen after this.
diff --git a/chrome/test/webdriver/session.cc b/chrome/test/webdriver/session.cc
index 693b574..55aa190 100644
--- a/chrome/test/webdriver/session.cc
+++ b/chrome/test/webdriver/session.cc
@@ -69,20 +69,22 @@ Session::~Session() {
   SessionManager::GetInstance()->Remove(id_);
 }
 
-bool Session::Init(const FilePath& browser_dir) {
-  bool success = false;
-  if (thread_.Start()) {
-    RunSessionTask(NewRunnableMethod(
-        this,
-        &Session::InitOnSessionThread,
-        browser_dir,
-        &success));
-  } else {
+ErrorCode Session::Init(const FilePath& browser_dir) {
+  if (!thread_.Start()) {
     LOG(ERROR) << "Cannot start session thread";
-  }
-  if (!success)
     delete this;
-  return success;
+    return kUnknownError;
+  }
+
+  ErrorCode code = kUnknownError;
+  RunSessionTask(NewRunnableMethod(
+      this,
+      &Session::InitOnSessionThread,
+      browser_dir,
+      &code));
+  if (code != kSuccess)
+    Terminate();
+  return code;
 }
 
 void Session::Terminate() {
@@ -903,24 +905,27 @@ void Session::RunSessionTaskOnSessionThread(Task* task,
   done_event->Signal();
 }
 
-void Session::InitOnSessionThread(const FilePath& browser_dir, bool* success) {
+void Session::InitOnSessionThread(const FilePath& browser_dir,
+                                  ErrorCode* code) {
   automation_.reset(new Automation());
-  automation_->Init(browser_dir, success);
-  if (!*success)
+  automation_->Init(browser_dir, code);
+  if (*code != kSuccess)
     return;
 
+  bool success = false;
   std::vector<int> tab_ids;
-  automation_->GetTabIds(&tab_ids, success);
-  if (!*success) {
+  automation_->GetTabIds(&tab_ids, &success);
+  if (!success) {
     LOG(ERROR) << "Could not get tab ids";
+    *code = kUnknownError;
     return;
   }
   if (tab_ids.empty()) {
     LOG(ERROR) << "No tab ids after initialization";
-    *success = false;
-  } else {
-    current_target_ = FrameId(tab_ids[0], FramePath());
+    *code = kUnknownError;
+    return;
   }
+  current_target_ = FrameId(tab_ids[0], FramePath());
 }
 
 void Session::TerminateOnSessionThread() {
diff --git a/chrome/test/webdriver/session.h b/chrome/test/webdriver/session.h
index bdf0e16..a7b3e91 100644
--- a/chrome/test/webdriver/session.h
+++ b/chrome/test/webdriver/session.h
@@ -62,8 +62,8 @@ class Session {
   // Starts the session thread and a new browser, using the exe found in
   // |browser_dir|. If |browser_dir| is empty, it will search in all the default
   // locations. Returns true on success. On failure, the session will delete
-  // itself and return false.
-  bool Init(const FilePath& browser_dir);
+  // itself and return an error code.
+  ErrorCode Init(const FilePath& browser_dir);
 
   // Terminates this session and deletes itself.
   void Terminate();
@@ -226,7 +226,7 @@ class Session {
   void RunSessionTaskOnSessionThread(
       Task* task,
       base::WaitableEvent* done_event);
-  void InitOnSessionThread(const FilePath& browser_dir, bool* success);
+  void InitOnSessionThread(const FilePath& browser_dir, ErrorCode* code);
   void TerminateOnSessionThread();
   void SendKeysOnSessionThread(const string16& keys, bool* success);
   ErrorCode SwitchToFrameWithJavaScriptLocatedFrame(