Closed Bug 867829 Opened 6 years ago Closed 6 years ago

Crash dump get queued and eventually sent if we asked to not send them

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE5
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: hub, Assigned: hub)

References

Details

(Whiteboard: c= p=2, QARegressExclude)

Attachments

(3 files, 2 obsolete files)

Crash dump get queued and eventually sent if we asked to not send them
See bug 836071 comment 11

We should delete the dumps if we said to not send them.

Or eventually moved away from the pending queue.
Assignee: nobody → hub
I'm also torn on this. It makes sense to still have them locally in case debugging is actually needed, OTOH, those people explicitely opted out from getting their crashes debugged.

I wonder if we want/need the privacy team to weigh in here.
We also have limits put to not clog the device storage, so keeping them too long isn't really appropriate.
Whiteboard: c=crashes
Whiteboard: c=crashes → c=performance
When crash reports are enabled, we already limit the queue length, from all I know (which might even come from breakpad itself), does that not work when it's disabled?
My concern is that as soon as the "send crash report" is enabled and we crash all these queued reports will be sent too. 

IMHO this is not right.
Proposed patch, albeit not sure what's the proper action we want here.
see also bug 836081
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #3)
> When crash reports are enabled, we already limit the queue length, from all
> I know (which might even come from breakpad itself), does that not work when
> it's disabled?

We still remove the old ones. The problem is if the user do enable "send crash reports" the one that were created when she didn't want will be sent to.

I think we should really just delete.
Comment on attachment 745200 [details] [diff] [review]
Bug 867829 - Delete crash dumps if we disabled reporting.

Do you agree on deleting the dump when we ask to not send it?

Thanks
Attachment #745200 - Flags: feedback?(kairo)
Comment on attachment 745200 [details] [diff] [review]
Bug 867829 - Delete crash dumps if we disabled reporting.

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

I think someone deactivating it and still want to have dumps for debugging is enough of an edge case that we can ignore it, though it would somehow be nice if we could take that into account.

::: toolkit/crashreporter/CrashSubmit.jsm
@@ -134,4 @@
>        return -1;
>      if (dateB < dateA)
>        return 1;
> -    return 0;

That change doesn't sounds right (it's no code change anyhow but I think the indentation was right before). ;-)
Attachment #745200 - Flags: feedback?(kairo) → feedback+
Good catch. This indent change shouldn't have happened. How didn't I see it. I'll update the patch and put it up for review.
Attachment #745200 - Attachment is obsolete: true
Attachment #746980 - Flags: review?(fabrice)
Attachment #746980 - Flags: review?(benjamin)
Comment on attachment 746980 [details] [diff] [review]
Bug 867829 - Delete crash dumps if we disabled reporting.

I'm a little worried about this. IIRC, FHR was trying to use counts of minidumps to report on what percentage of crash reports were actually being submitted. This has the potential to screw up that calculation, unless we're also keep raw counts somewhere else. f?gps, because I don't remember what FHR is doing right now

B2G is in this weird situation where we auto-submit reports after, rather than just saving them so the user could resubmit them via about:crashes. I'm not completely aware of what the B2G UI looks like, but could we instead of deleting these completely, could we annotate the ones that we would have auto-submitted and only submit those via the automatic mechanism? That way manual submission would still be available via about:config or whatever the B2G equivalent is.
Attachment #746980 - Flags: feedback?(gps)
Note that on desktop if you hit a browser crash and decline to send it we delete it immediately. Plugin crashes just sit in pending if you don't send them, but we have no auto-resubmit process.
Comment on attachment 746980 [details] [diff] [review]
Bug 867829 - Delete crash dumps if we disabled reporting.

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

FHR does not currently run on B2G, but that very likely won't always be true. While this won't affect FHR today, it has implications for the future.

FHR's crash reporting mechanism has known wonkiness and is far from bullet-proof. I would ideally like to see the crash reporting service (or some entity within Gecko) expose a "log" of crash info that FHR can simply read from. Contrast with today, where FHR is poking at crash files in the profile directory directly. In doing so, FHR is fragile to changes in how crash reports are saved and prone to breakage - especially considering we don't have excellent test coverage of FHR crash reporting because, well, testing crashes is hard.
Attachment #746980 - Flags: feedback?(gps)
Also it should be noted that we already have an algorithm in place to remove all but the 10 most recent crashes that haven't been sent.
Given what Ted and gps said, I think deleting is OK and we should have Breakpad do a (rotating? size-limited?) log of when we crashed or something to feed that into FHR.

That said, gps, testing crashes on desktop is easy, QA does it all the time with a nice add-on that Ted once created that induces crashes. ;-)
Blocks: 833574
Note that if you ask the user what to do we still don't delete the crash dump. Therefor, I have a part 2 coming, but it involve some other changes in Gaia as well ; it will be split.
Status: NEW → ASSIGNED
Whiteboard: c=performance → c=
Whiteboard: c= → c= p=2
Gaia changes to have a callback for dismissing and send the proper event.
Comment on attachment 748246 [details] [diff] [review]
Bug 867829 - Part 3: receive the delete-crash event to delete crash dump when user declined.

We need part 2 in Gaia for this to work.
Attachment #748246 - Flags: review?(fabrice)
Comment on attachment 748244 [details] [diff] [review]
Bug 867829 - Part 2: have a dismiss callback for the system banner so we can know the user didn't want to send crash report.

Vivien, if you are not the proper reviewer, let me know who is. Thanks.
Attachment #748244 - Flags: review?(21)
Part 2 is available as a pull request https://github.com/mozilla-b2g/gaia/pull/9729
Attachment #746980 - Flags: review?(benjamin) → review+
Attachment #746980 - Flags: review?(fabrice) → review+
Comment on attachment 748246 [details] [diff] [review]
Bug 867829 - Part 3: receive the delete-crash event to delete crash dump when user declined.

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

::: b2g/chrome/content/shell.js
@@ +1107,3 @@
>        shell.submitCrash(e.detail.crashID);
>      }
> +    else if (e.detail.type == "delete-crash" && e.detail.crashID) {

nit : } else if { ...

Also, use double quotes everywhere for strings.
Attachment #748246 - Flags: review?(fabrice) → review+
Comment on attachment 748244 [details] [diff] [review]
Bug 867829 - Part 2: have a dismiss callback for the system banner so we can know the user didn't want to send crash report.

I feel like Margaret know this code much better than me.
Attachment #748244 - Flags: review?(21) → review?(margaret.leibovic)
Comment on attachment 748244 [details] [diff] [review]
Bug 867829 - Part 2: have a dismiss callback for the system banner so we can know the user didn't want to send crash report.

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

Pretty straightforward change, looks good to me.

::: apps/system/js/system_banner.js
@@ +4,5 @@
>    banner: document.getElementById('system-banner'),
>  
>    // Shows a banner with a given message.
> +  // Optionally shows a button with a given label/callback/dismiss.
> +  // 'dismiss' is called when the banner is dismissed. It is optionnal.

Nit: s/optionnal/optional.

It seems odd for dismiss to be part of buttonParams, since it's not related to the button (it's called regardless of whether the button is pressed or not). However, this is probably the simplest way to add this feature without changing the API, so I suppose it's fine.
Attachment #748244 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 748244 [details] [diff] [review]
Bug 867829 - Part 2: have a dismiss callback for the system banner so we can know the user didn't want to send crash report.

Also, are mozContentEvents guaranteed to be received in order? We wouldn't want to introduce a race here that could cause a crash to be deleted before it is submitted. Perhaps this patch could be improved to avoid calling deleteCrash if submitCrash has been called (or avoid calling dismiss() if callback() has been called).
https://hg.mozilla.org/mozilla-central/rev/e561be22a3b2
https://hg.mozilla.org/mozilla-central/rev/8e40c3907e2d

Friendly reminder that B2G work should be landing on birch, not inbound :)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Part 2 is still missing because it is gaia. Will close once landed there too.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Address Margaret comment about the mozContent events. Made it an either / or situation.

new pull request is https://github.com/mozilla-b2g/gaia/pull/9849
Attachment #748244 - Attachment is obsolete: true
Attachment #751048 - Flags: review?(margaret.leibovic)
Filed bug 873530 as tapping on the "Report" button fails.
Comment on attachment 751048 [details] [diff] [review]
Part 2: have a dismiss callback for the system banner so we can know the user didn't want to send crash report.

Nice, this is better.
Attachment #751048 - Flags: review?(margaret.leibovic) → review+
Part 2 got merge in the gaia repository.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
As STR of bug 836071#c0, I'm a little worried about this for b2g18(V1.1) also.
of course, it is abnormal behavior that b2g killed by SIGKILL. but, it also happens suddenly removal of battery at this moments. (i think it is more closes user case.) 
if it make a happens, crash reports sends to server, although user already choose "Ask me when a crash occurs" or "Never send"
i thinks it's need to land on b2g18(V1.1) and v1-trains. how do u thinks [:jsmith] ?
Flags: needinfo?(jsmith)
Going to redirect the question over to Naoki, as he is the crash reporting expert on the QA team.
Flags: needinfo?(jsmith) → needinfo?(nhirata.bugzilla)
See Also: → 900313
See Also: 900313
After further testing, I saw what you saw and reported Bug 900595
Flags: needinfo?(nhirata.bugzilla)
Given that this causes bug 900588 and bug 900595 on 1.1, which one could even consider a privacy issue (reports containing private data being sent when the user opted out), I'm nominating this. Also, leo seems to very much want this fix as well to be able to run crash reporting correctly.
blocking-b2g: --- → leo?
Block for privacy issues - get this uplifted to v1.1.
blocking-b2g: leo? → leo+
Duplicate of this bug: 900588
Uplifted 9357244ac6d792c236c755ef7c81a272b615554b to:
v1-train: c4246df6659734dedde3d1514fd17d6b01e7935f
You set status-b2g18 to fixed, but I see no mention of the two gecko patches being uplifted....
Flags: needinfo?(jhford)
(In reply to Hubert Figuiere [:hub] from comment #43)
> You set status-b2g18 to fixed, but I see no mention of the two gecko patches
> being uplifted....

The uplifting tool does that automatically, reset the flag.
Flags: needinfo?(jhford)
Needs branch-specific patches for the b2g18 uplift.
Flags: needinfo?(hub)
Target Milestone: --- → 1.1 QE5
OK, I'll make them and checkin.
Flags: needinfo?(hub)
v1.1.0hd: c4246df6659734dedde3d1514fd17d6b01e7935f
Whiteboard: c= p=2 → c= p=2, QARegressExclude
You need to log in before you can comment on or make changes to this bug.