1427104 - Allow Firefox to send crash reports to Mozilla option not working

Status: REOPENED

(Firefox :: Settings UI, defect, P3)

Description

[Ralph Day]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171206182557

Steps to reproduce:

1. Options | Privacy & Security | Firefox Data Collection and Use
Uncheck "Allow Firefox to send crash reports to Mozilla"
2. Browse until you get a Firefox crash.


Actual results:

Crash reporter dialog comes up with the send crash report to Mozilla box checked.
If you don't notice and click Restart Firefox the crash report will be sent to Mozilla.


Expected results:

Crash reporter dialog should not have the send crash report to Mozilla box checked at a minimum when "Allow Firefox to send crash reports to Mozilla" is unchecked.  Probably should also be greyed out as the option is set to not allow crash reports to be sent.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Lonnen, is this a dupe of bug 1427111?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Lonnen, is this a dupe of bug 1427111?

I don't think it is.

I think this is confusion about what the two checkboxes mean - we have 3 checkboxes to consider. 1 is in about:preferences, and 2 are in about:tabcrashed.

The one in about:preferences is "Allow Firefox to send crash reports to Mozilla", which I'll call A.

The ones in about:tabcrashed are:

"Send an automated crash report so we can fix issues like this.", which I'll call B

and

"Update preferences to automatically submit reports when Firefox crashes.", which I'll call C.


A and C actually control the same preference under the hood - browser.crashReports.unsubmittedCheck.autoSubmit (now browser.crashReports.unsubmittedCheck.autoSubmit2 as of 57.0.3).

B controls browser.tabs.crashReporting.sendReport.

I think the reporter is concerned that A is not controlling the pref / state for B.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Tentatively re-opening and de-duping. I don't think these are the same bugs.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Okay, thanks for the explanation and for doing the work to figure that out. I'm going to mark this as P1 since it looks like it can cause sending of data when the user has explicitly requested not to.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]: This may be too late for 58, and I'm not sure when this was introduced but it seems like something we should at least track for 58 or 59 so we can make sure this gets fixed sooner rather than later.

Comment 7

[Gerry Chang [:gchang]]

Per comment #6, track 58+/59+.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

jaws, can you help find someone to work on this? thanks.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Ted, I think this would need to be fixed in the Crash Reporter. Would you be able to take this or redirect it?

Comment 10

[(not currently active) Ted Mielczarek]

I haven't been doing work on the crash reporter client lately. gsvelto has done some work, and mconley has done work on the browser chrome bits. Either of them would be a good choice to fix this.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

I can take this.

Comment 12

[Gabriele Svelto [:gsvelto]]

Chiming in because I fiddled with the crashreporter code a lot. I believe this is a regression; I remember that preference did work correctly in the past, probably *before* we had content crash auto-submission. The way it worked was remarkably roundabout though, because it set stuff outside of the Gecko preferences (e.g. in the Windows registry) since the crashreporter client could not read the prefs. Tomorrow I'll dig out the old code and verify when the behavior was changed. NI?ing me too to not forget about this.

Comment 13

[Gabriele Svelto [:gsvelto]]

After some digging I've unearthed the first thing that looks fishy. We use the nsICrashReporter.submitReports field to enable/disable submission from the crash reporter client. Setting the value triggers a complex, platform-dependent implementation that communicates the pref to the crashreporter client. However that value is never touched on Firefox, the only user remaining in the code is Fennec:

https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/mobile/android/chrome/content/browser.js#2078

I don't have time today but I'll try running a bisection next week to figure out when setting this parameter was removed from the desktop preferences. I seem to remember this was still working when we had the London all-hands but I haven't tried it since. Mike, if you have some spare cycles and can run the bisection before next week you know where to look :)

Comment 14

[Mike Kaply [:mkaply]]

It was done as part of this bug - https://bugzilla.mozilla.org/show_bug.cgi?id=1287178

Comment 15

[anhtuanho]

(In reply to Gabriele Svelto [:gsvelto] from comment #12)
> Chiming in because I fiddled with the crashreporter code a lot. I believe
> this is a regression; I remember that preference did work correctly in the
> past, probably *before* we had content crash auto-submission. The way it
> worked was remarkably roundabout though, because it set stuff outside of the
> Gecko preferences (e.g. in the Windows registry) since the crashreporter
> client could not read the prefs. Tomorrow I'll dig out the old code and
> verify when the behavior was changed. NI?ing me too to not forget about this.

maybe this's my problems . I'm sorry .i'm just click on guidelines , i will comeback and sorry

Comment 16

[Mike Conley (:mconley) (:⚙️)]

mkaply's right - bug 1287178 is where changes started, but I don't think that's the full story. Here are my findings.

In bug 12487178, I noticed[1] that the "Enable crash reporter" didn't actually toggle the enabled state of the crash reporter. Instead, what it did was set the default state of the "Send crash report to Mozilla" checkbox in the crash reporter client.

So, I:

1) Changed the string in about:preferences to be: "Allow &brandShortName; to send backlogged crash reports on your behalf". This is the string I worked out with both legal and mheubusch.
2) Made it so that the checkbox controls browser.crashReports.unsubmittedCheck.autoSubmit only.

So, in sum, the preference to set the default state of "Send crash reports to Mozilla" was removed from about:preferences (although still exists in about:config), and instead we exposed the preference for sending background tab crash reports to Mozilla.


Then, in bug 1365133, the string was changed back to:

Allow &brandShortName; to send crash reports to Mozilla


I'm not sure if that was intentional or an oversight.

TL;DR:

But sum those two bugs together, and what you get is a checkbox in about:preferences that only controls the state of whether or not we submit background crash reports, doesn't affect the default "Send crash report to Mozilla" checkbox in the crash reporter client or about:tabcrashed page, and has the string:

"Allow &brandShortName; to send crash reports to Mozilla"


Next steps:

We need to figure out what we want to actually expose here to users, and how we want to phrase them once we know what they are. Exactly what preferences do we want to give users wrt crash reports? Who makes that decision?

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Hey lonnen, you were suggested as someone who might be shot-caller on this. Please let me know if I've got the wrong person!

Right now we've got a confusingly named checkbox in about:preferences that has the string:

"Allow &brandShortName; to send crash reports to Mozilla"

but actually controls whether or not background tab crashes can be reported automatically to Mozilla.

That's pretty confusing. One thing we could easily do is change the string (back) to:

"Allow &brandShortName; to send backlogged crash reports on your behalf"

which would be a much better description for what that checkbox actually controls.

However, there's maybe an open question here of whether or not we want to expose more options to the user here regarding crash reporter behaviour. The sorts of additional things we might want to expose are:

1) The enabled/disabled state of the entire crash reporter itself. I'm not entirely sure this can be controlled by a pref - I know it can be controlled by an environment variable (see bug 1429152), but we could change that.

2) The default state of the "send report to Mozilla" checkboxes in both the tab and main process crash reporter.

Do you feel like we ought to expose these to users? If so, I can try to work with UX to get the language and checkbox arrangement ironed out.

If, however, we decide to keep the single option exposed, we can just change the string as I laid out earlier in this comment.

Comment 18

[Mike Kaply [:mkaply]]

> "Allow &brandShortName; to send backlogged crash reports on your behalf"

I think this string is still confusing because the main crash reporter can have backlogged crash reports as well.

This really on controls tab crashes right? Not browser crashes?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Kaply [:mkaply] from comment #18)
> This really on controls tab crashes right? Not browser crashes?

For Release and late betas, it's just for tab crashes.

For EARLY_BETA_OR_EARLIER, it _might_ include browser crashes, because in those releases, shortly after start-up, we'll scan the crashes directory for any crashes that haven't been submitted yet - and if this checkbox is checked, we'll send them.

Comment 20

[Gabriele Svelto [:gsvelto]]

Just my 2p

(In reply to Mike Conley (:mconley) (:⚙️) from comment #17)
> 1) The enabled/disabled state of the entire crash reporter itself. I'm not
> entirely sure this can be controlled by a pref - I know it can be controlled
> by an environment variable (see bug 1429152), but we could change that.

For a regular user I don't think this is desirable. If Firefox crashes and the crash reporter is entirely disabled there's no way for the user to tell what just happened since the crash reporter client is not shown.

> 2) The default state of the "send report to Mozilla" checkboxes in both the
> tab and main process crash reporter.

I think this is the most sensible choice, and it's the old behavior so it shouldn't "surprise" our users. With the option off the crash reporter client would start with all controls grayed out except for the Restart and Quit buttons. The other controls are enabled only if the user explicitly checks the "Submit crash report" checkbox. 

> Do you feel like we ought to expose these to users? If so, I can try to work
> with UX to get the language and checkbox arrangement ironed out.
> 
> If, however, we decide to keep the single option exposed, we can just change
> the string as I laid out earlier in this comment.

I believe that having the old option back is a good idea. We've already had quite a bit of pushback from privacy-conscious users for removing the FHR option so I see no harm in offering them more control.

Comment 21

[Lonnen :lonnen]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #17)
> Hey lonnen, you were suggested as someone who might be shot-caller on this.
> Please let me know if I've got the wrong person!


I'm not a Firefox peer, but I have been adjacent to the crash reporter a long time. I'm not sure who would be the best person. I've asked around and there isn't a clear owner -- Gabriele is probably most familiar. I believe there's a discussion among eng manager's happening now about resourcing it with a clear owner.

> ...

> The sorts of additional things we might want to expose are:
> 
> 1) The enabled/disabled state of the entire crash reporter itself. I'm not
> entirely sure this can be controlled by a pref - I know it can be controlled
> by an environment variable (see bug 1429152), but we could change that.

I expect something like this, with "always, ask me, never" options as the primary or only setting. We'd need to figure out the inter-process communication.

> 2) The default state of the "send report to Mozilla" checkboxes in both the
> tab and main process crash reporter.

Perhaps confusing to have it in prefs, and then prompt again.
 
> Do you feel like we ought to expose these to users? If so, I can try to work
> with UX to get the language and checkbox arrangement ironed out.

I favor easier to explain, fewer user decisions, and following other patterns. It's difficult to express "always send some types of crashes, some time, if you haven't already sent them." If we think it makes a big difference we have the means to test language and implementation -- surveys, SHIELD, etc.

> If, however, we decide to keep the single option exposed, we can just change
> the string as I laid out earlier in this comment.


For context, I'm also thinking ahead about https://bugzilla.mozilla.org/show_bug.cgi?id=1426479 where Osmose has been looking at collecting JS errors. These would need similar language, and could possibly rely on the same prefs, but have a totally different user experience because they happen constantly and are usually much less disruptive.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Thanks, lonnen.

ni?ing myself to not let this slip off my radar.

Comment 23

[(not currently active) Ted Mielczarek]

For some historical context: the crashreporter client saves the value of the "Send crash reports to Mozilla" checkbox in various OS-specific locations:
* ~/.mozilla/firefox/Crash Reporter/crashreporter.ini on Linux:
  https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter_linux.cpp#39
  https://dxr.mozilla.org/mozilla-central/rev/0d806b3230fe4767fa70cdee57db87d1e9a5ba49/toolkit/crashreporter/client/crashreporter_gtk_common.cpp#357
* The Windows registry on Windows:
  https://dxr.mozilla.org/mozilla-central/rev/0d806b3230fe4767fa70cdee57db87d1e9a5ba49/toolkit/crashreporter/client/crashreporter_win.cpp#920
* In NSUserDefaults on macOS:
  https://dxr.mozilla.org/mozilla-central/rev/0d806b3230fe4767fa70cdee57db87d1e9a5ba49/toolkit/crashreporter/client/crashreporter_osx.mm#175

I think we did things this way because the crashreporter client is a platform-native app and doesn't have access to all of Gecko, and also we can have startup crashes before the profile is available, so it's hard to depend on a pref here.

With that being said, I think it'd be fine to change how that works if we can come up with a more sensible way to do things. We could use a pref and pass that value down to the crashreporter in an environment variable or something like that (but we'd have to figure out how to persist changes to the checkbox from the crashreporter client).

Comment 24

[Gabriele Svelto [:gsvelto]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #23)
> With that being said, I think it'd be fine to change how that works if we
> can come up with a more sensible way to do things. We could use a pref and
> pass that value down to the crashreporter in an environment variable or
> something like that (but we'd have to figure out how to persist changes to
> the checkbox from the crashreporter client).

That wouldn't work for startup crashes when we might not have the time to read a pref and set the corresponding environment variable. Since we need to persist the pref anyway it's better to just use that mechanism. The UI can change of course if we feel like it. This might include opt-in auto-submit like we have for content crashes.

Comment 25

[(not currently active) Ted Mielczarek]

True. Maybe we should just consolidate all the platforms to use an .ini file like we do on Linux, which should make it simpler to change the value from within the Firefox prefs, at least?

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Hey gsvelto, do you have the cycles to take this off of my hands?

Comment 27

[Gabriele Svelto [:gsvelto]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #26)
> Hey gsvelto, do you have the cycles to take this off of my hands?

I'd be glad to help but I'm going on paternal leave for two weeks starting on Monday, sorry :(

Comment 28

[Mike Conley (:mconley) (:⚙️)]

I've opened communications with Tina_Hsieh and mheubusch to get their input here.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

It's too late to change this for 59 but we might still be able to get in a change for 60. 
More likely 61 since may be string changes that need translation.

Comment 30

[Pascal Chevrel:pascalc]

Mike, this bug is marked as a P1 but has been stalled for months, should we move it from P1 to P3?

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Yeah, that's probably more realistic.

Also unassigning self.

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.