848102 - CheckForLastRunCrash shouldn't move dump if MOZ_CRASHREPORTER_NO_REPORT is set

Status: RESOLVED INCOMPLETE

(Toolkit :: Crash Reporting, defect)

Description

[Andrew Halberstadt [:ahal]]

On B2G minidumps get submitted even if MOZ_CRASHREPORTER_NO_REPORT is set.

I'm not very familiar with breakpad, but from what I understand from ted.. B2G calls [1] from [2] which does [3]. The b2g automated tests set MOZ_CRASHREPORTER_NO_REPORT, but the minidumps get moved anyway meaning the harness can't use them with minidump_stackwalker.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#2390
[2] http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#108
[3] http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#2435

Comment 1

[Jonathan Griffin (:jgriffin)]

We're not really setting MOZ_CRASHREPORTER_NO_REPORT for B2G, because the way we launch B2G causes us to ignore environment variables.

We launch B2G using b2g.sh: http://mxr.mozilla.org/mozilla-central/source/build/mobile/b2gautomation.py#314

In order to set environment variables that would actually affect the B2G process, we'd have to modify this file before executing it.

Is there a pref we could set that would have the same effect?

Comment 2

[Andrew Halberstadt [:ahal]]

I think we should at least be able to work around this problem (filed bug 848124). I don't really care if we use the environment variable, a pref or whatever. But both this bug and bug 848124 are things that should be fixed whether or not they break b2g minidump stackwalking so I'm inclined to go the route that fixes them along the way.

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - don't copy minidumps to pending directory if MOZ_CRASHREPORTER_NO_REPORT is set

Pretty sure this is all we need. I don't know how to test this on b2g, but at least it doesn't change behaviour for desktop

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.0 - don't copy minidumps to pending directory if MOZ_CRASHREPORTER_NO_REPORT is set

Actually the call to GetExtraFileForMinidump should also be wrapped in the if statement.

Comment 5

[Andrew Halberstadt [:ahal]]

I managed to test this out with b2g. It looks like the b2g process is not automagically restarted which means the minidumps remain in profile/minidumps after the test run. They do however get removed from profile/minidumps the next time b2g.sh is run (even if MOZ_CRASHREPORTER_NO_REPORT is set).

So I *think* this bug doesn't actually block bug 843296, but it should still be fixed.

Comment 6

[Andrew Halberstadt [:ahal]]

I'll leave it up to ted whether this patch is still worth taking or not.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 721841 [details] [diff] [review]
Patch 2.0 - don't copy minidumps to pending directory if MOZ_CRASHREPORTER_NO_REPORT is set

Normally I would be all about fixing this for the sake of correctness, but if we don't need this for automation let's just leave it be. We can always revisit if need be.