Use CreateProcess instead of subprocess.Popen to launch Chrome in the mini_installer test framework.

Use CreateProcess instead of subprocess.Popen to launch Chrome in the mini_installer test framework.

When creating a new process using the subprocess module in Python, the parent process's stdout will never be closed until the child process exits. This caused the mini_installer test slave to time out since it waited for the stdout of python.exe to close, but chrome.exe was still running. See a sample error message at http://goo.gl/XUYlu3. Using Windows API "CreateProcess" fixes this issue.

NOTRY=True
BUG=264859
TEST=
1) Uninstall Chrome (if it's installed.)
2) Build Chrome with Release mode and make sure that mini_installer.exe is created.
3) Go to src\chrome\test\mini_installer
4) Create the following Python script:

import subprocess
command = ['python', 'test_installer.py', 'config\\config.config', r'--build-dir=<build-dir>', '--target=Release']
p = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, bufsize=0)
f = p.stdout
while f.read(1): pass
print 'done'

where <build-dir> is the path to main build directory (the parent of the Release directory). Then save it as test.py.
5) Run "python test.py". You should see "done" in the output.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224256

[sukolsak, 2013-09-13 06:04:59]

Could you please take a look at this?

[robertshield, 2013-09-13 12:55:12]

LGTM in the sense that I like that this avoids a non-obvious side effect of
subprocess.Popen. 

I think we should also make sure that we have no running Chrome, installer or
other child processes when the tests complete though. 

See
http://nullege.com/codes/show/src@w@i@windmill-1.6@windmill@browser@killablep...
and 
http://stackoverflow.com/questions/53208/how-do-i-automatically-destroy-child...
for one way of accomplishing this.

A more hacky way would be to simply enumerate and kill chrome.exe, setup.exe and
installer.exe at the end of the test. 

One last note: I believe that the clean state should also check that these
processes are not running and fail if they are.

[gab, 2013-09-13 15:11:14]

Why is Chrome still running? That's a problem imo and I'm not sure we should
workaround it...

[sukolsak, 2013-09-13 15:48:36]

On 2013/09/13 15:11:14, gab wrote:
> Why is Chrome still running? That's a problem imo and I'm not sure we should
> workaround it...

There is a bug in the installer that creates
HKEY_CURRENT_USER\Software\Chromium\BrowserCrashDumpAttempts when we first run
Chromium at system level after installing/uninstalling Chromium at user level.
This causes the test to fail since HKEY_CURRENT_USER\Software\Chromium is not
supposed to exist at system level, and that's why Chrome is still running. It
will be closed once the patch for cleaning the machine is up. But using job
objects as Robert suggested could also work.

[sukolsak, 2013-09-13 18:08:15]

On 2013/09/13 15:48:36, sukolsak wrote:
> On 2013/09/13 15:11:14, gab wrote:
> > Why is Chrome still running? That's a problem imo and I'm not sure we should
> > workaround it...
> 
> There is a bug in the installer that creates
> HKEY_CURRENT_USER\Software\Chromium\BrowserCrashDumpAttempts when we first run
> Chromium at system level after installing/uninstalling Chromium at user level.
> This causes the test to fail since HKEY_CURRENT_USER\Software\Chromium is not
> supposed to exist at system level, and that's why Chrome is still running. It
> will be closed once the patch for cleaning the machine is up. But using job
> objects as Robert suggested could also work.

It seems that we can't put Chrome process handle in a job object, because Chrome
also uses a job object to manage its child processes. The ability to use nested
jobs was only add in Windows 8. If we do it in Windows 7, we will not be able to
use tabs. You can try this with the following script:

import time
import win32job
import win32process

job_handle = win32job.CreateJobObject(None, '')
extended_info = win32job.QueryInformationJobObject(job_handle,
win32job.JobObjectExtendedLimitInformation)
extended_info['BasicLimitInformation']['LimitFlags'] =
win32job.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE
win32job.SetInformationJobObject(job_handle,
win32job.JobObjectExtendedLimitInformation, extended_info)

chrome_path = 'path\\to\\chrome.exe'
process_handle, _, _, _ = win32process.CreateProcess(None, chrome_path, None,
None, 0, 0, None, None, win32process.STARTUPINFO())
win32job.AssignProcessToJobObject(job_handle, process_handle)
time.sleep(600)

I work around this by using "taskkill /f /im chrome.exe".

[grt (UTC plus 2), 2013-09-13 18:55:40]

lgtm

[gab, 2013-09-13 20:25:05]

On 2013/09/13 15:48:36, sukolsak wrote:
> On 2013/09/13 15:11:14, gab wrote:
> > Why is Chrome still running? That's a problem imo and I'm not sure we should
> > workaround it...
> 
> There is a bug in the installer that creates
> HKEY_CURRENT_USER\Software\Chromium\BrowserCrashDumpAttempts when we first run
> Chromium at system level after installing/uninstalling Chromium at user level.
> This causes the test to fail since HKEY_CURRENT_USER\Software\Chromium is not
> supposed to exist at system level, and that's why Chrome is still running. It
> will be closed once the patch for cleaning the machine is up. But using job
> objects as Robert suggested could also work.

Ah, so the test fails while chrome is running and we thus never kill chrome.exe?

Would it make sense after a failure in one state to proceed with the actions
remaining (perhaps without verifying remaining states?).

Even that might not be enough if the test process dies in the middle say...; how
about having the cleaning step at the beginning of the test fixture also kill
the known processes (and by "known" I mean those listed as not expected to be
running in the clean state -- maybe that's just chrome.exe)?

[sukolsak, 2013-09-13 21:06:28]

On 2013/09/13 20:25:05, gab wrote:
> On 2013/09/13 15:48:36, sukolsak wrote:
> > On 2013/09/13 15:11:14, gab wrote:
> > > Why is Chrome still running? That's a problem imo and I'm not sure we
should
> > > workaround it...
> > 
> > There is a bug in the installer that creates
> > HKEY_CURRENT_USER\Software\Chromium\BrowserCrashDumpAttempts when we first
run
> > Chromium at system level after installing/uninstalling Chromium at user
level.
> > This causes the test to fail since HKEY_CURRENT_USER\Software\Chromium is
not
> > supposed to exist at system level, and that's why Chrome is still running.
It
> > will be closed once the patch for cleaning the machine is up. But using job
> > objects as Robert suggested could also work.
> 
> Ah, so the test fails while chrome is running and we thus never kill
chrome.exe?

Yes.

> Would it make sense after a failure in one state to proceed with the actions
> remaining (perhaps without verifying remaining states?).

Proceeding with the remaining actions even though we know that it is wrong might
have unintended consequences. I'd rather have it fail fast.

> Even that might not be enough if the test process dies in the middle say...;
how
> about having the cleaning step at the beginning of the test fixture also kill
> the known processes (and by "known" I mean those listed as not expected to be
> running in the clean state -- maybe that's just chrome.exe)?

I think this is what this CL (https://codereview.chromium.org/23523045/) is
doing. It calls _RunCleanCommand() every time before running a test. This method
is supposed to do everything to put the machine in the clean state, which
includes killing chrome.exe (and other known processes.) Currently, it calls
uninstall_chrome.py, which calls UninstallString with the --force-uninstall
option. This option kills chrome.exe.

[gab, 2013-09-13 21:16:09]

https://codereview.chromium.org/24126005/diff/6001/chrome/test/mini_installer...
File chrome/test/mini_installer/test_installer.py (right):

https://codereview.chromium.org/24126005/diff/6001/chrome/test/mini_installer...
chrome/test/mini_installer/test_installer.py:208: subprocess.call('taskkill /f
/im chrome.exe', shell=True)
If the intention is to exit cleanly, would it make more sense to call
_RunCleanCommand() from here instead of doing a kill of chrome.exe (which is
kind of arbitrary cleaning since it doesn't clean everything anyways)?

[sukolsak, 2013-09-13 21:39:22]

On 2013/09/13 21:16:09, gab wrote:
>
https://codereview.chromium.org/24126005/diff/6001/chrome/test/mini_installer...
> File chrome/test/mini_installer/test_installer.py (right):
> 
>
https://codereview.chromium.org/24126005/diff/6001/chrome/test/mini_installer...
> chrome/test/mini_installer/test_installer.py:208: subprocess.call('taskkill /f
> /im chrome.exe', shell=True)
> If the intention is to exit cleanly, would it make more sense to call
> _RunCleanCommand() from here instead of doing a kill of chrome.exe (which is
> kind of arbitrary cleaning since it doesn't clean everything anyways)?

Yes, it makes sense. I will do this in the _RunCleanCommand() CL. Note that it's
actually a bit different. RunCleanCommand() only kills the known processes. But
here we want to kill all child processes and their descendants (which might be
unknown) that were created during the tests. The immediate child processes are
guaranteed to exit because we use subprocess.call(), but their descendants might
outlive them. One way to keep track of them is to use a job object, but it will
cause problems for processes that also use job objects. So, hopefully all
processes that outlive their parents are killed in _RunCleanCommand().

[grt (UTC plus 2), 2013-09-14 02:30:09]

i'm not convinced that killing chromes, setups, etc, at the end of the test is
the right thing. there may be situations where the end of the test is never
reached (process crash), so i think it's better to invest in getting the machine
in the proper state before the tests run. furthermore, i can imaging an
interesting feature for the harness being the ability to tell it to run up to a
particular action and then quit. i think this will come in handy when we start
finding real test failures in late stages. it'd be nice for a dev to let the
framework get the machine into the state just before the failure and then
manually run the failing action (in a debugger, for example). if the test
ordinarily kills and cleans on exit, then you'll need to special-case this.

to address left-over setups and chromes, i think the right place to ensure that
there are no running chromes is in the quit_chrome_* actions, and the right
place to check for running setups is in the install_chrome_* and
uninstall_chrome_* actions. the gist here being that the action handlers should
be responsible for verifying that the processes they invoke complete and
terminate properly.

my $.02

[gab, 2013-09-16 14:40:15]

On 2013/09/14 02:30:09, grt wrote:
> i'm not convinced that killing chromes, setups, etc, at the end of the test is
> the right thing. there may be situations where the end of the test is never
> reached (process crash), so i think it's better to invest in getting the
machine
> in the proper state before the tests run. furthermore, i can imaging an
> interesting feature for the harness being the ability to tell it to run up to
a
> particular action and then quit. i think this will come in handy when we start
> finding real test failures in late stages. it'd be nice for a dev to let the
> framework get the machine into the state just before the failure and then
> manually run the failing action (in a debugger, for example). if the test
> ordinarily kills and cleans on exit, then you'll need to special-case this.
> 
> to address left-over setups and chromes, i think the right place to ensure
that
> there are no running chromes is in the quit_chrome_* actions, and the right
> place to check for running setups is in the install_chrome_* and
> uninstall_chrome_* actions. the gist here being that the action handlers
should
> be responsible for verifying that the processes they invoke complete and
> terminate properly.
> 
> my $.02

Interesting, I was more going towards cleaning both on exit and start.

The idea for cleaning on exit is to try to be as nice as possible to the next
test (or to the dev running this and not wanting his machine in a crazy state).
The idea for cleaning on start is that we have no choice since we can't rely
100% on the clean on exit...

I see how not cleaning on exit can be nice for debugging maybe, but not
necessarily; i.e., if the test fails to verify everything was uninstalled
properly and a dev needs to debug... he will need to re-run the test and
actually pause it at the state BEFORE the uninstall occurs (and then run the
uninstall in the debugger himself).

So I'm thinking it could be nice to have an option such as
--stop-on-state="chrome_installed" eventually, but I don't think this conflicts
with cleaning on TearDown.

[sukolsak, 2013-09-19 00:35:14]

On 2013/09/16 14:40:15, gab wrote:
> On 2013/09/14 02:30:09, grt wrote:
> > i'm not convinced that killing chromes, setups, etc, at the end of the test
is
> > the right thing. there may be situations where the end of the test is never
> > reached (process crash), so i think it's better to invest in getting the
> machine
> > in the proper state before the tests run. furthermore, i can imaging an
> > interesting feature for the harness being the ability to tell it to run up
to
> a
> > particular action and then quit. i think this will come in handy when we
start
> > finding real test failures in late stages. it'd be nice for a dev to let the
> > framework get the machine into the state just before the failure and then
> > manually run the failing action (in a debugger, for example). if the test
> > ordinarily kills and cleans on exit, then you'll need to special-case this.
> > 
> > to address left-over setups and chromes, i think the right place to ensure
> that
> > there are no running chromes is in the quit_chrome_* actions, and the right
> > place to check for running setups is in the install_chrome_* and
> > uninstall_chrome_* actions. the gist here being that the action handlers
> should
> > be responsible for verifying that the processes they invoke complete and
> > terminate properly.
> > 
> > my $.02
> 
> Interesting, I was more going towards cleaning both on exit and start.
> 
> The idea for cleaning on exit is to try to be as nice as possible to the next
> test (or to the dev running this and not wanting his machine in a crazy
state).
> The idea for cleaning on start is that we have no choice since we can't rely
> 100% on the clean on exit...
> 
> I see how not cleaning on exit can be nice for debugging maybe, but not
> necessarily; i.e., if the test fails to verify everything was uninstalled
> properly and a dev needs to debug... he will need to re-run the test and
> actually pause it at the state BEFORE the uninstall occurs (and then run the
> uninstall in the debugger himself).
> 
> So I'm thinking it could be nice to have an option such as
> --stop-on-state="chrome_installed" eventually, but I don't think this
conflicts
> with cleaning on TearDown.

I've removed taskkill from the latest submission. This CL has been rendered
unnecessary by https://codereview.chromium.org/23523045/ because we now kill
Chrome on exit. But it's still better to avoid the side effects of
subprocess.Popen, which include child processes inheriting handles from their
parent process.

[gab, 2013-09-19 18:22:10]

https://codereview.chromium.org/24126005/diff/17001/chrome/test/mini_installe...
File chrome/test/mini_installer/launch_chrome.py (right):

https://codereview.chromium.org/24126005/diff/17001/chrome/test/mini_installe...
chrome/test/mini_installer/launch_chrome.py:45: _, _, process_id, _ =
win32process.CreateProcess(None, chrome_path, None,
Add a comment here as to why we use CreateProcess.

Otherwise lgtm!

[sukolsak, 2013-09-19 20:23:03]

https://codereview.chromium.org/24126005/diff/17001/chrome/test/mini_installe...
File chrome/test/mini_installer/launch_chrome.py (right):

https://codereview.chromium.org/24126005/diff/17001/chrome/test/mini_installe...
chrome/test/mini_installer/launch_chrome.py:45: _, _, process_id, _ =
win32process.CreateProcess(None, chrome_path, None,
On 2013/09/19 18:22:11, gab wrote:
> Add a comment here as to why we use CreateProcess.
> 
> Otherwise lgtm!

Done. Thank you.

[commit-bot: I haz the power, 2013-09-19 20:25:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sukolsak@chromium.org/24126005/22001

[commit-bot: I haz the power, 2013-09-20 00:28:45]

Change committed as 224256