CheckForLastRunCrash shouldn't move dump if MOZ_CRASHREPORTER_NO_REPORT is set

RESOLVED INCOMPLETE

Status

()

RESOLVED INCOMPLETE
6 years ago
6 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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
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?
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
Created attachment 721837 [details] [diff] [review]
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
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Attachment #721837 - Flags: review?(ted)
(Assignee)

Comment 4

6 years ago
Created attachment 721841 [details] [diff] [review]
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.
Attachment #721837 - Attachment is obsolete: true
Attachment #721837 - Flags: review?(ted)
Attachment #721841 - Flags: review?(ted)
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
I'll leave it up to ted whether this patch is still worth taking or not.
No longer blocks: 843296
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.
Attachment #721841 - Flags: review?(ted)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.