Validate debuggee.targetId before use in chrome.debugger

chrome/browser/extensions/api/debugger/debugger_apitest.cc

 // Copyright (c) 2012 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 <string>
 
 #include "base/command_line.h"
 #include "base/memory/ref_counted.h"
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
@@ skipping 19 matching lines @@
   ~DebuggerApiTest() override {}
 
   void SetUpCommandLine(base::CommandLine* command_line) override;
   void SetUpOnMainThread() override;
 
   // Run the attach function. If |expected_error| is not empty, then the
   // function should fail with the error. Otherwise, the function is expected
   // to succeed.
   testing::AssertionResult RunAttachFunction(const GURL& url,
                                              const std::string& expected_error);
+  testing::AssertionResult RunAttachFunctionImpl(

[Devlin, 2015/02/09 23:11:22]

Two things:
- Despite what you see around the codebase, we try to avoid "Impl" names (though
we often do a terrible job).  Maybe something like
"RunAttachFunctionOnTarget()"?
- This should probably be private (no tests should be using it directly).

[robwu, 2015/02/10 12:56:31]

Done.

+      const std::string& debuggee_target, const std::string& expected_error);
 
   const Extension* extension() const { return extension_.get(); }
   base::CommandLine* command_line() const { return command_line_; }
 
  private:
   // The command-line for the test process, preserved in order to modify
   // mid-test.
   base::CommandLine* command_line_;
 
   // A basic extension with the debugger permission.
@@ skipping 15 matching lines @@
                              .Set("manifest_version", 2)
                              .Set("permissions",
                                   ListBuilder().Append("debugger"))).Build();
 }
 
 testing::AssertionResult DebuggerApiTest::RunAttachFunction(
     const GURL& url, const std::string& expected_error) {
   ui_test_utils::NavigateToURL(browser(), url);
   content::WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Attach by tabId

[Devlin, 2015/02/09 23:11:22]

nit: comments should usually end in a period (also line 86).

[robwu, 2015/02/10 12:56:31]

Done.

   int tab_id = SessionTabHelper::IdForTab(web_contents);
+  std::string debugee_by_tab = base::StringPrintf("{\"tabId\": %d}", tab_id);
+  testing::AssertionResult result =
+      RunAttachFunctionImpl(debugee_by_tab, expected_error);
+  if (!result)
+    return result;
+
+  // Attach by targetId
+  scoped_refptr<DebuggerGetTargetsFunction> get_targets_function =
+      new DebuggerGetTargetsFunction();
+  scoped_ptr<base::Value> value(
+      extension_function_test_utils::RunFunctionAndReturnSingleResult(
+          get_targets_function.get(), "[]", browser()));
+  base::ListValue* targets = NULL;
+  EXPECT_TRUE(value->GetAsList(&targets));
+
+  std::string debugger_target_id;
+  for (size_t i = 0; i < targets->GetSize(); ++i) {
+    base::DictionaryValue* target_dict = NULL;
+    EXPECT_TRUE(targets->GetDictionary(i, &target_dict));
+    int id;
+    if (target_dict->GetInteger("tabId", &id) && id == tab_id) {
+      EXPECT_TRUE(target_dict->GetString("id", &debugger_target_id));
+      break;
+    }
+  }
+  EXPECT_TRUE(!debugger_target_id.empty());
+
+  std::string debugee_by_target_id =
+      base::StringPrintf("{\"targetId\": \"%s\"}", debugger_target_id.c_str());
+  result = RunAttachFunctionImpl(debugee_by_target_id, expected_error);
+
+  return result;
+}
+
+testing::AssertionResult DebuggerApiTest::RunAttachFunctionImpl(
+    const std::string& debuggee_target, const std::string& expected_error) {
   scoped_refptr<DebuggerAttachFunction> attach_function =
       new DebuggerAttachFunction();
   attach_function->set_extension(extension_.get());
-  std::string args = base::StringPrintf("[{\"tabId\": %d}, \"1.1\"]", tab_id);
 
-  if (!expected_error.empty()) {
-    std::string actual_error =
-        extension_function_test_utils::RunFunctionAndReturnError(
-            attach_function.get(), args, browser());
-    if (actual_error != expected_error) {
-      return testing::AssertionFailure() << "Did not get correct error: "
-          << "expected: " << expected_error << ", found: " << actual_error;
-    }
+  std::string actual_error;
+  if (!RunFunction(attach_function.get(),
+                   base::StringPrintf("[%s, \"1.1\"]", debuggee_target.c_str()),
+                   browser(),
+                   extension_function_test_utils::NONE)) {
+    actual_error = attach_function->GetError();
   } else {
-    if (!RunFunction(attach_function.get(),
-                     args,
-                     browser(),
-                     extension_function_test_utils::NONE)) {
-      return testing::AssertionFailure() << "Could not run function: "
-          << attach_function->GetError();
-    }
-
     // Clean up and detach.
     scoped_refptr<DebuggerDetachFunction> detach_function =
         new DebuggerDetachFunction();
     detach_function->set_extension(extension_.get());
     if (!RunFunction(detach_function.get(),
-                     base::StringPrintf("[{\"tabId\": %d}]", tab_id),
+                     base::StringPrintf("[%s]", debuggee_target.c_str()),
                      browser(),
                      extension_function_test_utils::NONE)) {
-      return testing::AssertionFailure() << "Could not detach: "
-          << detach_function->GetError();
+      return testing::AssertionFailure() << "Could not detach from "
+          << debuggee_target << " : " << detach_function->GetError();
+    }
+  }
+
+  if (expected_error.empty()) {

[Devlin, 2015/02/09 23:11:22]

We can simplify these ifs to be
if (expected_error.empty() && !actual_error.empty()) {
  return ...
} else if (expected_error != actual_error) {
  return ...
}
return success;

[robwu, 2015/02/10 12:56:31]

Done.

+    if (!actual_error.empty()) {
+      return testing::AssertionFailure() << "Could not attach to "
+          << debuggee_target << " : " << actual_error;
+    }
+  } else {
+    if (actual_error != expected_error) {
+      return testing::AssertionFailure() << "Did not get correct error upon "
+          << "attach to " << debuggee_target << " : "
+          << "expected: " << expected_error << ", found: " << actual_error;
     }
   }
   return testing::AssertionSuccess();
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, Debugger) {
   ASSERT_TRUE(RunExtensionTest("debugger")) << message_;
 }
 
 IN_PROC_BROWSER_TEST_F(DebuggerApiTest,
@@ skipping 19 matching lines @@
                                           extension()->id().c_str())),
                   std::string()));
 
   // Append extensions on chrome urls switch. The extension should now be able
   // to debug any extension.
   command_line()->AppendSwitch(switches::kExtensionsOnChromeURLs);
   EXPECT_TRUE(RunAttachFunction(other_ext_url, std::string()));
 }
 
 }  // namespace extensions