529137 - Update Talos to cleanup child processes

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

Currently, Talos cleans up browser-related process before and after a test.  For e10s, this code needs to be updated to allow it to cleanup any orphaned child processes as well.

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: [checked in]patch

This patch causes Talos process management routines to be aware of the e10s child process ("mozilla-runtime") when cleaning up processes, and checking if any processes are running.  It defaults to "mozilla-runtime" but can be overridden by adding a "child_process" setting to a .config file.

This patch does not update some dead code, e.g., RunProcessAndWaitForOutput() in ffprocess.py.

Comment 2

[alice nodelman [:alice] [:anode]]

What platforms has this been tested on?

Comment 3

[Jonathan Griffin (:jgriffin)]

I've tested this on Windows 7 and Ubuntu 9.04; do you need it tested on other platforms?

Comment 4

[alice nodelman [:alice] [:anode]]

We also run on mac 10.4/10.5 and windowsxp/vista.  I think that you probably have windows pretty decently covered, but a mac run would increase confidence.

Comment 5

[Jonathan Griffin (:jgriffin)]

Electrolysis is currently not supported on the Mac, but I will test it on the Mac to make sure I haven't broken anything there.

Comment 6

[Jonathan Griffin (:jgriffin)]

I tested this on 10.6, which is the only mac OS I have easy access to, and it worked fine.

Comment 7

[Benjamin Smedberg]

ping? I want to turn on OOPP by default on the electrolysis tree this week, but I don't want to kill the Talos slave pool.

Comment 8

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #7)
> ping? I want to turn on OOPP by default on the electrolysis tree this week, but
> I don't want to kill the Talos slave pool.

bsmedberg: 1) what testing is needed to verify that OOPP-on-by-default is Talos-safe ? and 2) who's doing that?

Comment 9

[Benjamin Smedberg]

jgriffin wrote the patch and tested it, as the many comments above attest, I think.

Comment 10

[alice nodelman [:alice] [:anode]]

Comment on attachment 412733 [details] [diff] [review]
[checked in]patch

Checking in bcontroller.py;
/cvsroot/mozilla/testing/performance/talos/bcontroller.py,v  <--  bcontroller.py
new revision: 1.7; previous revision: 1.6
done
Checking in ffprocess.py;
/cvsroot/mozilla/testing/performance/talos/ffprocess.py,v  <--  ffprocess.py
new revision: 1.15; previous revision: 1.14
done
Checking in run_tests.py;
/cvsroot/mozilla/testing/performance/talos/run_tests.py,v  <--  run_tests.py
new revision: 1.57; previous revision: 1.56
done
Checking in ttest.py;
/cvsroot/mozilla/testing/performance/talos/ttest.py,v  <--  ttest.py
new revision: 1.39; previous revision: 1.38
done

Comment 11

[Benjamin Smedberg]

Back to alice for downtime/deployment, right? Can we get this done soon?

Comment 12

[alice nodelman [:alice] [:anode]]

Completed by comment #10, I didn't close the bug when it landed as it wasn't assigned to me.

Comment 13

[Benjamin Smedberg]

Hey thanks! I didn't realize that landing into CVS was the same as deployment.