1080783 - Autophone - process crash dumps for non unittests

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

Currently, Autophone does not process crash dumps for the Smoketest, S1S2 or Webappstartup tests. It should do so, and if Treeherder is configured, it should send the results to Treeherder as an artifact. Once we have S3 storage available, we may also want to upload the dump to S3. See Bug 1080782.

gbrown: Are you interested in this one?

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: bug-1080783-1-v1.patch

process crash dumps: This adds a new file/class to process the dumps. Unfortunately, I was unable to use mozcrash due to its restrictions on methods and its tight coupling with the in-tree frameworks. Particularly troublesome were print statements, use of structured logging and the emitting of log messages inside of what in my opinion should have been pure processing methods.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: bug-1080783-2-v1.patch

Better define AutophoneTreeherder.submit_complete usage to eliminate duplicate error messages: submit_complete was confusing and a source of duplicate failure messages in Treeherder.

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: bug-1080783-3-v1.patch

Reset the PhoneTest.test_result on setup_job so we do no carry forward values from previous run. 1 liner in the spirt of keeping patches small and unrelated changes separate. ;-)

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: bug-1080783-4-v1.patch

Do not duplicate bugs in autophonetreeherder.get_bug_suggestions when there are no matching bugs. Logic error in original patch was the cause of additional duplicate failures in Treeherder.

Comment 5

[Bob Clary [:bc] (inactive)]

Test run where I had a process continually killing fennec via kill -11 on my devices:

https://treeherder.allizom.org/ui/#/jobs?repo=mozilla-inbound&revision=1a5309ae3125&filter-searchStr=autophone

Normal test run where I let the chips fail where they may:

https://treeherder.allizom.org/ui/#/jobs?repo=mozilla-inbound&revision=0b4e473beaa5&filter-searchStr=autophone

Comment 6

[Bob Clary [:bc] (inactive)]

I forgot to mention that in the test runs, the samsung-gs3 and the nexus-one are the devices which used these patches. The other nexus devices are those running in production without these patches.

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 8543760 [details] [diff] [review]
bug-1080783-1-v1.patch

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

::: autophonecrash.py
@@ +44,5 @@
> +        :param remote_profile_dir: path on device to the Firefox
> +            profile.
> +        :param upload_dir: path to a host directory to be used to contain
> +            ANR traces, tombstones uploaded from the device.
> +

Extra blank line here and elsewhere.

@@ +56,5 @@
> +
> +    @property
> +    def remote_dump_dir(self):
> +        """Minidump directory in Firefox profile
> +

Just make this docstring one line.

@@ +64,5 @@
> +        return self._remote_dump_dir
> +
> +    def delete_anr_traces(self):
> +        """empty ANR traces.txt file.
> +

Ditto, and "Empty".

@@ +95,5 @@
> +                self.delete_anr_traces()
> +            except ADBError:
> +                self.logger.warning("Error pulling %s" % traces)
> +            except IOError:
> +                self.logger.warning("Error pulling %s" % traces)

Should this not be self.logger.exception() so we record the exception?

@@ +160,5 @@
> +        exception = None
> +
> +        logcat = self.adb.get_logcat()
> +
> +        for i, line in enumerate(logcat):

Do you think it's worth this being a while loop instead, so you can skip over the lines you process below, rather than going back and checking them on line 171 again?

Comment 8

[Bob Clary [:bc] (inactive)]

(In reply to Mark Côté [:mcote] from comment #7)
> Comment on attachment 8543760 [details] [diff] [review]
> bug-1080783-1-v1.patch
> 
> Review of attachment 8543760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: autophonecrash.py
> @@ +44,5 @@
> > +        :param remote_profile_dir: path on device to the Firefox
> > +            profile.
> > +        :param upload_dir: path to a host directory to be used to contain
> > +            ANR traces, tombstones uploaded from the device.
> > +
> 
> Extra blank line here and elsewhere.
> 

When I use Emac's reformat on docstrings, the extra blank line is added automatically. I'd like to keep that unless you really object to it.

> @@ +56,5 @@
> > +
> > +    @property
> > +    def remote_dump_dir(self):
> > +        """Minidump directory in Firefox profile
> > +
> 
> Just make this docstring one line.
> 

Ok

> @@ +64,5 @@
> > +        return self._remote_dump_dir
> > +
> > +    def delete_anr_traces(self):
> > +        """empty ANR traces.txt file.
> > +
> 
> Ditto, and "Empty".
> 

Ok.

> @@ +95,5 @@
> > +                self.delete_anr_traces()
> > +            except ADBError:
> > +                self.logger.warning("Error pulling %s" % traces)
> > +            except IOError:
> > +                self.logger.warning("Error pulling %s" % traces)
> 
> Should this not be self.logger.exception() so we record the exception?
> 

I've tried to stay away from exception if it is an error that can be ignored. What do you think of emitting the exception in the warning as in:

> > +            except ADBError, e:
> > +                self.logger.warning("Error %s pulling %s" % (e, traces)) 

etc?

> @@ +160,5 @@
> > +        exception = None
> > +
> > +        logcat = self.adb.get_logcat()
> > +
> > +        for i, line in enumerate(logcat):
> 
> Do you think it's worth this being a while loop instead, so you can skip
> over the lines you process below, rather than going back and checking them
> on line 171 again?

I lifted that directly from mozcrash.py with additions to fix Bug 1116731. To change it I would introduce a state to track whether I'm looking for the exception marker, the exception type or the exception location and remove the look ahead. That's fine with me if you like.

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to Bob Clary [:bc:] from comment #8)
> (In reply to Mark Côté [:mcote] from comment #7)
> > Comment on attachment 8543760 [details] [diff] [review]
> > bug-1080783-1-v1.patch
> > 
> > Review of attachment 8543760 [details] [diff] [review]:
> > -----------------------------------------------------------------

> > @@ +160,5 @@
> > > +        exception = None
> > > +
> > > +        logcat = self.adb.get_logcat()
> > > +
> > > +        for i, line in enumerate(logcat):
> > 
> > Do you think it's worth this being a while loop instead, so you can skip
> > over the lines you process below, rather than going back and checking them
> > on line 171 again?
> 
> I lifted that directly from mozcrash.py with additions to fix Bug 1116731.
> To change it I would introduce a state to track whether I'm looking for the
> exception marker, the exception type or the exception location and remove
> the look ahead. That's fine with me if you like.

Actually, this would be more of a pain to implement using a simple loop without the look ahead and wouldn't gain much. We automatically terminate the loop once the uncaught exception is detected and don't loop back to check them again. The one gain I can think of is being able to report the exception if the exception had a type but not a location. I'll leave it to you to make the call.

Comment 10

[Mark Côté [:mcote]]

(In reply to Bob Clary [:bc:] from comment #8)
> (In reply to Mark Côté [:mcote] from comment #7)
> > Comment on attachment 8543760 [details] [diff] [review]
> > bug-1080783-1-v1.patch
> > 
> > Review of attachment 8543760 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: autophonecrash.py
> > @@ +44,5 @@
> > > +        :param remote_profile_dir: path on device to the Firefox
> > > +            profile.
> > > +        :param upload_dir: path to a host directory to be used to contain
> > > +            ANR traces, tombstones uploaded from the device.
> > > +
> > 
> > Extra blank line here and elsewhere.
> > 
> 
> When I use Emac's reformat on docstrings, the extra blank line is added
> automatically. I'd like to keep that unless you really object to it.

Hm well I'd *prefer* if tools respected style and conventions and not the other way around, but it's not a huge deal.

> > @@ +95,5 @@
> > > +                self.delete_anr_traces()
> > > +            except ADBError:
> > > +                self.logger.warning("Error pulling %s" % traces)
> > > +            except IOError:
> > > +                self.logger.warning("Error pulling %s" % traces)
> > 
> > Should this not be self.logger.exception() so we record the exception?
> > 
> 
> I've tried to stay away from exception if it is an error that can be
> ignored. What do you think of emitting the exception in the warning as in:
> 
> > > +            except ADBError, e:
> > > +                self.logger.warning("Error %s pulling %s" % (e, traces)) 
> 
> etc?

Sure, I don't really care about the logging level; I just think exceptions should only be completely ignored if they're expected and handled appropriately.  If they are unexpected, we should log information about them, so we can diagnose failures later.

(In reply to Bob Clary [:bc:] from comment #9)
> (In reply to Bob Clary [:bc:] from comment #8)
> > (In reply to Mark Côté [:mcote] from comment #7)
> > > Comment on attachment 8543760 [details] [diff] [review]
> > > bug-1080783-1-v1.patch
> > > 
> > > Review of attachment 8543760 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> 
> > > @@ +160,5 @@
> > > > +        exception = None
> > > > +
> > > > +        logcat = self.adb.get_logcat()
> > > > +
> > > > +        for i, line in enumerate(logcat):
> > > 
> > > Do you think it's worth this being a while loop instead, so you can skip
> > > over the lines you process below, rather than going back and checking them
> > > on line 171 again?
> > 
> > I lifted that directly from mozcrash.py with additions to fix Bug 1116731.
> > To change it I would introduce a state to track whether I'm looking for the
> > exception marker, the exception type or the exception location and remove
> > the look ahead. That's fine with me if you like.
> 
> Actually, this would be more of a pain to implement using a simple loop
> without the look ahead and wouldn't gain much. We automatically terminate
> the loop once the uncaught exception is detected and don't loop back to
> check them again. The one gain I can think of is being able to report the
> exception if the exception had a type but not a location. I'll leave it to
> you to make the call.

Oh true, I missed that break.  All good.

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: bug-1080783-1-review-patch

Patch addressing review comments: fixed the docstrings, the exception logging for anr traces and an additional indent issue.

Comment 12

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/7fb94b3fcaa254996e98abddadc4f572fbe99ee0
https://github.com/mozilla/autophone/commit/6a047b9dee375b4988b3642ce32cbc682ce3732f
https://github.com/mozilla/autophone/commit/fe24e2d16c6fba407e38b500e0506415cb0cfecf
https://github.com/mozilla/autophone/commit/53aefcbba0dd0e30e0c1deb54ee6c21222f4c26c