Validate debuggee.targetId before use in chrome.debugger

Validate debuggee.targetId before use in chrome.debugger

And refactored the tests to make sure that the debugger is detached upon
returning from RunAttachFunction. Previously, if the debugger
unexpectedly succeeded in attaching, the method would return (because
empty error != some error), causing the attached debugger to not be
detached.

R=rdevlin.cronin@chromium.org
BUG=456841
TEST=browser_tests DebuggerApiTest.DebuggerNotAllowedOnOtherExtensionPages

Committed: https://crrev.com/409bf9d6104f83c80cd85bd261784b39cab8e93e
Cr-Commit-Position: refs/heads/master@{#315675}

[robwu, 2015-02-09 21:18:35]

rob@robwu.nl changed reviewers:
+ kalman@chromium.org, meacer@chromium.org

[robwu, 2015-02-09 21:18:35]

rob@robwu.nl changed required reviewers:
+ rdevlin.cronin@chromium.org

[robwu, 2015-02-09 21:18:36]

Devlin: PTAL

Ben, Mustafa: FYI

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

Thanks for fixing this, Rob. :)

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_api.cc:533: if
(PermissionsData::IsRestrictedUrl(agent_host_->GetURL(),
I'm a little worried about race conditions here, if, for instance, the debugger
is attached before the url is fully resolved. Not sure how much we can do about
it, but a sanity check that url is_valid() would probably be better than
nothing.

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_api.cc:537: agent_host_ = NULL;
nit: prefer nullptr now.

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/debugger/debugger_apitest.cc (right):

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:40:
testing::AssertionResult RunAttachFunctionImpl(
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).

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:78: // Attach by
tabId
nit: comments should usually end in a period (also line 86).

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:140: if
(expected_error.empty()) {
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:32]

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_api.cc:533: if
(PermissionsData::IsRestrictedUrl(agent_host_->GetURL(),
On 2015/02/09 23:11:22, Devlin wrote:
> I'm a little worried about race conditions here, if, for instance, the
debugger
> is attached before the url is fully resolved. Not sure how much we can do
about
> it, but a sanity check that url is_valid() would probably be better than
> nothing.

Isn't the IsValidSchemeForExtensions check in PermissionsData::IsRestrictedUrl
sufficient?

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_api.cc:537: agent_host_ = NULL;
On 2015/02/09 23:11:22, Devlin wrote:
> nit: prefer nullptr now.

Done, but the rest of the file still uses NULL (should I change back to NULL,
replace the rest with nullptr, or leave it as-is?).

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/debugger/debugger_apitest.cc (right):

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:40:
testing::AssertionResult RunAttachFunctionImpl(
On 2015/02/09 23:11:22, Devlin wrote:
> 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).

Done.

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:78: // Attach by
tabId
On 2015/02/09 23:11:22, Devlin wrote:
> nit: comments should usually end in a period (also line 86).

Done.

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:140: if
(expected_error.empty()) {
On 2015/02/09 23:11:22, Devlin wrote:
> We can simplify these ifs to be
> if (expected_error.empty() && !actual_error.empty()) {
>   return ...
> } else if (expected_error != actual_error) {
>   return ...
> }
> return success;

Done.

[Devlin, 2015-02-10 17:14:43]

lgtm

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_api.cc:533: if
(PermissionsData::IsRestrictedUrl(agent_host_->GetURL(),
On 2015/02/10 12:56:31, robwu wrote:
> On 2015/02/09 23:11:22, Devlin wrote:
> > I'm a little worried about race conditions here, if, for instance, the
> debugger
> > is attached before the url is fully resolved. Not sure how much we can do
> about
> > it, but a sanity check that url is_valid() would probably be better than
> > nothing.
> 
> Isn't the IsValidSchemeForExtensions check in PermissionsData::IsRestrictedUrl
> sufficient?

Yeah, that'd probably work.  Nevermind.

https://codereview.chromium.org/910053002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/debugger/debugger_api.cc:537: agent_host_ = NULL;
On 2015/02/10 12:56:31, robwu wrote:
> On 2015/02/09 23:11:22, Devlin wrote:
> > nit: prefer nullptr now.
> 
> Done, but the rest of the file still uses NULL (should I change back to NULL,
> replace the rest with nullptr, or leave it as-is?).

The rule for nullptr is pretty much just "prefer nullptr".  So it's worth using
in new code, or when refactoring old code, but probably not worth contaminating
the git blame history for the whole file (which is why we haven't just done a
repo-wide find-and-replace).

https://codereview.chromium.org/910053002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/debugger/debugger_apitest.cc (right):

https://codereview.chromium.org/910053002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:93: base::ListValue*
targets = NULL;
nit: nullptr (also line 98)

https://codereview.chromium.org/910053002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:100: int id;
nit: even though it shouldn't matter, please initialize this to something sane
(-1 is a good choice, since we use that for invalid tab ids).

https://codereview.chromium.org/910053002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:110: result =
RunAttachFunctionOnTarget(debugee_by_target_id, expected_error);
nit: just return RunAttach... here.

[robwu, 2015-02-10 22:35:33]

New patchsets have been uploaded after l-g-t-m from rdevlin.cronin@chromium.org

[robwu, 2015-02-10 22:35:44]

https://codereview.chromium.org/910053002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/debugger/debugger_apitest.cc (right):

https://codereview.chromium.org/910053002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:93: base::ListValue*
targets = NULL;
On 2015/02/10 17:14:42, Devlin wrote:
> nit: nullptr (also line 98)

Done.

https://codereview.chromium.org/910053002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:100: int id;
On 2015/02/10 17:14:42, Devlin wrote:
> nit: even though it shouldn't matter, please initialize this to something sane
> (-1 is a good choice, since we use that for invalid tab ids).

Done.

https://codereview.chromium.org/910053002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/debugger/debugger_apitest.cc:110: result =
RunAttachFunctionOnTarget(debugee_by_target_id, expected_error);
On 2015/02/10 17:14:42, Devlin wrote:
> nit: just return RunAttach... here.

Done.

[robwu, 2015-02-10 22:36:16]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2015-02-10 22:36:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910053002/40001

[commit-bot: I haz the power, 2015-02-10 23:37:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-10 23:37:46]

Patchset 3 (id:??) landed as
https://crrev.com/409bf9d6104f83c80cd85bd261784b39cab8e93e
Cr-Commit-Position: refs/heads/master@{#315675}