1067008 - [mozprocess] mozprocess.pid running_process should look at the entire command not just the executable name

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Daniel Y Lee]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140830210550

Steps to reproduce:

Currently, mozprocess.pid.runnnig_processes only looks at the executable names and tries to match it with the given argument

http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/pid.py#47

However, this will make it very difficult to use with the unit test. For instance, in our unit test, we look for a running instance of a python script proclaunch.py

>> python proclaunch.py

However, mozprocess.pid.runnnig_processes cannot find this process since proclaunch.py is NOT an executable name. "python" is. 



Expected results:

We should change the behavior so it returns a list of processes that have 'name' as a substring of the command. This is should be backward compatible as far as I know it.

Comment 1

[Daniel Y Lee]

proposed patch:

@@ -64,9 +68,8 @@
         if 'STAT' in process and not defunct:
             if process['STAT'] == 'Z+':
                 continue
-        prog = command[0]
-        basename = os.path.basename(prog)
-        if basename == name:
+        command = [os.path.basename(cmd) for cmd in command]
+        if name in command:
             retval.append((int(process['PID']), command))
     return retval

Comment 2

[Andrew Halberstadt [:ahal]]

Thanks for the patch Daniel! For the future, could you upload the actual patch file as a bugzilla attachment and check the "patch" box? For more details, see:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

This patch looks like a step in the right direction, but it assumes that every word in 'command' is a path. In reality it will be something like ['python', 'foo.py', '--bar']. Instead we should convert this list into a string (hint: use subprocess.list2cmdline) and then test for 'name in command'.

Does that make sense? Let me know if you have any questions.

Comment 3

[Daniel Y Lee]

Hi Andrew,

Thanks for the review! My reason for using os.path.basename(cmd) was that os.path.basename() is a "safe" function. For instance:

os.path.basename("--bar") = "--bar"

I liked how this approach would NOT construct what we have deconstructed few lines of code before. In addition, although unlikely, there are corner cases for which not calling "basename" may cause an issue. 

For instance, say we are looking to see if a process "foo" is running. It is not, but we do have the following process running:

["/user/foo/bar", "baz"]

Simply looking for a substring "foo" in our string "/user/foo/bar baz" will produce false positive. Highly unlikely, but it did came up on my mind.

Comments please!

Comment 4

[Andrew Halberstadt [:ahal]]

(In reply to Daniel Y Lee from comment #3)
> Thanks for the review! My reason for using os.path.basename(cmd) was that
> os.path.basename() is a "safe" function. For instance:
> 
> os.path.basename("--bar") = "--bar"

It's not completely safe, e.g:
os.path.basename('--url=http://foo.com/bar')

> For instance, say we are looking to see if a process "foo" is running. It is
> not, but we do have the following process running:
> 
> ["/user/foo/bar", "baz"]
> 
> Simply looking for a substring "foo" in our string "/user/foo/bar baz" will
> produce false positive. Highly unlikely, but it did came up on my mind.

You are right that there is a chance that this will cause false positives, but my opinion is that worrying about that too much might be being a little too cautious. My reasoning as follows..

Prior to this patch, the mozprocess tests would grep the output of ps, which just blindly does a substring match (e.g 'ps -ef | grep usr' returns many results). Furthermore, it looks like there is currently nothing using this 'get_pids' method. Look at [1], though there are many results, they are all defined somewhere else. So we don't have to worry too much about breaking things that currently depend on this edge case.

In the end, there are so many different edge cases that we could probably talk about the desired behaviour for days. Instead, let's just do a substring search :). It's good that you are thinking about things like this though, thanks!

Comment 5

[Andrew Halberstadt [:ahal]]

(forgot to post the link)

[1] http://dxr.mozilla.org/mozilla-central/search?q=get_pids&=mozilla-central&redirect=true

Comment 6

[Daniel Y Lee]

Hi Andrew,

Thanks for your awesome advices. I'll make appropriate changes and will try to submit a patch by end of today, if not tomorrow morning :)

Daniel

Comment 7

[Daniel Y Lee]

Attachment: ticket-1067008.diff

Took some time getting used to using hg. Very simple substring search! Looking forward to your comment. Then, I will move straight to https://bugzilla.mozilla.org/show_bug.cgi?id=705864

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8490461 [details] [diff] [review]
ticket-1067008.diff

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

Thank you, this is great! For the record, you can still use git if you want. You can make the patch whichever way you see fit (the article I linked was just for hg though). For git I like to use the 'git format-patch' command.

The next step here is to go to run the mozbase tests (if you haven't already) to make sure that nothing broke, see [1]. Once you see the tests are passing, go to the keywords field of this bug, and add the keyword 'checkin-needed'. That will tell our sheriffs that the patch is ready to land, and they will take care of it for you.

[1] https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Running_the_tests

Comment 9

[Daniel Y Lee]

Hi Andrew,

Thank you for your kinds instructions. Will go back and work on the origin bug now :)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42637c6037a9

Comment 11

[Daniel Y Lee]

Thank you! Should I consider this ticket closed?

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42637c6037a9