923500 - Add check_for_java_exception() to mozcrash

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Ed Morley [:emorley]]

Bug 910320 would like to add support for checking for Android java exceptions to talos. We currently have checkForJavaException() in remoteautomation.py [1] - we should move this to mozcrash so we can use it in talos too.

Once this has landed & we've rolled out the new version of mozcrash, we can proceed with bug 910320.

[1] http://hg.mozilla.org/mozilla-central/file/51b36c5fd45f/build/mobile/remoteautomation.py#l89

Comment 1

[Ed Morley [:emorley]]

Attachment: Patch v1

A copy of http://hg.mozilla.org/mozilla-central/file/51b36c5fd45f/build/mobile/remoteautomation.py#l89 with a few comments rearranged.

Once this is rolled out, I'll remove the remoteautomation.py method and get both remoteautomation.py and Talos using this.

Comment 2

[Ed Morley [:emorley]]

Attachment: Part 1: Use mozcrash.check_for_java_exception()

Use the new mozcrash.check_for_java_exception() added by bug 910320 (this will obviously have to land after the updated mozcrash is rolled out).

Comment 3

[Ed Morley [:emorley]]

Attachment: Part 2: Fix some existing Pyflakes warnings

ttest.py:15: 'glob' imported but unused
ttest.py:19: 're' imported but unused
ttest.py:21: 'shutil' imported but unused
ttest.py:129: local variable 'cleanup_result' is assigned to but never used
ttest.py:199: local variable 'e' is assigned to but never used

Comment 4

[Ed Morley [:emorley]]

Comment on attachment 813653 [details] [diff] [review]
Part 1: Use mozcrash.check_for_java_exception()

Dammit wrong bug number + bzexport. These were meant for bug 910320.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 813592 [details] [diff] [review]
Patch v1

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

::: mozcrash/mozcrash/mozcrash.py
@@ +158,5 @@
> +
> +def check_for_java_exception(logcat):
> +    """
> +    Prints a summary of a fatal java exception, if present in the provided
> +    logcat output.

This should mention what the logcat parameter should be. I take it it should be a list of strings?

@@ +176,5 @@
> +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at org.mozilla.gecko.GeckoApp$21.run(GeckoApp.java:1833)
> +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at android.os.Handler.handleCallback(Handler.java:587)
> +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at android.os.Handler.dispatchMessage(Handler.java:92)
> +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at android.os.Looper.loop(Looper.java:123)
> +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)

This seems super verbose for a comment, but if this is what the original code looked like then I'm not going to make you change it right now.

@@ +181,5 @@
> +        if "REPORTING UNCAUGHT EXCEPTION" in line or "FATAL EXCEPTION" in line:
> +            # Strip away the date, time, logcat tag and pid from the next two lines and
> +            # concatenate the remainder to form a concise summary of the exception.
> +            found_exception = True
> +            logre = re.compile(r".*\): \t?(.*)")

Can you move this outside the loop?

@@ +185,5 @@
> +            logre = re.compile(r".*\): \t?(.*)")
> +            m = logre.search(logcat[i+1])
> +            if m and m.group(1):
> +                top_frame = m.group(1)
> +            m = logre.search(logcat[i+2])

These aren't checking that i+1 or i+1 < len(logcat).

Comment 6

[Ed Morley [:emorley]]

Attachment: Patch v2

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> > +def check_for_java_exception(logcat):
> > +    """
> > +    Prints a summary of a fatal java exception, if present in the provided
> > +    logcat output.
> 
> This should mention what the logcat parameter should be. I take it it should
> be a list of strings?

Correct; added :-)

> @@ +176,5 @@
> > +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at org.mozilla.gecko.GeckoApp$21.run(GeckoApp.java:1833)
> > +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at android.os.Handler.handleCallback(Handler.java:587)
> > +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at android.os.Handler.dispatchMessage(Handler.java:92)
> > +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at android.os.Looper.loop(Looper.java:123)
> > +        # 01-30 20:15:41.937 E/GeckoAppShell( 1703): 	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)
> 
> This seems super verbose for a comment, but if this is what the original
> code looked like then I'm not going to make you change it right now.

Yeah agree, but the regexp makes a bit more sense with it. However, I've reduced the number of example lines, so hopefully now not quite as bad.

> @@ +181,5 @@
> > +        if "REPORTING UNCAUGHT EXCEPTION" in line or "FATAL EXCEPTION" in line:
> > +            # Strip away the date, time, logcat tag and pid from the next two lines and
> > +            # concatenate the remainder to form a concise summary of the exception.
> > +            found_exception = True
> > +            logre = re.compile(r".*\): \t?(.*)")
> 
> Can you move this outside the loop?

Whilst ordinarily I'd agree the compile should be outside of any loops, in this case we break immediately after (so the compile is only ever performed once), and in fact 99.9% of the time we don't even perform the compile, since we don't match "REPORTING UNCAUGHT EXCEPTION" or "FATAL EXCEPTION". Therefore moving the compile to before the loop means we do extra work in the majority case (albeit so little that we wouldn't actually care) - and I think it improves readability to keep the compile near to where the regexp is being matched. However, will move if you really object :-)

> @@ +185,5 @@
> > +            logre = re.compile(r".*\): \t?(.*)")
> > +            m = logre.search(logcat[i+1])
> > +            if m and m.group(1):
> > +                top_frame = m.group(1)
> > +            m = logre.search(logcat[i+2])
> 
> These aren't checking that i+1 or i+1 < len(logcat).

Added a check / warning if the logcat was truncated. Pretty jetlagged right now, but |len(logcat) < i + 3| should be right I think.

Comment 7

[Ed Morley [:emorley]]

(In reply to Ed Morley (Away until 9th Oct) [:edmorley UTC+1] from comment #6)
> Added a check / warning if the logcat was truncated. Pretty jetlagged right
> now, but |len(logcat) < i + 3| should be right I think.

I meant |len(logcat) >= i + 3|

Comment 8

[Ed Morley [:emorley]]

Ty :-)

https://github.com/mozilla/mozbase/commit/fd8d2cc2926346d67645fc24f2a2dd0aad5d73ed