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)

x86_64
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zcampbell, Unassigned)

Details

Attachments

(2 files)

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.
Attached patch bug_1075487.diffSplinter Review
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 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+
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 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+
(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)
btw I had set leave-open with the intention of keeping the enhancement Dave suggested as a good first bug.
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 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 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 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)
There's no longer a reason to keep this open.
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: leave-open
Resolution: --- → FIXED
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.