Closed Bug 923500 Opened 7 years ago Closed 7 years ago

Add check_for_java_exception() to mozcrash

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: emorley, Assigned: emorley)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #813592 - Flags: review?(ted)
Blocks: 923513
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).
Attachment #813653 - Flags: review?(jmaher)
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
Attachment #813654 - Flags: review?(jmaher)
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.
Attachment #813653 - Attachment is obsolete: true
Attachment #813653 - Flags: review?(jmaher)
Attachment #813654 - Attachment is obsolete: true
Attachment #813654 - Flags: review?(jmaher)
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).
Attachment #813592 - Flags: review?(ted) → review+
Attached patch Patch v2Splinter Review
(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.
Attachment #813592 - Attachment is obsolete: true
Attachment #814400 - Flags: review?(ted)
(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|
Attachment #814400 - Flags: review?(ted) → review+
Blocks: 924931
Depends on: 926408
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 927119
You need to log in before you can comment on or make changes to this bug.