check_for_crashes should be replaced with log_crashes wherever structured logging is in use

RESOLVED FIXED in mozilla35

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

(Blocks: 1 bug)

unspecified
mozilla35
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
This will be an unstructured side channel for failure output until it is, so is required for the mozharness refactor. Looks like a drop in replacement, but we'll have to detect whether structured logging is available for the mozrunner case.

What's the way to manufacture a crash to test this out?
I force crashes by using a null pointer deref in chrome js:

Cu.import("resource://gre/modules/ctypes.jsm");
let zero = new ctypes.intptr_t(8);
let badptr = ctypes.cast(zero, ctypes.PointerType(ctypes.int32_t));
badptr.contents;

(this is the kind of thing that we could put into a testing metaharness).
There's also CrashTestUtils.jsm, but that's mostly if you need specific kinds of crashes:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/CrashTestUtils.jsm?force=1

jgraham's method will work fine.
(Assignee)

Comment 3

4 years ago
Not a drop in replacement, because check_for_crashes has a return value used by the harnesses. log_crashes seems clearly intended to have a side effect so probably should be renamed, or mozcrash should expose this elsewhere.
(Assignee)

Comment 4

4 years ago
Created attachment 8492234 [details] [diff] [review]
Provide an indication whether a crash was logged by log_crashes

I imagine someone has an opinion about this.
Attachment #8492234 - Flags: review?(ted)
Attachment #8492234 - Flags: feedback?(james)
(Assignee)

Updated

4 years ago
Assignee: nobody → cmanchester
Status: NEW → ASSIGNED
Comment on attachment 8492234 [details] [diff] [review]
Provide an indication whether a crash was logged by log_crashes

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

I don't know what the use case is here, but using a stateful API seems rather wonky. If we need to know whether the current log_crashes() actually logged anything it could return a boolean:

diff --git a/testing/mozbase/mozcrash/mozcrash/mozcrash.py b/testing/mozbase/mozcrash/mozcrash/mozcrash.py
index c41c16c..204ab17 100644
--- a/testing/mozbase/mozcrash/mozcrash/mozcrash.py
+++ b/testing/mozbase/mozcrash/mozcrash/mozcrash.py
@@ -116,11 +116,14 @@ def log_crashes(logger,
                 stackwalk_binary=None,
                 dump_save_path=None):
     """Log crashes using a structured logger"""
+    logged_crash = False
     for info in CrashInfo(dump_directory, symbols_path, dump_save_path=dump_save_path,
                           stackwalk_binary=stackwalk_binary):
         kwargs = info._asdict()
         kwargs.pop("extra")
         logger.crash(process=process, test=test, **kwargs)
+        logged_crash = True
+    return logged_crash
Attachment #8492234 - Flags: feedback?(james) → feedback-
(Assignee)

Comment 6

4 years ago
(In reply to James Graham [:jgraham] from comment #5)
> Comment on attachment 8492234 [details] [diff] [review]
> Provide an indication whether a crash was logged by log_crashes
> 
> Review of attachment 8492234 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know what the use case is here, but using a stateful API seems
> rather wonky. If we need to know whether the current log_crashes() actually
> logged anything it could return a boolean:
> 
> diff --git a/testing/mozbase/mozcrash/mozcrash/mozcrash.py
> b/testing/mozbase/mozcrash/mozcrash/mozcrash.py
> index c41c16c..204ab17 100644
> --- a/testing/mozbase/mozcrash/mozcrash/mozcrash.py
> +++ b/testing/mozbase/mozcrash/mozcrash/mozcrash.py
> @@ -116,11 +116,14 @@ def log_crashes(logger,
>                  stackwalk_binary=None,
>                  dump_save_path=None):
>      """Log crashes using a structured logger"""
> +    logged_crash = False
>      for info in CrashInfo(dump_directory, symbols_path,
> dump_save_path=dump_save_path,
>                            stackwalk_binary=stackwalk_binary):
>          kwargs = info._asdict()
>          kwargs.pop("extra")
>          logger.crash(process=process, test=test, **kwargs)
> +        logged_crash = True
> +    return logged_crash

The use case is setting the harness return code (or just return value from runTests) based on a crash. I'm not that invested in the exact approach taken, but in defense of this approach the api already seems pretty stateful because processing a dump has the side effect of removing the original dump files (and optionally saving them). Returning a value from a method called "log_crashes" is a little wonky, too.
Comment on attachment 8492234 [details] [diff] [review]
Provide an indication whether a crash was logged by log_crashes

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

That's gross. Why not just make log_crashes return a count of crashes it logged?
Attachment #8492234 - Flags: review?(ted) → review-
(Assignee)

Comment 8

4 years ago
Created attachment 8493273 [details] [diff] [review]
Provide an indication whether a crash was logged by log_crashes
Attachment #8493273 - Flags: review?(ted)
(Assignee)

Updated

4 years ago
Attachment #8492234 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 8493276 [details] [diff] [review]
Trivial fixes for formatting crashes in the tbplformatter.

A bug fix and simple modification for compatibility with existing output.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=f9c11ae4b5ab
Attachment #8493276 - Flags: review?(james)
(Assignee)

Comment 10

4 years ago
Created attachment 8493315 [details] [diff] [review]
Log crashes with structured logging for those using structured logging.

Logging some crashes:
https://tbpl.mozilla.org/?tree=Try&rev=6b8a149a7b80
Attachment #8493315 - Flags: review?(ahalberstadt)
Comment on attachment 8493315 [details] [diff] [review]
Log crashes with structured logging for those using structured logging.

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

Looks good, a couple comments that apply to all modified files.

::: build/mobile/b2gautomation.py
@@ +117,5 @@
>              try:
> +                logger = get_default_logger()
> +                if logger is not None:
> +                    crash_count = mozcrash.log_crashes(logger, local_dump_dir, symbolsPath,
> +                                                       test=self.lastTestSeen)

Can we save the arguments in a mozcrash_args variable so they only need to be updated in one place?

@@ +118,5 @@
> +                logger = get_default_logger()
> +                if logger is not None:
> +                    crash_count = mozcrash.log_crashes(logger, local_dump_dir, symbolsPath,
> +                                                       test=self.lastTestSeen)
> +                    crashed = crash_count > 0

It's probably ok to just set crashed to the return value of log_crashes.
Attachment #8493315 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 12

4 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #11)
> Comment on attachment 8493315 [details] [diff] [review]
> Log crashes with structured logging for those using structured logging.
> 
> Review of attachment 8493315 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, a couple comments that apply to all modified files.
> 
> ::: build/mobile/b2gautomation.py
> @@ +117,5 @@
> >              try:
> > +                logger = get_default_logger()
> > +                if logger is not None:
> > +                    crash_count = mozcrash.log_crashes(logger, local_dump_dir, symbolsPath,
> > +                                                       test=self.lastTestSeen)
> 
> Can we save the arguments in a mozcrash_args variable so they only need to
> be updated in one place?

Maybe I don't see what you mean, but the argument names aren't identical, so I don't think so.

> 
> @@ +118,5 @@
> > +                logger = get_default_logger()
> > +                if logger is not None:
> > +                    crash_count = mozcrash.log_crashes(logger, local_dump_dir, symbolsPath,
> > +                                                       test=self.lastTestSeen)
> > +                    crashed = crash_count > 0
> 
> It's probably ok to just set crashed to the return value of log_crashes.
Attachment #8493276 - Flags: review?(james) → review+
Comment on attachment 8493273 [details] [diff] [review]
Provide an indication whether a crash was logged by log_crashes

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

Looks good, thanks!
Attachment #8493273 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/37935ebab1d1
https://hg.mozilla.org/mozilla-central/rev/8b6aebc85b17
https://hg.mozilla.org/mozilla-central/rev/1244ab6f3d0e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.