733826 - global ffprocess shouldnt check for dwwim

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

http://www.file.net/process/dwwin.exe.html

dwwim is a windows utility. But we dont check for it in
ffprocess_win.py but instead in ffprocess.py:

http://hg.mozilla.org/build/talos/file/51075a756118/talos/ffprocess.py#l71

We also code the list of processes in several places.

Instead, each platform-dependent class should have a list of processes
to monitor coded in a single place instead of copied all over the
code.  Only the windows class should care about dwwim.

Comment 1

[jdev005]

Attachment: any changes required ?

Comment 2

[Jeff Hammel]

Comment on attachment 654650 [details] [diff] [review]
any changes required ?

So this doesn't really suffice, as we will still need these methods to be called from e.g. ttest.py

What we should do is make a list in the ffprocess class of "extra programs" that should be killed, that contains crashreporter (and possibly whatever talkback is, damned if i know).

Then ffprocess_win32 can extend this list, adding 'dwwim'

It appears that checkBrowserAlive isn't used at all and can be gotten rid of:

(talos)│grep 'checkBrowserAlive' *.py
ffprocess.py:    def checkBrowserAlive(self, process_name):

Comment 3

[jdev005]

Attachment: changes made

Comment 4

[Jeff Hammel]

Comment on attachment 657882 [details] [diff] [review]
changes made

Close, but you'll need to change the signature for ProcessesWithNames:

+    def ProcessesWithNames(self,extra,*process_names):

There's no reason to do this since the logic is the same for extra and each of the process_names.  Removing this will also allow you to have have to add this code:

+
+        for name in extra :
+            pids = self.GetPidsByName(name)
+            if len(pids) > 0:
+                processes_with_names.extend([(pid, name) for pid in pids])
+

+        return self.ProcessesWithNames(extra_prog,process_name, child_process)
should be self.extra_prog.  You'll have to build this up a bit differently to avoid changing the method signature:

return self.ProcessesWithNames(*([process_name, child_process] + self.extra_prog))

(Likewise elsewhere in the code.)

+    self.extra_prog.append('dwwim')

This won't quite work.  For one, you're at the class level so 'self' isn't defined.

class Win32Process(FFProcess):
 
    extra_prog = FFProcess.extra_prog[:] + ['dwwim']

Giving an r- for now but this is really close.

Comment 5

[jdev005]

Attachment: done :)

Comment 6

[Jeff Hammel]

Comment on attachment 658143 [details] [diff] [review]
done :)

Looks great!  Thanks!

Comment 7

[Jeff Hammel]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=18d42afa3046

will land upon success

Comment 8

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #7)
> pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=18d42afa3046
> 
> will land upon success

Looks green!

Comment 9

[Mozilla RelEng Bot]

Try run for 18d42afa3046 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=18d42afa3046
Results (out of 73 total builds):
    success: 67
    failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-18d42afa3046

Comment 10

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/1c5976f92643

Thanks for the patch!