Closed
Bug 1075487
Opened 10 years ago
Closed 10 years ago
get_b2g_pid is unreliable because of multiple processes running.
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: zcampbell, Unassigned)
Details
Attachments
(2 files)
1.38 KB,
patch
|
davehunt
:
review+
mdas
:
review+
|
Details | Diff | Splinter Review |
999 bytes,
patch
|
jgriffin
:
review+
davehunt
:
feedback+
|
Details | Diff | Splinter Review |
On b2g now apps run in multiple processes: APPLICATION SEC USER PID PPID VSIZE RSS WCHAN PC NAME b2g 0 root 2816 1 223760 86760 ffffffff b6f49894 S /system/b2g/b2g (Nuwa) 0 root 2835 2816 71844 9888 ffffffff b6f49894 S /system/b2g/b2g Homescreen 2 u0_a2949 2949 2835 136556 32988 ffffffff b6f49894 S /system/b2g/b2g Find My Device 2 u0_a3340 3340 2835 81912 20932 ffffffff b6f49894 S /system/b2g/b2g Smart Collectio 2 u0_a3356 3356 2816 90660 32060 ffffffff b6e78894 S /system/b2g/plugin-container (Preallocated a 2 u0_a3570 3570 2835 79340 17844 ffffffff b6f49894 S /system/b2g/b2g The if logic inside this method is now buggy. http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/mixins/b2g.py#28 This is causing some failures when b2g crashes so the if statement resolves true but the regular expression does not. 00:13:26.831 File "/var/jenkins/2/workspace/flame-kk-319.b2g-inbound.ui.functional.smoke/.env/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/runner/mixins/b2g.py", line 33, in get_b2g_pid 00:13:26.832 return pid.group(1) 00:13:26.832 AttributeError: 'NoneType' object has no attribute 'group Perhaps the if statement should be replaced with simple the regular expression match.
Reporter | ||
Comment 1•10 years ago
|
||
This picks the correct pid and also will return nothing if the original b2g process has crashed and left other processes running.
Attachment #8500989 -
Flags: review?(dave.hunt)
Comment 2•10 years ago
|
||
Comment on attachment 8500989 [details] [diff] [review] bug_1075487.diff Review of attachment 8500989 [details] [diff] [review]: ----------------------------------------------------------------- I'm okay with this but I think it would be more readable if we avoided the regular expression entirely. Requesting additional review from Malini for her thoughts. Also, your commit message needs fixing. :) ::: testing/marionette/client/marionette/runner/mixins/b2g.py @@ +28,5 @@ > def get_b2g_pid(dm): > b2g_output = dm.shellCheckOutput(['b2g-ps']) > + pid_re = re.compile(r"""b2g[\s]+0[\s]+root\s+([\d]+).*/system/b2g/b2g""") > + pid = pid_re.match(b2g_output) > + if pid: Rather than a regular expression, could we split the output on newlines and whitespace and check the first (application) and last (name)? It would make the code much more readable.
Attachment #8500989 -
Flags: review?(mdas)
Attachment #8500989 -
Flags: review?(dave.hunt)
Attachment #8500989 -
Flags: review+
Reporter | ||
Comment 3•10 years ago
|
||
I'd be OK with changing it too. I modified the regex simply to follow the existing style but let's see what mdas says.
Comment 4•10 years ago
|
||
Comment on attachment 8500989 [details] [diff] [review] bug_1075487.diff Review of attachment 8500989 [details] [diff] [review]: ----------------------------------------------------------------- I'm happy with getting rid of the regex, but this is good in the meantime as well. Thanks!
Attachment #8500989 -
Flags: review?(mdas) → review+
Reporter | ||
Comment 5•10 years ago
|
||
Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c28659e3d0ab
Keywords: leave-open
Reporter | ||
Comment 6•10 years ago
|
||
(In reply to Zac C (:zac) from comment #5) > Try: > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c28659e3d0ab Malini, the one failure in there seems to be related to the other bug we fixed. Can this be checked in now?
Flags: needinfo?(mdas)
Reporter | ||
Comment 7•10 years ago
|
||
btw I had set leave-open with the intention of keeping the enhancement Dave suggested as a good first bug.
Comment 8•10 years ago
|
||
landed https://hg.mozilla.org/integration/mozilla-inbound/rev/788ff9d05938
Flags: needinfo?(mdas)
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/788ff9d05938
Comment 10•10 years ago
|
||
I was going to reopen this, but it seems it was never closed! :) This patch doesn't work on my recent build. I'm using the most recent flame build with this output: b2g 0 root 1697 1 208968 81972 ffffffff b6f0d67c S /system/b2g/b2g (Nuwa) 0 root 1701 1697 72136 13960 ffffffff b6f0d67c S /system/b2g/b2g Homescreen 2 u0_a1790 1790 1701 144664 35408 ffffffff b6f0d67c S /system/b2g/b2g Built-in Keyboa 2 u0_a1938 1938 1697 101160 30432 ffffffff b6f2f67c S /system/b2g/plugin-container (Preallocated a 2 u0_a2051 2051 1701 80552 19764 ffffffff b6f0d67c S /system/b2g/b2g and it doesn't find the pid, and so I don't get any b2g related output on failure :( I'm uploading a patch that does string comparison after splitting instead of fun regexes.
Attachment #8508454 -
Flags: review?(jgriffin)
Attachment #8508454 -
Flags: feedback?(zcampbell)
Attachment #8508454 -
Flags: feedback?(dave.hunt)
Comment 11•10 years ago
|
||
Comment on attachment 8508454 [details] [diff] [review] find pid based on string splitting Review of attachment 8508454 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me!
Attachment #8508454 -
Flags: feedback?(dave.hunt) → feedback+
Comment 12•10 years ago
|
||
Comment on attachment 8508454 [details] [diff] [review] find pid based on string splitting Review of attachment 8508454 [details] [diff] [review]: ----------------------------------------------------------------- Hopefully the formatting of 'b2g-ps' won't change with future versions!
Attachment #8508454 -
Flags: review?(jgriffin) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8508454 [details] [diff] [review] find pid based on string splitting Review of attachment 8508454 [details] [diff] [review]: ----------------------------------------------------------------- clearing flag, zac says its fine
Attachment #8508454 -
Flags: feedback?(zcampbell)
Comment 14•10 years ago
|
||
pushed to m-i https://hg.mozilla.org/integration/mozilla-inbound/rev/8e47c6c73995
Comment 16•10 years ago
|
||
There's no longer a reason to keep this open.
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•