778267 - [mozprocess] mozprocess tests should be refactored

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

mozprocess{1,2}.py are in need of some love.  Some issues:

- there is some overlapping code, e.g. make_proclaunch:
https://github.com/mozilla/mozbase/blob/master/mozprocess/tests/mozprocess2.py#L22
  The comment at line
  https://github.com/mozilla/mozbase/blob/master/mozprocess/tests/mozprocess2.py#L18
  is obselete (we don't even use mutt to run the tests).

- check_for_processes is mostly a horrible function.  Even if we don't
  change the way this works on windows, we should change the way it
  works on linux.  We already have facilities in
  https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/pid.py
  to do this, and there are improved talos facilities in
  http://hg.mozilla.org/build/talos/file/3dfc185631ae/talos/utils.py
  for this exact thing.  To rehash a discussion about whether we ever
  *should* shell out to ps or not to determine process information, of
  course we shouldn't.  But the fact is we do shell out to ps to
  determine process information here and in talos and probably other
  places.  While this definitely isn't good practice, we want this for
  testing mozprocess and for checking if firefox is running for talos
  and possibly other places.  So we should at least consolidate this
  code and maintain it in a central place.

There is probably some other cleanup that is needed too, but this will
at least be a plus.

Comment 1

[Jeff Hammel]

Attachment: output of failure

In addition to needing refactor the tests don't work on all platforms

Comment 2

[Jeff Hammel]

We should really fix these tests or disable them by default.  They need to be rewritten. They don't work with clang.  We do not document what you actually need to run these tests, let alone check for it.  If we want these tests we should throw some time at them.  Disabling takes a few seconds.

Comment 3

[Jeff Hammel]

Looking at mozprocess1.py v mozprocess2.py, is there any reason these should be separate files or even separate classes?  I'm pretty much never in favor of arbitrary numbers/names as we have here.

I'll combine there unless there's a reason not to.

Comment 4

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #3)
> Looking at mozprocess1.py v mozprocess2.py, is there any reason these should
> be separate files or even separate classes?  I'm pretty much never in favor
> of arbitrary numbers/names as we have here.
> 
> I'll combine there unless there's a reason not to.

Done with bug 838075

Comment 5

[Jeff Hammel]

I'm broadening the scope (just) a bit and adding some deps; alternatively, if it is desired to create a meta bug to track this and other issues...that is fine, but IMHO this is the central issue.

Comment 6

[Jeff Hammel]

As best I can tell, this could be done by replacing the existing C + Make + whatever with python subprocesses

Comment 7

[Anhad Jai Singh (:ffledgling)]

Attachment: proclaunch.py

This script is supposed to be a python replacement of the proclaunch.c

Clint, Jeff could you please take a look at it and see if this is okay?

Comment 8

[Jeff Hammel]

Comment on attachment 788327 [details]
proclaunch.py

thanks, bugzilla

Comment 9

[Jeff Hammel]

Comment on attachment 788327 [details]
proclaunch.py

> import manifestparser 
> import ConfigParser 
> import argparse 
> import multiprocessing 
> import time

nit: alphabetize imports

>     while elapsed_time < maxtime:
>         time.sleep(1)
>         elapsed_time+=1

could make this variable instead of hardcoding 1

> exit(0)

Undefined; you need to import sys and sys.exit(0)

> def child(section, parsed_manifest):
>     print "Entering %s" % section
>     children = parsed_manifest.get(section, 'children') 

This function mixes parsing logic with execution logic.  In general...I'm not a fan.  I would try to make functions as close to a particular intention as possible; here, I would split parsing the manifest detailing children from their launching.  In the same way, please add a docstring detailing what the function does and why as well as the parameters to the function.  Here, I would, perhaps, split it into two functions: one to parse the children from the manifest and another to launch them with parameters.  I'd also consider making the brunt of this program a class.

>    print children
>    maxtime = parsed_manifest.getint(section, 'maxtime')
>    print maxtime

If you're printing debugging information, please print what you're printing:
print '%s:children: %s' % (section, children)
print '%s:maxtime: %s' % (section, maxtime)

> sectioned = False

Not sure if I understand the usecase of supporting sectioned and unsectioned.  Could you elaborate?  Also, document this please.

> children.split(',')

You repeat this twice.  Please don't; instead, do this once.  Also document the use of CSV for children.

> for c in children.split(','): 
>    if c in parsed_manifest.sections(): 
>        sectioned = True 
>        break

This seems magic.  What if some other of c are not in the manifest?

> idle(maxtime)

You repeat this twice.  Don't.

> for i in range(len(children)):

just `for i in children` please; either append after calling start or use the index -1 to get the last one.

> process_list.append(multiprocessing.Process(target=child, args=(children[i], parsed_manifest)))

parsed_manifest is an object; did you mean the file name?]

There should be a main function, def main() and a `if __name__ == '__main__':  main()` at the top level.

Please don't include debug comments in the final version.  Looks not too bad for a first stab, but will have to see a revision for good feedback.

Comment 10

[Anhad Jai Singh (:ffledgling)]

Attachment: classed_proclaunch_old-api.py

Hi Jeff, I've tried to fix the proclaunch.py as best I could based on your feedback, please take a look and tell me the bits that need changes.

The one bit I couldn't figure out was where you asked me to give the time increment as a variable. Can you explain that bit?

I've tried to keep the proclaunch.py as close to the original Clint wrote in terms of usage, so that all existing manifest files still work as expected.

Comment 11

[Jeff Hammel]

Comment on attachment 795086 [details]
classed_proclaunch_old-api.py

> #!/usr/bin/env python
> 
> import argparse
> import collections
> import ConfigParser
> import multiprocessing
> import time
> 
> ProcessNode = collections.namedtuple('ProcessNode', ['maxtime', 'children'])
> 
> class ProcessLauncher:
> 
>     """ Create and Launch process trees specified by a '.ini' file """
> 
>     # Children is a dictionary used to store information from the
>     # Configuration file in a more usable format.
>     # 
>     # Key : string contain the name of child process
>     # Value : A Named tuple of the form (max_time, [list of child processes of Key])
>     children = {}
> 
>     def __init__(self, manifest):
>         """
>         Parses the manifest and stores the information about the process tree
>         in a format usable by the class.
>         
>         Raises IOError if :
>             - The path does not exist
>             - The file cannot be read
>         Raises ConfigParser.*Error if:
>             - Files does not contain section headers
>             - File cannot be parsed because of incorrect specification
> 
>         :param manifest: Path to the manifest file that contains the
>         configuration for the process tree to be launched 
>         """
> 
>         cfgparser = ConfigParser.ConfigParser()
>         
>         if not cfgparser.read(manifest):
>             raise IOError('The manifest could not be found/opened')
>         
>         for section in cfgparser.sections():
>             m_time = cfgparser.get(section, 'maxtime')
>             children = cfgparser.get(section, 'children')

Should probably note via comment
that this will raise an error if maxtime or children is not
in each manifest section.

ABICT, it is confusing where nothing gets launched since all sections require maxtime 
and children specified and there is nothing saying what children can/should be.
In any case, I think this needs some thinking and documentation.

>             pn = ProcessNode(maxtime=m_time,
>                              children=[x.strip() for x in children.split(',')])
>             self.children[section] = pn
> 
>     def run(self):
>         """
>         When called, this function launches the process tree.
>         """

No need for the words 'When called'

>         self._run('main')
> 
>     def _run(self, proc_name):
>         """
>         Runs the process specified by the name `proc_name` in the manifest file.

'name' == 'section name' ?

>         Then makes calls to launch the child processes of `proc_name`
> 
>         :param proc_name: File name of the manifest as a string.
>         """
>         if proc_name in self.children.keys():

This can be much simpler and avoid an indented block:

         if proc_name not in self.children:
             raise IOError(...)

The rest may be deindented one level.

> 
>             maxtime = int(self.children[proc_name].maxtime)

Why int and not float?

>             print "Launching %s for %d" % (proc_name, maxtime)

Should display the time units (seconds).

>             self._launch(maxtime)
> 
>             while self.children[proc_name].children:
>                 child = self.children[proc_name].children.pop()
>                 print "Child: %s" % (child)
>                 try:
>                     # If this is an integer, launch `child` number of processes
>                     # with the max-time inherited from the current process
>                     for i in range(int(child)):
>                         self._launch(maxtime)
>                     continue

I'm not 100% for overloading children for this purpose; i'd probably have another key
(e.g. `processes = 2` which cannot be used with children), but if we do switch on whether children is an int, this should be made very clear.

>                 except ValueError:
>                     # This means it is not an Integer, we assume it is a child
>                     # with it's own section
>                     pass
>                 self._run(child)
>         else:
>             raise IOError("%s is not a valid process" % (proc_name))
> 
>     def _launch(self, running_time):
>         """
>         Create and launch a process and idles for the time specified by
>         `running_time`
> 
>         :param running_time: Running time of the process in seconds.
>         """
>         print "Process called with %s" % (running_time)
> 
>         def _idle(running_time):
>             UNIT_TIME = 1
>             elapsed_time = 0
> 
>             while elapsed_time < running_time:
>                 time.sleep(UNIT_TIME)
>                 elapsed_time += 1

elapsed_time should be += UNIT_TIME, I believe

>         p = multiprocessing.Process(target=_idle, args=(running_time,))
>         p.start()
> 
> 
> 
> if __name__ == '__main__':
> 
>     parser = argparse.ArgumentParser()
>     parser.add_argument("manifest", help="Specify the configuration .ini file")
>     args = parser.parse_args()
> 
>     proclaunch = ProcessLauncher(args.manifest)
>     proclaunch.run()

Please include an .ini file and documentation in the python file somewhere as a 
docstring as to the form of the .ini file.  Ideally, the command line parser
would print the format as help, etc.

A good start, but needs a bit of work.

Comment 12

[Anhad Jai Singh (:ffledgling)]

(In reply to Jeff Hammel [:jhammel] from comment #11)

> I'm not 100% for overloading children for this purpose; i'd probably have
> another key
> (e.g. `processes = 2` which cannot be used with children), but if we do
> switch on whether children is an int, this should be made very clear.

I agree, overloading children is actually not a very good idea, infact I think if possible we should try to change the behavior where an integer as a child is interpreted as the number of children, and the children inherit the max_time of their parents.

We should either entirely remove integers as children, allowing only "named" children with a section in the .ini file, or if we really do want to keep integers, change the interpretation to an integer representing an "un-named" child running for the time give by the integer. so that children=c1, 10 will now run c1, and an un-named child for 10 units of time.

The current file is trying to maintain consistency in behaviour with the existing proclaunch.c (hence it's called classed_proclaunch_old-api.py).

I vote for removing integers (or their current interpretation) from children entirely, so that we treat every comma separated value in the children field as a string which should correspond to a section in the .ini file.
This might increase the amount we have to write in the .ini file, especially in case of more complex process trees, for eg.
children=c1, 7
will change to
children=c1, c2, c2, c2, c2, c2, c2, c2
but this will increase readability and be more straightforward interpretation.

Jeff, what are your thoughts on this?

> Please include an .ini file and documentation in the python file somewhere
> as a 
> docstring as to the form of the .ini file.  Ideally, the command line parser
> would print the format as help, etc.

I'll be able to do this once we've decided on the format/api/interpreation we want to use for the .ini file.

Comment 13

[Jeff Hammel]

I would advocate removing as much overloading as possible, either going with the model you suggest at last: `children=c1, c2, c2, c2, c2, c2, c2, c2` or adding an additional non-overloaded parameter to indicate how many processes to launch for a child, e.g.

[foo]
maxtime = 25
children = bar

[bar]
maxtime = 10
nprocesses = 2

(If bar had children as a key, each of the 2 processes would have those children).

If one wanted to get really fancy:

[foo]
maxtime = 25
children = 2*bar, 2*fleem

But that's probably overkill.

I would not have children inherit maxtime; a child could actually have multiple parents, so it becomes ambiguous.

And of course the case where children is missing just means the process has no child processes.

Comment 14

[Anhad Jai Singh (:ffledgling)]

Attachment: classed_proclaunch_new-api.py

This is an implementation of the new api, the 2*bar, 2*fleem version.

Jeff, please take a look and tell me if more changes are needed, I've added some documentation and will add more once the code is okay with you.

Comment 15

[Jeff Hammel]

Comment on attachment 797642 [details]
classed_proclaunch_new-api.py

Overall...this looks great!  f+ for sure.  Everything I have beyond that are nits.  I'm glad you chose the notation you did; overkill, perhaps, but pretty simple to parse and extensible in the case we get more mozprocess tests, which we want.

Nit:
    """ Create and Launch process trees specified by a '.ini' file """

    # Children is a dictionary used to store information from the,
    # Configuration file in a more usable format.
    # Key : string contain the name of child process
    # Value : A Named tuple of the form (max_time, (list of child processes of Key))
    #   Where each child process is a list of type: [count to run, name of child] 
    children = {}

    # Unit time for processes in seconds
    UNIT_TIME = 1
    """ Typical .ini file accepted by this class :

I would include the .ini snippet as part of the class's docstring.  I would not split across the comments -- not sure if that gets included in __doc__, but in any case...not really necessary.  The documentation itself, however, is very nice and thorough.

                children = [[y.strip() for y in x.strip().split('*')]

This should probably be split('*', 1)
Also, for the sake of sanity, I would prohibit '*' and ',' from being in the child (section) names.

                            for x in cfgparser.get(section, 'children').split(',')]
            except ConfigParser.NoOptionError:

I would also make children an instance variable instead of a class variable. (Also, classes should inherit from object).

        if proc_name not in self.children.keys():
            raise IOError("%s is not a valid process" % (proc_name))

It'd be nice if this sanity check was earlier. Generally, the earlier the better for sanity.  In this case I'd do all of the parsing validation in __init__ ; you could also validate that the number is an integer. Also, s/self.children.keys()/self.children/ ,  and no need for the ()s around proc_name in string interpolation.

        maxtime = int(self.children[proc_name].maxtime)

Again, I'd do this in __init__

        print " "*level+"Launching %s for %d*%d seconds" % (proc_name, maxtime, self.UNIT_TIME)

Please don't mix the + operation and string interpolation operators this way.  Just let " "*level be the first interpolation parameter: "%sLaunching %s..." % (" "*level, proc_name....

        print " "*level+"Finished %s" % (proc_name)

And again.

Other than that, it looks great

Comment 16

[William Lachance (:wlach)]

Clint asked me to take a look at this bug (since he was NEEDINFO'd) so I did. I approve of both ffledging's implementation and jhammel's comments. Assuming ffledging can provide an actual patch with the last comments addressed, I don't see much that needs to happen before we get this landed.

Comment 17

[Anhad Jai Singh (:ffledgling)]

Attachment: Patch for proclaunch

So I was actually hoping to turn this in with the actual tests and the other changes required, but I'm putting up this patch for proclaunch.py so that it doesn't bit-rot. Please note that this proclaunch.py is not compatible with the existing .ini files, since we change the way we interpret the .ini files.

Comment 18

[Jeff Hammel]

Comment on attachment 801803 [details] [diff] [review]
Patch for proclaunch

Review of attachment 801803 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! r+ with the corrections I've noted.

::: mozprocess/tests/proclaunch.py
@@ +97,5 @@
> +            m_time = cfgparser.get(section, 'maxtime')
> +            try:
> +                m_time = int(m_time)
> +            except ValueError:
> +                raise ValueError('Expected maxtime to be an integer, specified %s' % (m_time))

No need for the ()s around m_time here

@@ +107,5 @@
> +                            for x in cfgparser.get(section, 'children').split(',')]
> +                try:
> +                    for i in range(len(children)):
> +                        # No multiplicate factor infront of a process implies 1
> +                        if len(children[i]) is 1:

This should almost assuredly be ==, not `is`

@@ +110,5 @@
> +                        # No multiplicate factor infront of a process implies 1
> +                        if len(children[i]) is 1:
> +                            children[i] = [1, children[i][0]]
> +                        else:
> +                            children[i][0] = int(children[i][0])

I think there is also the case of e.g. 

[mychild]
children = 3*i5*,2*i7

So I think you should probably check for len(children) == 0 in addition to == 1.

I think you'll also need to ensure that children are not the empty string.

You'll definately want to test if the children are in parser.sections(). You do this below but IMHO its probably better to do this in __init__ vs at run time.

Comment 19

[Anhad Jai Singh (:ffledgling)]

Attachment: 0001-Bug-778267-Adding-Proclaunch-python-version-Added-an.patch

This patch addresses the nits pointed out in the previous patch. Adds 2 tests for deep and shallow trees and ports all other tests to use the python proclaunch.
The old files were not modified instead, new files with a "_python" ending were created, so that the existing tests can be run with proclaunch.c and proclaunch.py simultaneously.

Would love any feedback/comments on this Jeff. Thanks!

Comment 20

[Jeff Hammel]

Comment on attachment 804723 [details] [diff] [review]
0001-Bug-778267-Adding-Proclaunch-python-version-Added-an.patch

Review of attachment 804723 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, provided, of course, that they pass ;).  But make sure you correct the "python" -> sys.executable prior to checkin! (I'm tempted to r- for that one thing, just because its so important, but instead I will just note it very loudly.)

Thanks for the patch!

::: mozprocess/tests/process_normal_broad.ini
@@ +2,5 @@
> +; This generates a Tree of the form:
> +;
> +;   main
> +;    \_ c1
> +;    |   \_ c2 

nit: please remove trailing whitespace

::: mozprocess/tests/proclaunch.py
@@ +112,5 @@
> +
> +                children = [[y.strip() for y in x.strip().split('*', 1)]
> +                            for x in c.split(',')]
> +                try:
> +                    for i in range(len(children)):

Probably better to do

`for i, child in enumerate(children):`

@@ +116,5 @@
> +                    for i in range(len(children)):
> +                        # No multiplicate factor infront of a process implies 1
> +                        if len(children[i]) == 1:
> +                            children[i] = [1, children[i][0]]
> +                        elif len(children[i]) == 0:

`elif not children[i]` (or `child` with the above enumerate)

@@ +117,5 @@
> +                        # No multiplicate factor infront of a process implies 1
> +                        if len(children[i]) == 1:
> +                            children[i] = [1, children[i][0]]
> +                        elif len(children[i]) == 0:
> +                            raise ConfigParser.ParsingError('Incorrect children field value for section %s' % section)

I'd also tend to allow this, with the same result as omitting the children parameter entirely.

::: mozprocess/tests/test_mozprocess_python.py
@@ +47,5 @@
> +            elif processName in line and not 'defunct' in line:
> +                detected = True
> +                break
> +
> +    return detected, output

It's questionable whether to keep this for parity or just replace.  I've decided parity is fine...but replacing would be better.

@@ +61,5 @@
> +
> +    def test_process_normal_finish(self):
> +        """Process is started, runs to completion while we wait for it"""
> +
> +        p = processhandler.ProcessHandler(["python", self.proclaunch, "process_normal_finish_python.ini"],

please use sys.executable here

@@ +75,5 @@
> +
> +    def test_process_wait(self):
> +        """Process is started runs to completion while we wait indefinitely"""
> +
> +        p = processhandler.ProcessHandler(["python", self.proclaunch,

likewise

@@ +92,5 @@
> +    def test_process_timeout(self):
> +        """ Process is started, runs but we time out waiting on it
> +            to complete
> +        """
> +        p = processhandler.ProcessHandler(["python", self.proclaunch, "process_waittimeout_python.ini"],

likewise

@@ +144,5 @@
> +    def test_process_kill(self):
> +        """Process is started, we kill it"""
> +
> +        p = processhandler.ProcessHandler(["python", self.proclaunch, "process_normal_finish_python.ini"],
> +                                          cwd=here)

likewise.  also, is setting cwd required?

Comment 21

[Anhad Jai Singh (:ffledgling)]

(In reply to Jeff Hammel [:jhammel] from comment #20)
> This looks good, provided, of course, that they pass ;).
I've run them  on linux and they do.
I'll also try testing them on the other platforms on try if possible.

> But make sure you correct the "python" -> >sys.executable prior to checkin!
I can't believe I didn't know about sys.executable! I never came across it before!
Fixed patch accordingly.

> It's questionable whether to keep this for parity or just replace.  I've
> decided parity is fine...but replacing would be better.
Can you explain what you mean by replace here? If you mean that I should remove the redundant "output" value, I'm fine with it, but it's being used by `def determine_status` as debug output in case a test fails, so it might be useful in such a scenario.

> likewise.  also, is setting cwd required?
The cwd is required when tests are run from outside the current directly ... which I think they are ... it helps the test_mozprocess_python.py file specify the path to the manifests correctly, else it tries looking for the manifests in the directory from which it was launched, rather than where the actual manifests are present. 

I've fixed all the other nits, upload a patch with those addressed. :)

Comment 22

[Anhad Jai Singh (:ffledgling)]

Attachment: 0002-Bug-778267-Adding-Proclaunch-python-version-Added-an.patch

Updated patch. Jeff, please take a look and tell me if this is any better, thanks!

Comment 23

[Jeff Hammel]

Comment on attachment 806596 [details] [diff] [review]
0002-Bug-778267-Adding-Proclaunch-python-version-Added-an.patch

Review of attachment 806596 [details] [diff] [review]:
-----------------------------------------------------------------

looks better.  thanks!

Comment 24

[cmtalbert]

This patch and the tests look good to me. This looks great. Thanks a bunch for all this work, Anhad! Sorry it took me so long to get back to you on this one.

Comment 25

[Anhad Jai Singh (:ffledgling)]

Results from the try :- https://tbpl.mozilla.org/?tree=Try&rev=3bef36150f3a

All kill_process tests on Windows are failing with a non-zero error code exception. The code being returned is 572, which is the code for an application terminated via ^C (Ctrl-C) [ref: http://msdn.microsoft.com/en-us/library/windows/desktop/ms681388(v=vs.85).aspx]. I don't know why I'm getting this error.

The deep_wait test fails on both linux and Mac, I will investigate to check if this is a real failure or a problem in the tests.

I'll also look at the windows error and try to reproduce it, but any info someone might have on that front would be great.

Comment 26

[Anhad Jai Singh (:ffledgling)]

So after a bit of investigation I've been able to solve the errors for linux and Mac (nothing more than a stray initialization of a variable).

I've also determined the cause for the "AssertionError: Detected non-zero return code of: 572" error that we get on windows.
(We seem to have faced the exact same error in the past https://bugzilla.mozilla.org/show_bug.cgi?id=790765#c54 )

The exit code of 572 (0x23c) for windows means termination by a CONTROL_C (^C) termination ... which is to be expected since that is what we actually do in processhandler.py[1] .

Python documentation suggests using Popen.terminate[2]  , but since we utilize winprocess.py's TerminateJobObject(), which in turn uses Windows' underlying TerminateJobObject[3], some changes probably need to be made to the values being passed into it. I tried fiddling with the values a bit, but without much success.

I'll continue looking into this, but it would be great if someone with windows expertise could chip in with some advice.

Till this issue is resolved, I'll take Jeff's suggestion and upload a patch with the fixed tests for Linux and Mac, and with Windows tests disabled.



[1] https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/processhandler.py#L112-L127
[2] http://docs.python.org/2/library/subprocess.html#subprocess.Popen.terminate
[3] http://msdn.microsoft.com/en-us/library/windows/desktop/ms686709(v=vs.85).aspx

Related links:
* http://msdn.microsoft.com/en-us/library/windows/desktop/ms686722(v=vs.85).aspx
* http://msdn.microsoft.com/en-us/library/windows/desktop/ms686714(v=vs.85).aspx
* http://msdn.microsoft.com/en-us/library/windows/desktop/ms681382(v=vs.85).aspx
* http://msdn.microsoft.com/en-us/library/windows/desktop/ms681388(v=vs.85).aspx

Comment 27

[Anhad Jai Singh (:ffledgling)]

Attachment: 0004-Bug-778267-adds-proclaunch.py-and-mozprocess-tests-r.patch

This is the patch with the fix for the failing deep_wait tests on linux and OSX.
I've disabled the tests on windows for now.

I'm carrying over the r+ from last time, since there are no major changes to the patch; Jeff it would still be great if you could take a look and see if something requires fixing.

Here's the relevant try run - https://tbpl.mozilla.org/?tree=Try&rev=8941791eba9f

The tests pass on the *nix platforms but there are failures on windows.
I managed to reduce the number of failures from the last try run, by modifying the value being passed to winprocess.TerminateJobObject() in processhandler.py[1].

Passing 0x0 instead of winprocess.ERROR_CONTROL_C_EXIT (0x23c) seems to have done the trick since that is the return value we expect, but I have *not* included this change as part of the above patch, since I'm not completely sure if this is the what needs to be done. 

I think the windows bit might need it's own bug, since it seems to be a problem in the processhandler.py code, and other people also seem to be encountering mozprocess bugs on windows similar to the ones we're seeing.
Noting documented cases here:
1.https://bugzilla.mozilla.org/show_bug.cgi?id=795670#c9 (managed to repro this locally) 
2. https://bugzilla.mozilla.org/show_bug.cgi?id=790765#c54 ( repro'd on try, see https://tbpl.mozilla.org/php/getParsedLog.php?id=28201740&tree=Try )




[1]https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/processhandler.py#L112-L127

Comment 28

[Jeff Hammel]

Honestly, I don't know if I'm qualified to speak what to do here.  I don't know why things are done the way that they're done and I don't know enough about windows to speculate.  In absence of that, I would recommend filing a bug (if such a bug doesn't exist) and skipping the test on windows per that bug

Comment 29

[Anhad Jai Singh (:ffledgling)]

Follow up on the Windows tests https://bugzilla.mozilla.org/show_bug.cgi?id=892902

Comment 30

[Jeff Hammel]

pushed: https://github.com/mozilla/mozbase/commit/a49d775725c4bd5a97492f1728428bdac62aea7f

Comment 31

[Jeff Hammel]

It looks like there's an intermittent.  From https://travis-ci.org/mozilla/mozbase/builds/11881700 :

======================================================================

FAIL: test_process_kill_broad_wait (test_mozprocess_python.ProcTest)

Process is started, we use a broad process tree, we let it spawn

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/build/mozilla/mozbase/mozprocess/tests/test_mozprocess_python.py", line 201, in test_process_kill_broad_wait

p.didTimeout)

File "/home/travis/build/mozilla/mozbase/mozprocess/tests/test_mozprocess_python.py", line 271, in determine_status

self.assertTrue(not detected, "Detected process is still running, process output: %s" % output)

AssertionError: Detected process is still running, process output: travis 1067 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1068 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1069 1067 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1071 1068 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1072 1067 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1073 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1074 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1075 1067 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1076 1068 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1078 1068 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1079 1067 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1081 1073 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1082 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1084 1068 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1085 1067 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1086 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1089 1068 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1090 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1091 1082 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1092 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1097 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1098 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1099 1073 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1100 1092 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1102 1082 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1104 1092 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1109 1097 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1110 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1112 1103 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1116 1097 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1123 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1124 1097 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1125 1103 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1126 1115 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1127 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1132 1115 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1139 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1143 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1144 1127 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1149 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1151 1127 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1160 1127 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1164 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1168 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1179 1168 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1181 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1188 1168 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1189 1181 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1210 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1211 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1212 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1213 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1214 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1215 1 0 20:17 pts/2 00:00:00 /home/travis/virtualenv/python2.7/bin/python /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py process_normal_broad_python.ini

travis 1218 919 0 20:17 pts/2 00:00:00 grep /home/travis/build/mozilla/mozbase/mozprocess/tests/proclaunch.py

----------------------------------------------------------------------

Ran 231 tests in 88.675s

FAILED (failures=1, skipped=2)

The command "python test.py" exited with 1.

Comment 32

[Jeff Hammel]

Filed bug 921632 re comment 31