[Android] Fix port leak in the forwarder.

[Android] Fix port leak in the forwarder.

The forwarder has been using a CommandLine object to run its
`adb -s <serial> forward --remove <port>` command since October.
CommandLine::AppendSwitchesAndArguments mangles that adb command
into `adb -s --remove <serial> forward <port>` in its attempt to
keep switches before arguments. That mangled command is not valid,
and adb never unmaps the port as a result. The host forwarder
recognized this as a failure, though not a fatal one, and removed
the port from its serial->port map regardless of whether the unmap
command succeeded.

This patch adds a variant of base::GetAppOutputAndError that takes
a vector<string> s.t. arguments are not reordered. (A similar variant
already exists for base::GetAppOutput.) It also switches all uses of
base::GetAppOutputWithExitCode to this new variant.

BUG=691654
Review-Url: https://codereview.chromium.org/2736053003
Cr-Commit-Position: refs/heads/master@{#460440}
Committed: https://chromium.googlesource.com/chromium/src/+/86c756ccdc4ea1293fe6bea1c09b5a627baa0989

[jbudorick, 2017-03-08 04:29:40]

jbudorick@chromium.org changed reviewers:
+ tedchoc@chromium.org

[jbudorick, 2017-03-08 04:29:40]

A special Tuesday edition of Friday Forwarder Fun.

[Ted C, 2017-03-08 04:55:06]

lgtm

https://codereview.chromium.org/2736053003/diff/1/tools/android/forwarder2/ho...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/2736053003/diff/1/tools/android/forwarder2/ho...
tools/android/forwarder2/host_forwarder_main.cc:322: const
std::vector<std::string> split_command = base::SplitString(
maybe a comment here about the need for it to be ordered so using the vector
variant here (I could see an unsuspecting person trying to unify all
GetAppOutput*s and not seeing the difference here.

[jbudorick, 2017-03-08 05:37:39]

jbudorick@chromium.org changed reviewers:
+ thakis@chromium.org

[jbudorick, 2017-03-08 05:37:40]

+thakis for //base owners

https://codereview.chromium.org/2736053003/diff/1/tools/android/forwarder2/ho...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/2736053003/diff/1/tools/android/forwarder2/ho...
tools/android/forwarder2/host_forwarder_main.cc:322: const
std::vector<std::string> split_command = base::SplitString(
On 2017/03/08 04:55:05, Ted C wrote:
> maybe a comment here about the need for it to be ordered so using the vector
> variant here (I could see an unsuspecting person trying to unify all
> GetAppOutput*s and not seeing the difference here.

Done.

[Nico, 2017-03-08 16:18:35]

Why did the forwarder move to CommandLine? Instead of adding a function to
CommandLine just for the forwarder, could that change just be undone?

(You didn't link to it, so I don't know if that switch was a minor cleanup or
part of some bigger thing)

[jbudorick, 2017-03-08 16:24:17]

On 2017/03/08 16:18:35, Nico wrote:
> Why did the forwarder move to CommandLine? Instead of adding a function to
> CommandLine just for the forwarder, could that change just be undone?
> 
> (You didn't link to it, so I don't know if that switch was a minor cleanup or
> part of some bigger thing)

oops, sorry. That was https://codereview.chromium.org/2374183008/.

[Nico, 2017-03-08 16:30:50]

Please add tests for the bug this fixes and for the bug fixed by
https://codereview.chromium.org/2374183008/

Other than that, I suppose this is small enough to be ok.
(http://go/cultivate-recordio if you haven't see it though)

[jbudorick, 2017-03-08 16:34:12]

On 2017/03/08 16:30:50, Nico wrote:
> Please add tests for the bug this fixes and for the bug fixed by
> https://codereview.chromium.org/2374183008/

I'd known the day would come when I'd need to add tests for the forwarder. I'll
work on it and get back to you.

> 
> Other than that, I suppose this is small enough to be ok.
> (http://go/cultivate-recordio if you haven't see it though)

I had not.

If you'd prefer, I can leave out the change in //base, use the existing vector
version of GetAppOutput, and just deal with not getting the stderr, though
that's suboptimal from the forwarder's perspective.

[jbudorick, 2017-03-28 16:25:33]

PTAL.

On 2017/03/08 16:34:12, jbudorick wrote:
> On 2017/03/08 16:30:50, Nico wrote:
> > Please add tests for the bug this fixes and for the bug fixed by
> > https://codereview.chromium.org/2374183008/
> 
> I'd known the day would come when I'd need to add tests for the forwarder.
I'll
> work on it and get back to you.

Split HostControllersManager into its own class, split the adb logic into its
own function for testing, and added tests that cover both bugs. I expect that
adding the new suite to the bots will be a bit tricky as it's the first host
gtest suite that'll run as part of an android build, so I'd prefer to do so in a
separate CL.

> 
> > 
> > Other than that, I suppose this is small enough to be ok.
> > (http://go/cultivate-recordio if you haven't see it though)
> 
> I had not.
> 
> If you'd prefer, I can leave out the change in //base, use the existing vector
> version of GetAppOutput, and just deal with not getting the stderr, though
> that's suboptimal from the forwarder's perspective.

[jbudorick, 2017-03-28 16:27:12]

The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 16:28:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-03-28 16:29:52]

base/ lgtm, but Ted will probably want to look at tools/android again.

[commit-bot: I haz the power, 2017-03-28 16:33:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 16:33:22]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Ted C, 2017-03-28 23:57:37]

lgtm w/ style nits

I didn't look much at the code that moved from one file to another (just looked
for the newer functions)

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
File tools/android/forwarder2/host_controllers_manager.h (right):

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
tools/android/forwarder2/host_controllers_manager.h:21: enum : int {
does this need to be in the header or can it be in the cc file?

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
tools/android/forwarder2/host_controllers_manager.h:43: void
SetAdbPortForDeviceForTesting(const std::string& serial, int adb_port);
at a quick glance the ordering of the .cc file doesn't match the ordering of the
.h

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
File tools/android/forwarder2/host_controllers_manager_unittest.cc (right):

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
tools/android/forwarder2/host_controllers_manager_unittest.cc:40:
HostControllersManager h(base::Bind(&UnusedGetExitNotifierFD));
I would use something like manager over h (or even hcm to be a bit more
verbose)...same below

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
tools/android/forwarder2/host_forwarder_main.cc:50: enum : int {
remove this now?

[jbudorick, 2017-03-29 01:55:42]

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
File tools/android/forwarder2/host_controllers_manager.h (right):

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
tools/android/forwarder2/host_controllers_manager.h:21: enum : int {
On 2017/03/28 23:57:37, Ted C wrote:
> does this need to be in the header or can it be in the cc file?

host_forwarder_main.cc uses it as well.

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
tools/android/forwarder2/host_controllers_manager.h:43: void
SetAdbPortForDeviceForTesting(const std::string& serial, int adb_port);
On 2017/03/28 23:57:37, Ted C wrote:
> at a quick glance the ordering of the .cc file doesn't match the ordering of
the
> .h

Both of these are leftover from some intermediate iteration. Removed.

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
File tools/android/forwarder2/host_controllers_manager_unittest.cc (right):

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
tools/android/forwarder2/host_controllers_manager_unittest.cc:40:
HostControllersManager h(base::Bind(&UnusedGetExitNotifierFD));
On 2017/03/28 23:57:37, Ted C wrote:
> I would use something like manager over h (or even hcm to be a bit more
> verbose)...same below

Switched to manager.

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/2736053003/diff/80001/tools/android/forwarder...
tools/android/forwarder2/host_forwarder_main.cc:50: enum : int {
On 2017/03/28 23:57:37, Ted C wrote:
> remove this now?

ah, yep. purged a bunch of includes from this file, too.

[jbudorick, 2017-03-29 14:56:55]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2017-03-29 14:56:56]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2736053003/#ps100001
(title: "tedchoc comments")

[commit-bot: I haz the power, 2017-03-29 14:57:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 17:34:00]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490799415564630,
"parent_rev": "08a3b590f839b9f93d421b65a0570067b7b604c9", "commit_rev":
"86c756ccdc4ea1293fe6bea1c09b5a627baa0989"}

[commit-bot: I haz the power, 2017-03-29 17:34:45]

Description was changed from

==========
[Android] Fix port leak in the forwarder.

The forwarder has been using a CommandLine object to run its
`adb -s <serial> forward --remove <port>` command since October.
CommandLine::AppendSwitchesAndArguments mangles that adb command
into `adb -s --remove <serial> forward <port>` in its attempt to
keep switches before arguments. That mangled command is not valid,
and adb never unmaps the port as a result. The host forwarder
recognized this as a failure, though not a fatal one, and removed
the port from its serial->port map regardless of whether the unmap
command succeeded.

This patch adds a variant of base::GetAppOutputAndError that takes
a vector<string> s.t. arguments are not reordered. (A similar variant
already exists for base::GetAppOutput.) It also switches all uses of
base::GetAppOutputWithExitCode to this new variant.

BUG=691654
==========

to

==========
[Android] Fix port leak in the forwarder.

The forwarder has been using a CommandLine object to run its
`adb -s <serial> forward --remove <port>` command since October.
CommandLine::AppendSwitchesAndArguments mangles that adb command
into `adb -s --remove <serial> forward <port>` in its attempt to
keep switches before arguments. That mangled command is not valid,
and adb never unmaps the port as a result. The host forwarder
recognized this as a failure, though not a fatal one, and removed
the port from its serial->port map regardless of whether the unmap
command succeeded.

This patch adds a variant of base::GetAppOutputAndError that takes
a vector<string> s.t. arguments are not reordered. (A similar variant
already exists for base::GetAppOutput.) It also switches all uses of
base::GetAppOutputWithExitCode to this new variant.

BUG=691654

Review-Url: https://codereview.chromium.org/2736053003
Cr-Commit-Position: refs/heads/master@{#460440}
Committed:
https://chromium.googlesource.com/chromium/src/+/86c756ccdc4ea1293fe6bea1c09b...
==========

[commit-bot: I haz the power, 2017-03-29 17:34:46]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/86c756ccdc4ea1293fe6bea1c09b...