862253 - [mozrunner] Error Message for a wrong binary is not helpful

Status: RESOLVED WORKSFORME

(Testing :: Mozbase, defect)

Description

[Johannes Vogel (:nebelhom)]

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

Steps to reproduce:

mozmill -b 'path/to/bad/binary' -m mutt/mutt/tests/all-tests.ini


Actual results:

['firefox-latest.en-US.linux64.tar.bz2', '-profile', '/tmp/tmpc4V2f0.mozrunner']
TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed
RESULTS | Passed: 0
RESULTS | Failed: 0
RESULTS | Skipped: 0
Traceback (most recent call last):
  File "/home/nebelhom/Mozilla-Dev/mozmill/mozmill/mozmill/__init__.py", line 755, in run
    mozmill.run(tests, self.options.restart)
  File "/home/nebelhom/Mozilla-Dev/mozmill/mozmill/mozmill/__init__.py", line 419, in run
    self.stop()
  File "/home/nebelhom/Mozilla-Dev/mozmill/mozmill/mozmill/__init__.py", line 522, in stop
    self.runner.cleanup()
  File "/home/nebelhom/Mozilla-Dev/mozmill/venv/local/lib/python2.7/site-packages/mozrunner-5.10-py2.7.egg/mozrunner/runner.py", line 224, in cleanup
    self.stop()
  File "/home/nebelhom/Mozilla-Dev/mozmill/venv/local/lib/python2.7/site-packages/mozrunner-5.10-py2.7.egg/mozrunner/runner.py", line 213, in stop
    self.process_handler.kill()
  File "/home/nebelhom/Mozilla-Dev/mozmill/venv/local/lib/python2.7/site-packages/mozprocess-0.5-py2.7.egg/mozprocess/processhandler.py", line 619, in kill
    return self.proc.kill()
AttributeError: 'ProcessHandler' object has no attribute 'proc'
Exception AttributeError: "'ProcessHandler' object has no attribute 'proc'" in <bound method FirefoxRunner.cleanup of <mozrunner.runner.FirefoxRunner object at 0x2975e10>> ignored


Expected results:

This should maybe give a more meaningful error message such as "This is not a binary file"

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is just a request for a better error message we have to show. There is nothing in here which would really block a release. Lets move this to a mentored bug.

Comment 2

[Subhendu Ghosh]

Hi, I want to work on this bug. I know python, c and java. I am new to the Mozilla code base. Can you please guide me as how to proceed further in order to fix this bug?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Subhendu, you are welcome to work on this bug. All the necessary information you can find here: https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup

When you have cloned the repository make sure you do your work based off the master branch.

If you have questions don't hesitate to ask or drop by in our #automation channel on IRC. Thanks!

Comment 4

[Subhendu Ghosh]

Thanks! That will be of great help.

Comment 5

[Tarashish Mishra [:sunu]]

This bug seems to be fixed now.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Nope. Nothing has been changed here. We still have this massive failure message. As i have checked it's even part of mozbase and should most likely be fixed in mozrunner / mozprocess:

$ mozrunner -b /mozilla/bin/nightly/crashreporter.ini ['/mozilla/bin/nightly/crashreporter.ini', '-profile', '/tmp/tmpLkhgIg.mozrunner']
Traceback (most recent call last):
  File "/home/henrik/.virtualenvs/m2d/bin/mozrunner", line 9, in <module>
    load_entry_point('mozrunner==5.15', 'console_scripts', 'mozrunner')()
  File "/home/henrik/.virtualenvs/m2d/local/lib/python2.7/site-packages/mozrunner-5.15-py2.7.egg/mozrunner/runner.py", line 385, in cli
    CLI(args).run()
  File "/home/henrik/.virtualenvs/m2d/local/lib/python2.7/site-packages/mozrunner-5.15-py2.7.egg/mozrunner/runner.py", line 356, in run
    self.start(runner)
  File "/home/henrik/.virtualenvs/m2d/local/lib/python2.7/site-packages/mozrunner-5.15-py2.7.egg/mozrunner/runner.py", line 376, in start
    runner.start(debug_args=debug_args, interactive=interactive)
  File "/home/henrik/.virtualenvs/m2d/local/lib/python2.7/site-packages/mozrunner-5.15-py2.7.egg/mozrunner/runner.py", line 183, in start
    self.process_handler.run(timeout, outputTimeout)
  File "/home/henrik/.virtualenvs/m2d/local/lib/python2.7/site-packages/mozprocess-0.9-py2.7.egg/mozprocess/processhandler.py", line 621, in run
    self.proc = self.Process(self.cmd, **args)
  File "/home/henrik/.virtualenvs/m2d/local/lib/python2.7/site-packages/mozprocess-0.9-py2.7.egg/mozprocess/processhandler.py", line 76, in __init__
    universal_newlines, startupinfo, creationflags)
  File "/usr/lib/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1308, in _execute_child
    raise child_exception
OSError: [Errno 13] Permission denied

Comment 7

[Subhendu Ghosh]

sorry whimboo for not attending this bug for so long due to my semester exams in between and after that my hard disk crashed. now got it replaced. I will be quite regular on this bug now.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thank you for letting us know. Whenever you have problems ask or join us on IRC in #automation.

Comment 9

[Subhendu Ghosh]

https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/runner.py#L97
so in the above lines the binary path is checked. now the next few lines will check it the binary file is executable or not.

+        #check if the file is binary
+	st = os.stat(self.binary)
+	executable=bool(st.st_mode & (stat.S_IXUSR|stat.S_IXGRP|stat.S_IXOTH))
+              
+	if not executable:
+		print >> sys.stderr, "Binary File executed is not valid"
+		sys.exit(1)


in the above lines the binary file is checked it it is executable. if True then the program continues properly else if Fasle the program terminates with non zero exit code. tried with self.stop() instead of sys.exit(1) but the same OSError 13: Permission Denied is raised.

Now after the change the output is:

(venv)subho@ubuntu:~/mozmill$ mozrunner -b firefox/updater.ini
Binary File executed is not valid
(venv)subho@ubuntu:~/mozmill$

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I discussed that already with Subhendu on IRC yesterday, but why do we have to call sys.exit() here? William, he mentioned that it was your proposal. Can you explain it? I ask because I do not think that we should do that within an API. sys.exit() should only be called from a CLI class. So why can't we simply throw an OSError with the right message contained as already done when the binary does not exist?

Comment 11

[William Lachance (:wlach)]

(In reply to Henrik Skupin (:whimboo) [away 06/28 - 07/07] from comment #10)
> I discussed that already with Subhendu on IRC yesterday, but why do we have
> to call sys.exit() here? William, he mentioned that it was your proposal.
> Can you explain it? I ask because I do not think that we should do that
> within an API. sys.exit() should only be called from a CLI class. So why
> can't we simply throw an OSError with the right message contained as already
> done when the binary does not exist?

Yes, I agree, we should use the same convention as above, which is to throw an OSError. I gave that advice before looking at the actual file.

Comment 12

[Subhendu Ghosh]

So as suggested, i changed the code as follows:

        #check if the file is binary
	st = os.stat(self.binary)
	executable=bool(st.st_mode & (stat.S_IXUSR|stat.S_IXGRP|stat.S_IXOTH))
              
	if not executable:
		raise OSError ("Binary File executed is not valid")


Output obtained:

(venv)subho@ubuntu:~/mozmill$ mozrunner -b firefox/updater.ini
Traceback (most recent call last):
  File "/home/subho/mozmill/venv/bin/mozrunner", line 8, in <module>
    load_entry_point('mozrunner==5.18', 'console_scripts', 'mozrunner')()
  File "/home/subho/mozbase/mozrunner/mozrunner/runner.py", line 447, in cli
    CLI(args).run()
  File "/home/subho/mozbase/mozrunner/mozrunner/runner.py", line 416, in run
    runner = self.create_runner()
  File "/home/subho/mozbase/mozrunner/mozrunner/runner.py", line 412, in create_runner
    return self.runner_class.create(**self.runner_args())
  File "/home/subho/mozbase/mozrunner/mozrunner/runner.py", line 82, in create
    clean_profile=clean_profile, process_class=process_class)
  File "/home/subho/mozbase/mozrunner/mozrunner/runner.py", line 276, in __init__
    Runner.__init__(self, profile, binary, **kwargs)
  File "/home/subho/mozbase/mozrunner/mozrunner/runner.py", line 108, in __init__
    raise OSError ("Binary File executed is not valid")
OSError: Binary File executed is not valid
(venv)subho@ubuntu:~/mozmill$ 


Any further change required? please do suggest.

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Subhendu Ghosh from comment #12)
> 	executable=bool(st.st_mode & (stat.S_IXUSR|stat.S_IXGRP|stat.S_IXOTH))

You should include a single space either side of the '=' on this line.

> 		raise OSError ("Binary File executed is not valid")

I would suggest changing this message to "File specified is not executable"

> Any further change required? please do suggest.

It would be great to include a test for this, but I see that there aren't any tests for mozrunner yet, so it might not be a simple task.

Comment 14

[Subhendu Ghosh]

Attachment: Patch review

Comment 15

[Subhendu Ghosh]

Comment on attachment 771787 [details] [diff] [review]
Patch review

here this is the patch to this bug for review.

Comment 16

[Subhendu Ghosh]

Attachment: Patch review

Sorry, i made some mistakes while making the last patch attached above. so made it afresh here.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 771788 [details] [diff] [review]
Patch review

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

Sorry Subho-bcrec that it has been taken so a long time for my reply. This review request somehow got lost. :/

When checking the patch I'm not sure if that is really the right approach to do. Why can't we simply catch the exception raised by processhandler in mozrunner.start()?

https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/local.py#L374

William, what do you think?

Comment 18

[William Lachance (:wlach)]

Comment on attachment 771788 [details] [diff] [review]
Patch review

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

In general I think this looks ok: I think it's a bit friendlier to emit a descriptive error like this earlier on than to assert later on execution. 

The one problem is that I don't think it'll play nicely with Mac, where we allow specifying a binary as a plist. We should probably put this logic as an else condition to the "if mozinfo.isMac and os.path.exists(...):" clause.

If that were fixed along with the whitespace issues, I'd be happy to give this an r+.

::: mozrunner/mozrunner/runner.py
@@ +99,5 @@
>  
> +        #bug = 862253, check if the file is a valid binary
> +	st = os.stat(self.binary)
> +	executable = bool(st.st_mode & (stat.S_IXUSR|stat.S_IXGRP|stat.S_IXOTH))
> +              

Extraneous whitespace, need to remove this before pushing. I'd actually just take out the newline altogether here.

@@ +101,5 @@
> +	st = os.stat(self.binary)
> +	executable = bool(st.st_mode & (stat.S_IXUSR|stat.S_IXGRP|stat.S_IXOTH))
> +              
> +	if not executable:
> +	    raise OSError ("File specified is not executable")

Should be no space between OSError and "(".

@@ +102,5 @@
> +	executable = bool(st.st_mode & (stat.S_IXUSR|stat.S_IXGRP|stat.S_IXOTH))
> +              
> +	if not executable:
> +	    raise OSError ("File specified is not executable")
> +	

Extraneous whitespace, need to remove this before pushing.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to William Lachance (:wlach) from comment #18)
> The one problem is that I don't think it'll play nicely with Mac, where we
> allow specifying a binary as a plist. We should probably put this logic as
> an else condition to the "if mozinfo.isMac and os.path.exists(...):" clause.

Well, why can't we simply move this check right after the pinfo code then? If something wrong will be specified and we can't read this file, another error will be shown. But once read in we should have the binary for all platforms, and that value should be tested, right?

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to William Lachance (:wlach) from comment #18)
> The one problem is that I don't think it'll play nicely with Mac, where we
> allow specifying a binary as a plist. We should probably put this logic as
> an else condition to the "if mozinfo.isMac and os.path.exists(...):" clause.

William, not sure I understand this. Since when do we allow to specify a binary as a plist? What we support AFAIR is the .app bundle and the real binary. Let me know if I misread something. Thanks.

Comment 21

[William Lachance (:wlach)]

Needinfo'ing myself to figure out what should happen next here

Comment 22

[William Lachance (:wlach)]

Sorry for the delay in replying to this.

(In reply to Henrik Skupin (:whimboo) from comment #20)
> (In reply to William Lachance (:wlach) from comment #18)
> > The one problem is that I don't think it'll play nicely with Mac, where we
> > allow specifying a binary as a plist. We should probably put this logic as
> > an else condition to the "if mozinfo.isMac and os.path.exists(...):" clause.
> 
> William, not sure I understand this. Since when do we allow to specify a
> binary as a plist? What we support AFAIR is the .app bundle and the real
> binary. Let me know if I misread something. Thanks.

Sorry, that's what I meant. The problem is that an app bundle isn't necessarily executable, so we need to handle that special case.

Subhendu: Do you have time to upload a new patch?

Comment 23

[Subhendu Ghosh]

Yes, i can upload new patch. What changes are needed?

Comment 24

[William Lachance (:wlach)]

(In reply to Subhendu Ghosh from comment #23)
> Yes, i can upload new patch. What changes are needed?

Modify the patch so that we first check if the file is an app bundle, and only do the check for executability if that's not the case. You can see some plist detection code just below what you changed earlier.

Comment 25

[William Lachance (:wlach)]

Hey Subhendu, are you still interested in working on this? If not (which is fine), I'd like to unassign you from it so someone else can pick it up.

Comment 26

[Subhendu Ghosh]

Hi William, yes i am still interested but currently i am having a shortage of time, will be free to work on it from next week.

Comment 27

[Subhendu Ghosh]

With the codebase updated, runner.py that used to be at https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/runner.py 
where the error checking was supposed to be done has been removed.Instead mozrunner==5.15 is installed as a dependency python package.In the new logs the error seems to generate in the bin folder under virtual environment directory. I think we need to consider the mozrunner codebase instead of mozmill.

Output on eecution:

(env)[subho@localhost mozmill]$ mozrunner -b firefox/updater.ini 
['firefox/updater.ini', '-profile', '/tmp/tmpRI6XxL.mozrunner']
Traceback (most recent call last):
  File "/home/subho/work/mozilla-mozmill/env/bin/mozrunner", line 9, in <module>
    load_entry_point('mozrunner==5.15', 'console_scripts', 'mozrunner')()
  File "/home/subho/work/mozilla-mozmill/env/lib/python2.7/site-packages/mozrunner-5.15-py2.7.egg/mozrunner/runner.py", line 385, in cli
    CLI(args).run()
  File "/home/subho/work/mozilla-mozmill/env/lib/python2.7/site-packages/mozrunner-5.15-py2.7.egg/mozrunner/runner.py", line 356, in run
    self.start(runner)
  File "/home/subho/work/mozilla-mozmill/env/lib/python2.7/site-packages/mozrunner-5.15-py2.7.egg/mozrunner/runner.py", line 376, in start
    runner.start(debug_args=debug_args, interactive=interactive)
  File "/home/subho/work/mozilla-mozmill/env/lib/python2.7/site-packages/mozrunner-5.15-py2.7.egg/mozrunner/runner.py", line 183, in start
    self.process_handler.run(timeout, outputTimeout)
  File "/home/subho/work/mozilla-mozmill/env/lib/python2.7/site-packages/mozprocess-0.21-py2.7.egg/mozprocess/processhandler.py", line 669, in run
    self.proc = self.Process([self.cmd] + self.args, **args)
  File "/home/subho/work/mozilla-mozmill/env/lib/python2.7/site-packages/mozprocess-0.21-py2.7.egg/mozprocess/processhandler.py", line 97, in __init__
    universal_newlines, startupinfo, creationflags)
  File "/usr/lib64/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/usr/lib64/python2.7/subprocess.py", line 1327, in _execute_child
    raise child_exception
OSError: [Errno 13] Permission denied
(env)[subho@localhost mozmill]$

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Subhendu, a while ago the contents of the mozbase repository has been moved to mozilla-central, which is also the repository of the Firefox code. Please see the updated URL in how to retrieve the latest code. Also the latest mozrunner is version 6.2. You may want to re-check best with the code on mozilla-central/testing/mozbase.

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

resetting the assigned to field for a few months of inactivity.

:whimboo, as you are the mentor, can you confirm this bug is still useful to get fixed and would still make a good first bug?

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just tested with latest mozrunner and my steps from comment 6 and it's still necessary, yes!

Comment 31

[Matt Gibson (:gibbo)]

I would like to work on this bug. I am familiar with Python, and I am trying to learn more about the Mozilla codebase.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great to hear that, Matt. I have assigned you to this bug now. If you have questions when reading through the former comments please let me know here or on IRC. Thanks.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Matt, can you please tell me if you still want to fix it? If not please tell us, so we can find someone else or do it on our own. Thanks.

Comment 34

[Akshat Jain]

I wish to work on this bug, can you please tell me how to start with it !!!

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

I would start with building firefox locally, the sources for mozrunner live in the main tree.

Here are some instructions for getting started:
http://areweeveryoneyet.org/onramp/desktop.html

While you are waiting for the source and build, you can familiarize yourself with the bug history.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I don't see why there is a need to build firefox yourself here. The only thing you would need to replicate the issue is to use a non binary as option for the --binary argument.

Comment 37

[Nagma Kapoor (:nagma)]

Is anyone working on this bug right now? I would be interested in it.

Comment 38

[Joel Maher ( :jmaher ) (UTC -8)]

:nagma, welcome to Mozilla.  This bug is for you to work on!

To get up to speed on some terminology and other ways of development, check out our bootcamp:
http://ateam-bootcamp.readthedocs.org/en/latest/guide/

Please ask questions in this bug or on irc in #ateam.

Comment 39

[Nagma Kapoor (:nagma)]

Hi Joel! Thanks for assigning me but I'm sorry I won't be able to work on this bug anymore. I've started working on another one for Marionette instead.

Comment 40

[Jacob Harrow-Mortelliti [:jakehm]]

Aight, gimme a go on this bad boy.

Comment 41

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

Unsure if there was anything else for that needinfo beyond assigning the bug. I'll leave it up until someone says otherwise.

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jacob, thanks for your interest. Lets see if we finally can get this bad boy fixed. :)

I would suggest that you get the code of mozilla-central via Mercurial (hg clone https://hg.mozilla.org/mozilla-central) or Git (git clone [e-mail]:mozilla/gecko-dev.git). The mozrunner package you can find under testing/mozbase/mozrunner. Example commands how to reproduce you can find above.

So let me know if you need something else.

Comment 43

[Jacob Harrow-Mortelliti [:jakehm]]

So it looks like Subhendu got pretty far on this.  Is the current source code similar enough that I can just take his changes and add code detecting if it's in an app bundle or should I start from scratch?

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As long as the code still applies and will help, you should try to re-use it. Just add the missing pieces. Starting from scratch would be a waste of time.

Comment 45

[Jacob Harrow-Mortelliti [:jakehm]]

hmm So what/where is the new file?  in testing/mozbase/mozrunner/mozrunner all I see is runners.py rather than runner.py.

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Interesting. There were indeed a lot of changes to the affected code within the last months. And that not only the move of the code, but also small but helpful fixes. It means the issue from my initial comment is not visible anymore. We now get a `No such file or directory` exception. The other issue regarding missing permissions for executing a subprocess we might want to leave. Putting another wrapper around it might not help given that it would only throw a different message.

Jacob, looks like that no more work needs to be done here. Are you interested to work on something else?

Comment 47

[Jacob Harrow-Mortelliti [:jakehm]]

Sure.  Do you have something else in mind?

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We found bug 1273843 for Jacob.