695937 - pageloader extension/talos should die more gracefully when pageset doesn't exist

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[[e-mail] (:bhearsum)]

I was debugging a really weird looking crash when trying to run Tp tonight, which looked like this:
(view as text)

python run_tests.py --noisy 20111019_1850_config.yml
 in dir /Users/cltbld/talos-slave/snowleopard/../talos-data/talos/ (timeout 21600 secs)
 watching logfiles {}
 argv: ['python', 'run_tests.py', '--noisy', '20111019_1850_config.yml']
 environment:
  Apple_PubSub_Socket_Render=/tmp/launch-Sq58Qb/Render
  CVS_RSH=ssh
  DISPLAY=/tmp/launch-jGAcQN/org.x:0
  HOME=/Users/cltbld
  LOGNAME=cltbld
  MOZ_CRASHREPORTER_NO_REPORT=1
  MOZ_NO_REMOTE=1
  NO_EM_RESTART=1
  PAGER=/bin/cat
  PATH=/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/X11/bin
  PWD=/Users/cltbld/talos-slave/talos-data/talos
  PYTHONPATH=/Library/Python/2.5/site-packages
  SHELL=/bin/bash
  SSH_AUTH_SOCK=/tmp/launch-9rbnbN/Listeners
  TMPDIR=/var/folders/H5/H5TD8hgwEqKq9hgKlayjWU+++TM/-Tmp-/
  USER=cltbld
  VERSIONER_PYTHON_PREFER_32_BIT=no
  VERSIONER_PYTHON_VERSION=2.6
  XPCOM_DEBUG_BREAK=warn
  __CF_USER_TEXT_ENCODING=0x1F6:0:0
 using PTY: False
RETURN:s: talos-r3-snow-001
RETURN:id:20110928134238
RETURN:<a href = "http://hg.mozilla.org/releases/mozilla-release/rev/58f3edbff1b9">rev:58f3edbff1b9</a>
talos-r3-snow-001: 
		Started Wed, 19 Oct 2011 18:50:47
Running test tp5: 
		Started Wed, 19 Oct 2011 18:50:47
	Screen width/height:1600/1200
	colorDepth:24
	Browser inner width/height: 1024/638

NOISE: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIFileInputStream.init]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: chrome://pageloader/content/pageloader.js :: plLoadURLsFromURI :: line 605"  data: no]
NOISE: __startBeforeLaunchTimestamp1319075462553__endBeforeLaunchTimestamp
NOISE: __startAfterTerminationTimestamp1319075463043__endAfterTerminationTimestamp
Traceback (most recent call last):
  File "run_tests.py", line 546, in <module>
    test_file(arg, screen, amo)
  File "run_tests.py", line 491, in test_file
    browser_dump, counter_dump, print_format = mytest.runTest(browser_config, test)
  File "/Users/cltbld/talos-slave/talos-data/talos/ttest.py", line 300, in runTest
    cm = self.cmanager.CounterManager(self._ffprocess, browser_config['process'], counters)
  File "/Users/cltbld/talos-slave/talos-data/talos/cmanager_mac.py", line 105, in __init__
    self.pid = self.ffprocess.GetPidsByName(process)[-1]
IndexError: list index out of range
program finished with exit code 1
elapsedTime=20.989227

And generated this crash report: https://crash-stats.mozilla.com/report/index/8499a162-7f66-4e9a-85ef-63a022111019

Eventually I figured out that I forgot to unpack the pageset prior to running the tests. The pageloader could die with a MUCH better error in cases like this. Talos could probably pre-detect this state, and not try to start Firefox at all, too.

Comment 1

[William Lachance (:wlach)]

I wonder if we should list the prerequisite extensions that each test requires, and somehow error out before running them if those extensions are not present in the profile.

Comment 2

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

we need to pick this up again.

Comment 3

[Mike Hoye [:mhoye]]

William, would you be willing to mentor this bug?

Comment 4

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

either wlach or dminor

Comment 5

[William Lachance (:wlach)]

To be honest I've mostly forgotten how the pageloader code works, so I wouldn't be the best mentor. Let's see if dminor is interested.

Comment 6

[Dan Minor [:dminor]]

I had planned to learn more about talos to be able to mentor this, but that didn't happen :)

Comment 7

[Shruti Jasoria [:ShrutiJ]]

Hello, I would like to work on this bug. Where do I start?

Comment 8

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

Hi Shruti!  I am excited to see your interest in this bug.  Please learn a bit about Talos and setup a Firefox build/development environment locally:
https://wiki.mozilla.org/Auto-tools/Projects/Talos

Once you have a local build, verify you can run |mach talos-test -a tsvgx| and it runs successfully.  After you get that done, I would be happy to work with you on how pageloader works and where it would make sense to add code to fix this bug.

Comment 9

[Shruti Jasoria [:ShrutiJ]]

Hi, I have finished setting up Talos locally. The above test ran successfully. What should I do next?

Comment 10

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

to run this we can do the same test but temporarily remove testing/talos/talos/tests/svgx/svgx.manifest.  Then run the test again and you should see a failure.

This is the code which loads the file:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/pageloader/chrome/pageloader.js#808

I would familiarize yourself with the code and APIs there- we could wrap it in a try/catch or find a way to check for the file to exist.  Lastly we would need to print out an error message and make sure that talos will terminate properly so it makes sense to the user/automation.

Comment 11

[Shruti Jasoria [:ShrutiJ]]

Is there any documentation for the APIs used in the above file? I'll read it before I start working on it.

Comment 12

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

the best place for documentation is MDN:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFileInputStream

I personally have to refer to that all the time I work on file IO from an addon, it is something I do once a year and always forget things.

Comment 13

[Shruti Jasoria [:ShrutiJ]]

I have familiarized myself with the APIs being used. According to my understanding, wrapping the statement using nsIFileInputStream in try..catch should work. While I was going through the documentation, I found a few methods which may help in doing this work in a simpler manner:

contains()
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFile#contains()

isFile()
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFile#isFile()

Will any of the above functions work, or should I go forward with try...catch?

Comment 14

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

you are on the right track.  This is great you found the references.  I would probably do a bit of both- check isFile() and if not, print appropriate error, likewise wrap it in a try/catch and print a more generic error.

The code can be modified locally and tested locally, once you feel it is good, I can test it on our try server and help out with signing the addon to land it in production.

Comment 15

[Shruti Jasoria [:ShrutiJ]]

I have made some changes to my repository. How do I test them locally?

Comment 16

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

ok, to test this there are a few things:
1) in comment 8 is discussed a method to run a test locally, validate you can do that with your patch applied
2) temporarily delete (I would rename) testing/talos/talos/tests/svgx/svgx.manifest (make sure to delete any svgx.manifest.develop files in the same directory), then run the talos test again.  I would expect an error!

If you get that and the error makes sense (as in "pageloader is unable to find manifest file: <filename>") then please attach your code to the bug for review.  If you are using mozreview (recommended), that is pushing to the mozreview repo.  If you have trouble setting that up, or want to keep it simpler for right now, then getting a diff |hg diff| and attaching the file as an attachment to this bug.

I look forward to seeing your code that fixes this!

Comment 17

[Shruti Jasoria [:ShrutiJ]]

Attachment: Required Changes with isFile() followed by try/catch

These are the changes which I have made in pageloader.js. They passed the tests locally.

After removing svgx.manifest and svgx.manifest.develop, the tests gave some error.

Please see the diff file and let me know if any changes are required to be made. Can you suggest better dumpLine messages for the conditions which I have written.

Comment 18

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

Comment on attachment 8766682 [details] [diff] [review]
Required Changes with isFile() followed by try/catch

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

this is a great start, lets fix the few small issues below!

::: testing/talos/talos/pageloader/chrome/pageloader.js
@@ +812,5 @@
>    var uriFile = manifestUri.QueryInterface(Ci.nsIFileURL);
>  
> +  if (uriFile.file.isFile() === false)
> +  {
> +    dumpLine("tp: invalid file");

I would like to see the filename printed here:
dumpLine("tp: invalid file: %s" % manifestUri);

@@ +820,5 @@
> +  try{
> +    fstream.init(uriFile.file, -1, 0, 0);
> +  }
> +  catch(ex){
> +    dumpLine("tp: the file doesn't exist");

same here, lets mention the file that doesn't exist.

@@ +825,5 @@
> +    return null;
> +  }
> +
> +
> +  

nit: too many blank lines and if you have a blank line, it should be blank, not whitespace.

Comment 19

[Shruti Jasoria [:ShrutiJ]]

Attachment: Bug_695937.diff

I have made the changes you asked for in comment 18. Please take a look and let me know if any other change has to be made.

Comment 20

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

Comment on attachment 8767032 [details] [diff] [review]
Bug_695937.diff

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

still a f+, this is looking better, now lets focus on the small details!

::: testing/talos/talos/pageloader/chrome/pageloader.js
@@ +812,5 @@
>    var uriFile = manifestUri.QueryInterface(Ci.nsIFileURL);
>  
> +  if (uriFile.file.isFile() === false)
> +  {
> +    dumpLine("tp: invalid file: %s" % manifestUri);

actually we should use uriFile.file here, I misled you.

@@ +820,5 @@
> +  try{
> +    fstream.init(uriFile.file, -1, 0, 0);
> +  }
> +  catch(ex){
> +    dumpLine("tp: the file %s doesn't exist" %uriFile.file);

please pay attention to spacing, add a space between % and uriFile.file.

Also be consistent with your { and }, these should match the rest of the file, there are a few cases where that isn't the case in your changes.

Comment 21

[Shruti Jasoria [:ShrutiJ]]

Attachment: Changes with corrected spacing

I have corrected the spacing and {} usage. Is there anything else which has to be done for this bug?

Comment 22

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

Comment on attachment 8767074 [details] [diff] [review]
Changes with corrected spacing

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

this is great.  Are you familiar with pushing to try server?  I would like to ensure this works on linux64 for at least one test |try: -b o -p linux64 -u none -t tp5o|.

If not, I can help with that.

Comment 23

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

pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=19eea2c695ba

Comment 24

[Shruti Jasoria [:ShrutiJ]]

can you assign me on this bug.Thanks :)

Comment 25

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

this is yours!

Comment 26

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

Attachment: updated patch for version bump + signing

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/903165e388f4
pageloader extension/talos should die more gracefully when pageset doesn't exist. r=jmaher

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/903165e388f4