Closed Bug 695937 Opened 10 years ago Closed 5 years ago

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


(Testing :: Talos, defect)

Not set


(firefox50 fixed)

Tracking Status
firefox50 --- fixed


(Reporter: bhearsum, Assigned: ShrutiJ, Mentored)



(Whiteboard: [good first bug][lang=js][talos_wishlist])


(2 files, 2 obsolete files)

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

python --noisy 20111019_1850_config.yml
 in dir /Users/cltbld/talos-slave/snowleopard/../talos-data/talos/ (timeout 21600 secs)
 watching logfiles {}
 argv: ['python', '', '--noisy', '20111019_1850_config.yml']
 using PTY: False
RETURN:s: talos-r3-snow-001
RETURN:<a href = "">rev:58f3edbff1b9</a>
		Started Wed, 19 Oct 2011 18:50:47
Running test tp5: 
		Started Wed, 19 Oct 2011 18:50:47
	Screen width/height:1600/1200
	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 "", line 546, in <module>
    test_file(arg, screen, amo)
  File "", line 491, in test_file
    browser_dump, counter_dump, print_format = mytest.runTest(browser_config, test)
  File "/Users/cltbld/talos-slave/talos-data/talos/", line 300, in runTest
    cm = self.cmanager.CounterManager(self._ffprocess, browser_config['process'], counters)
  File "/Users/cltbld/talos-slave/talos-data/talos/", line 105, in __init__ = self.ffprocess.GetPidsByName(process)[-1]
IndexError: list index out of range
program finished with exit code 1

And generated this crash report:

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.
Assignee: nobody → wlachance
Whiteboard: [good-first-bug]
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.
Whiteboard: [good-first-bug] → [good first bug]
Whiteboard: [good first bug] → [good first bug][mentor=wlach]
we need to pick this up again.
Whiteboard: [good first bug][mentor=wlach] → [good first bug][mentor=jhammel]
Assignee: wlachance → nobody
William, would you be willing to mentor this bug?
Flags: needinfo?(wlachance)
Whiteboard: [good first bug][mentor=jhammel] → [good first bug][mentor needed]
either wlach or dminor
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.
Flags: needinfo?(wlachance) → needinfo?(dminor)
Mentor: dminor
Flags: needinfo?(dminor)
Whiteboard: [good first bug][mentor needed] → [good first bug]
I had planned to learn more about talos to be able to mentor this, but that didn't happen :)
Mentor: dminor → nobody
Blocks: 1088251
Mentor: nobody → jmaher
Whiteboard: [good first bug] → [good first bug][lang=javascript]
Whiteboard: [good first bug][lang=javascript] → [good first bug][lang=js]
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][talos_wishlist]
Hello, I would like to work on this bug. Where do I start?
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:

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.
Hi, I have finished setting up Talos locally. The above test ran successfully. What should I do next?
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:

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.
Is there any documentation for the APIs used in the above file? I'll read it before I start working on it.
the best place for documentation is MDN:

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.
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:



Will any of the above functions work, or should I go forward with try...catch?
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.
I have made some changes to my repository. How do I test them locally?
Flags: needinfo?(jmaher)
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!
Flags: needinfo?(jmaher)
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.
Attachment #8766682 - Flags: feedback?(jmaher)
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.
Attachment #8766682 - Flags: feedback?(jmaher) → feedback+
Attached patch Bug_695937.diff (obsolete) — Splinter Review
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.
Attachment #8766682 - Attachment is obsolete: true
Attachment #8767032 - Flags: feedback?(jmaher)
Comment on attachment 8767032 [details] [diff] [review]

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.
Attachment #8767032 - Flags: feedback?(jmaher) → feedback+
I have corrected the spacing and {} usage. Is there anything else which has to be done for this bug?
Attachment #8767032 - Attachment is obsolete: true
Attachment #8767074 - Flags: feedback?(jmaher)
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.
Attachment #8767074 - Flags: feedback?(jmaher) → review+
can you assign me on this bug.Thanks :)
Flags: needinfo?(jmaher)
this is yours!
Assignee: nobody → shrutijasoria1996
Flags: needinfo?(jmaher)
Pushed by
pageloader extension/talos should die more gracefully when pageset doesn't exist. r=jmaher
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.