mozcrash.py: check_for_crashes() and check_for_java_exception() should return results

NEW
Unassigned

Status

Testing
Mozbase
3 years ago
3 years ago

People

(Reporter: bc, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox41 affected)

Details

(Reporter)

Description

3 years ago
In mozcrash.py, check_for_crashes() and check_for_java_exception() print the PROCESS-CRASH and minidump_stackwalk to stdout if quiet = False but do not . This precludes its reuse in other frameworks where printing to stdout is inappropriate. It would be useful if they returned the output so that consumers could handle it as needed.
I think the CrashInfo object that jgraham implemented as part of the structured logging work will do what you want:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozcrash/mozcrash/mozcrash.py#129

You can look at the implementation of check_for_crashes / log_crashes above that to see how it works.

It looks like we never updated the documentation when we made that change unfortunately:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/docs/mozcrash.rst
(Reporter)

Comment 2

3 years ago
CrashInfo isn't exported, but even if it were I'd end up reimplementing check_for_crashes() and check_for_java_exception(). If check_for_crashes() returned an empty list if no crashes were found or if crashes were found it returned a list like:

        [
          {
            'reason': 'PROCESS-CRASH',
            'signature': 'libmm-color-convertor.so + 0x1232',
            'stackwalk_output': '...',
            'stackwalk_errors': '...'
          },
        ]

and if check_for_java_exception() returned the exception as a string or the empty string if none were found, then I could pass quiet=True to both and reuse them.

That would probably be compatible with the code that consumes the return values which currently assumes a Boolean return value.
If you pass in quiet=True to check_for_crashes it just builds a CrashInfo object, and sets the return value. So I don't think there's much of anything to re-implement there.

You might also be able to (ab)use the log_crashes method that takes a "logger" object with a single "crash" method and calls it for each detected crash. That's how the structured logging works.

So, in general I think I am in favour of exposing CrashInfo, but, without knowing your exact requirements, I think there is already something to work with here (I know nothing about the java stuff).
(Reporter)

Comment 4

3 years ago
I don't parse logs to find crashes and don't rely on Treeherder to parse the logs either so emitting the crashes to a log doesn't help me.

In check_for_crashes if we moved the |if not quiet| at https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozcrash/mozcrash/mozcrash.py#92 to only be around the print statements and constructed and returned the list of objects I described in comment 2 regardless of the quiet value, it would work for me.

Changing the check_for_java_exception to return the exception instead of a boolean would be sufficient for me.

Just exposing CrashInfo still leaves me having to reimplement part of the logic in mozcrash.py which I thought we didn't like. It would certainly allow more flexibility in reusing mozcrash but doesn't alleviate the tying of mozcrash to log output/parsing.
That seems totally reasonable. `check_for_crashes` is returning a boolean right now, but if we made it return a list I think all of the existing callers would probably work without error.
Hmm, that sounds like it would be non-trivial to check since most consumers don't actively expect to crash, so you only find out they got broken at some random later time. So I'm dubious about making this kind of change.

In any case I think that if the log_crashes method doesn't do what you want (and I still think it does despite the name and somewhat unusual API), you'd be better off with an iterator than a list.
(Reporter)

Comment 7

3 years ago
jgraham explained how to use log_crashes as an api adapter. That does seem in contrast to its naming but would be workable for the check_for_crashes replacement.

I realize that I'm not stating what I'm really looking for but even using log_crashes I would end up still replicating the code beginning at https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozcrash/mozcrash/mozcrash.py#93 in autophone just like https://github.com/mozilla/autophone/blob/master/autophonecrash.py#L307

I guess what I want is to not replicate any code from mozcrash. Instead of returning a list of objects as described above, if we returned a list like
                [
                {'error_line': "application crashed [%s]" % signature
                 'reason': 'PROCESS-CRASH',
                 'signature': signature,
                 'stackwalk_output': '\n'.join(stackwalk_output),
                 'stackwalk_errors': '\n'.join(info.stackwalk_errors)},
                ]

then I could use check_for_crashes without needing to replicate any code from mozcrash.

(In reply to James Graham [:jgraham] from comment #6)
> Hmm, that sounds like it would be non-trivial to check since most consumers
> don't actively expect to crash, so you only find out they got broken at some
> random later time. So I'm dubious about making this kind of change.
> 

How would they break returning an empty list or empty string (for the java part) instead of False or a non-empty list or non-empty string instead of True unless they are checking against True or False?

> In any case I think that if the log_crashes method doesn't do what you want
> (and I still think it does despite the name and somewhat unusual API), you'd
> be better off with an iterator than a list.

Why?
FWIW my opinion is that mozcrash already does too much text formatting and I would happily rip out all of check_for_crashes if there weren't legacy consumers. The exact strings used to format output don't seem like a concern of the crash processing code.

> How would they break returning an empty list or empty string (for the java part) instead of False or a non-empty list or non-empty string instead of True unless they are checking against True or False?

They could well be testing against True or False. 

> Why?

Because processing the crashes lazily doesn't obviously have any disadvantages but seems better for memory consumption than building up a list of everything.
You need to log in before you can comment on or make changes to this bug.