Closed Bug 1341349 Opened 7 years ago Closed 7 years ago

Make the crashreporter send the crash ping when the FHR is enabled

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- verified

People

(Reporter: gsvelto, Assigned: gsvelto)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent
59 bytes, text/x-review-board-request
gfritzsche
: review+
Dexter
: review+
gchang
: approval-mozilla-aurora+
Details
[AURORA-REBASED] Respect FHR prefs
21.95 KB, patch
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #1310703 +++

Currently the crashreporter client sends the crash ping only if telemetry is enabled (which corresponds to the toolkit.telemetry.enabled preference). We want the crash ping to be sent even when only the FHR is enabled (datareporting.healthreport.uploadEnabled preference).
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review120430

::: toolkit/components/telemetry/TelemetrySession.jsm:222
(Diff revision 1)
>  
>        crs.setTelemetrySessionId(sessionId);
>  
> -      // We do not annotate the crash if telemetry is disabled to prevent the
> -      // crashreporter client from sending the crash ping.
> -      if (Utils.isTelemetryEnabled) {
> +      // We do not annotate the crash if the FHR upload is disabled to prevent
> +      // the crashreporter client from sending the crash ping.
> +      if (Preferences.get(PREF_FHR_UPLOAD_ENABLED, false)) {

I think this should also be checking a few other things, e.g. Telemetry.isOfficialTelemetry. , otherwise we would end up sending crash pings from developer builds/debug builds too.

To avoid duplicating the logic that already lives in  TelemetrySend, I think you could expose TelemetrySendImpl.sendingEnabled from TelemetrySend, [here](http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/toolkit/components/telemetry/TelemetrySend.jsm#211).
Georg, do you have an opinion on comment 3?
Flags: needinfo?(gfritzsche)
(In reply to Alessio Placitelli [:Dexter] from comment #3)
> ::: toolkit/components/telemetry/TelemetrySession.jsm:222
> (Diff revision 1)
> >  
> >        crs.setTelemetrySessionId(sessionId);
> >  
> > -      // We do not annotate the crash if telemetry is disabled to prevent the
> > -      // crashreporter client from sending the crash ping.
> > -      if (Utils.isTelemetryEnabled) {
> > +      // We do not annotate the crash if the FHR upload is disabled to prevent
> > +      // the crashreporter client from sending the crash ping.
> > +      if (Preferences.get(PREF_FHR_UPLOAD_ENABLED, false)) {
> 
> I think this should also be checking a few other things, e.g.
> Telemetry.isOfficialTelemetry. , otherwise we would end up sending crash
> pings from developer builds/debug builds too.

This is only one part.

Concerns:
(1) don't send data before the privacy policy was accepted
(1a) start sending data once the privacy policy was accepted
(2) don't send data from non-official builds etc.
(3) handle data upload pref switches
(3a) stop sending when data upload is disabled
(3b) start sending again when data upload is enabled

(1) & (2) can be addressed using e.g. TelemetrySends canSendNow() & sendingEnabled().
(1a) needs a TelemetryReportingPolicy listener.
(3) needs upload pref listeners.

I assume we can call the crash reporter repeatedly and override the previous annotations.
That way we can switch on/off.

I agree that we should consolidate the upload check logic, not duplicate it (whether in TelemetrySend, TelemetryController, ...).
Quickly checking around, the different pieces of the logic are still a bit... distributed and could be centralized.
In the end this is one state machine (with different actions on state transitions etc.).

If the annotation code is wrapped into clear functions (annoteCrashTelemetryURL() or something), this could presumably be called from different existing call sites?
Flags: needinfo?(gfritzsche)
Thanks to both for describing this, it's a lot more complex than I had naively anticipated :-/

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> I assume we can call the crash reporter repeatedly and override the previous
> annotations.

nsICrashReporter::annotateCrashReport() rewrites all the annotations when it's invoked so when it's called again for an annotation that already exists it updates it:

https://dxr.mozilla.org/mozilla-central/rev/58753259bfeb3b818eac7870871b0aae1f8de64a/toolkit/crashreporter/nsExceptionHandler.cpp#2287

> I agree that we should consolidate the upload check logic, not duplicate it
> (whether in TelemetrySend, TelemetryController, ...).
> Quickly checking around, the different pieces of the logic are still a
> bit... distributed and could be centralized.
> In the end this is one state machine (with different actions on state
> transitions etc.).
> 
> If the annotation code is wrapped into clear functions
> (annoteCrashTelemetryURL() or something), this could presumably be called
> from different existing call sites?

I'll have a look at the code and see what I can do to make it more centralized; I think this would be better in general as having just one place where the decision to send or not is made reduces the risk of "leaking" data accidentally (as I would have done with this patch).
(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> Thanks to both for describing this, it's a lot more complex than I had
> naively anticipated :-/

No problem, we can work it out! ;) Let's just go for the solution suggested by Georg in comment 3 for now and deal with the centralization/consolidation later.

> > I agree that we should consolidate the upload check logic, not duplicate it
> > (whether in TelemetrySend, TelemetryController, ...).
> > Quickly checking around, the different pieces of the logic are still a
> > bit... distributed and could be centralized.
> > In the end this is one state machine (with different actions on state
> > transitions etc.).
> > 
> > If the annotation code is wrapped into clear functions
> > (annoteCrashTelemetryURL() or something), this could presumably be called
> > from different existing call sites?
> 
> I'll have a look at the code and see what I can do to make it more
> centralized; I think this would be better in general as having just one
> place where the decision to send or not is made reduces the risk of
> "leaking" data accidentally (as I would have done with this patch).

I've filed bug 1345937 for the consolidation of the checks!
Thanks. So I went through all the various switches and came away with this conclusion: since the crashreporter cannot read the preferences directly it's necessary to add the annotation whenever sending the ping is possible - so that if we crash we'll send the ping - and remove it whenever it's not possible anymore - so that we won't send the ping if we crash.

In a nutshell I could listen to all the preferences involved and when one changes invoke TelemetrySend.sendingEnabled() and TelemetrySend.canSendNow() in the listener to adjust the annotation.
(In reply to Gabriele Svelto [:gsvelto] from comment #8)
> Thanks. So I went through all the various switches and came away with this
> conclusion: since the crashreporter cannot read the preferences directly
> it's necessary to add the annotation whenever sending the ping is possible -
> so that if we crash we'll send the ping - and remove it whenever it's not
> possible anymore - so that we won't send the ping if we crash.
> 
> In a nutshell I could listen to all the preferences involved and when one
> changes invoke TelemetrySend.sendingEnabled() and TelemetrySend.canSendNow()
> in the listener to adjust the annotation.

Yes, this makes sense.

> 
> Concerns:
> (1) don't send data before the privacy policy was accepted

This can be verified with TelemetrySend.canSendNow().

> (1a) start sending data once the privacy policy was accepted

This notification is dispatched to https://dxr.mozilla.org/mozilla-central/rev/34585620e529614c79ecc007705646de748e592d/toolkit/components/telemetry/TelemetrySend.jsm#688

> (2) don't send data from non-official builds etc.

This is handled by TelemetrySend.sendingEnabled()

> (3) handle data upload pref switches

You can watch "datareporting.healthreport.uploadEnabled" for that.

> (3a) stop sending when data upload is disabled
> (3b) start sending again when data upload is enabled

Only send/annotate positively if TelemetrySend.sendingEnabled() and TelemetrySend.canSendNow() are true.
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review121652

From a high-level look over the changes this looks ok.
Alessio, what do you think?

I did not do a detailed review and i did not look at the test changes.

::: toolkit/components/telemetry/TelemetryController.jsm:723
(Diff revision 2)
>      // Perform a lightweight, early initialization for the component, just registering
>      // a few observers and initializing the session.
>      TelemetrySession.earlyInit(this._testMode);
>  
> +    // Annotate crash reports so that we get pings for startup crashes
> +    TelemetrySend.annotateCrashReport();

Lets just make this `TelemetrySend.earlyInit()`.
That it ends up annotating crash reports is internal detail.

::: toolkit/components/telemetry/TelemetrySend.jsm:575
(Diff revision 2)
> +  // Whether sending pings has been overridden. We have it here instead
> +  // of the top of the file to ease testing.

Lets drop the second sentence, it doesn't seem to add any information.

::: toolkit/components/telemetry/TelemetrySend.jsm:637
(Diff revision 2)
>  
>      // Start sending pings, but don't block on this.
>      SendScheduler.triggerSendingPings(true);
>    }),
>  
> +  annotateCrashReport() {

This weird here, but it already was weird in TelemetrySession.jsm.
So lets do this now and make sure to have a follow-up bug (depending on bug 1345937?) to consolidate the crash annotation logic.

::: toolkit/components/telemetry/TelemetrySend.jsm:692
(Diff revision 2)
> +      // FIXME: When running tests this causes errors to be printed out if
> +      // TelemetrySend.shutdown() is called twice in a row without calling
> +      // TelemetrySend.setup() in-between.

Alessio, do you have an idea?
Attachment #8845074 - Flags: review?(gfritzsche)
Flags: needinfo?(alessio.placitelli)
Blocks: 1347077
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review121652

> This weird here, but it already was weird in TelemetrySession.jsm.
> So lets do this now and make sure to have a follow-up bug (depending on bug 1345937?) to consolidate the crash annotation logic.

Filed bug 1347077.

> Alessio, do you have an idea?

One way to fix this would be to have TelemetrySend.reset() call shutdown() and setup() but it would probably require some changes to ensure that the various asynchronous parts don't cause races in the tests.
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review121652

> One way to fix this would be to have TelemetrySend.reset() call shutdown() and setup() but it would probably require some changes to ensure that the various asynchronous parts don't cause races in the tests.

Unfortunately no smart ideas. This is the very same thing that's happening with the observer a few lines below.
In some tests, we have specific reasons not to call the setup function. In others, we don't, it just happens because they are legacy tests. I think we should fix the tests to call the *setup* function, where possible, rather than doing anything more complicated than this in the JSM file. And also live with the error message in the tests that we cannot fix.
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review121896

::: toolkit/components/telemetry/TelemetrySend.jsm:643
(Diff revision 2)
> +    try {
> +      const cr = Cc["@mozilla.org/toolkit/crash-reporter;1"];
> +      if (cr) {
> +        const crs = cr.getService(Ci.nsICrashReporter);
> +
> +        let clientId = ClientID.getCachedClientID();

Do you need to care about the [clientId being null](http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/toolkit/modules/ClientID.jsm#69)?
Attachment #8845074 - Flags: review?(alessio.placitelli)
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review121896

> Do you need to care about the [clientId being null](http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/toolkit/modules/ClientID.jsm#69)?

I don't think so, in my testing passing `null` just strips the annotation.
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

I didn't mean to set me as reviewer :)
Attachment #8845074 - Flags: review?(alessio.placitelli)
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review123428

::: toolkit/components/telemetry/TelemetryController.jsm:723
(Diff revision 3)
>      // Perform a lightweight, early initialization for the component, just registering
>      // a few observers and initializing the session.
>      TelemetrySession.earlyInit(this._testMode);
>  
> +    // Annotate crash reports so that we get pings for startup crashes
> +    TelemetrySend.earlySetup();

Lets be consistent with the preceding line: earlyInit()

::: toolkit/components/telemetry/TelemetrySend.jsm:187
(Diff revision 3)
>    /**
> +   * Partial setup that runs immediately at startup. This currently triggers
> +   * the crash report annotations.
> +   */
> +  earlySetup() {
> +    TelemetrySendImpl.annotateCrashReport();

Nit: call TelemetrySendImpl.earlyInit() here, inside that call annotate...().

::: toolkit/components/telemetry/TelemetrySend.jsm
(Diff revision 3)
> -  // Whether sending pings has been overridden. We have it here instead
> -  // of the top of the file to ease testing.
> -  _overrideOfficialCheck: false,

This looks like unrelated cleanup.
Do we need to do this in the same patch?

::: toolkit/components/telemetry/TelemetrySend.jsm:650
(Diff revision 3)
> +          // If we cannot send pings then clear the crash annotations
> +          clientId = "";
> +          server = "";

From what i understand, empty strings here are used to signal "don't send ping" to the crash reporter?

This seems hard to understand.
Lets just add an explicit boolean "ShouldSendCrashPing" field or so?

::: toolkit/components/telemetry/TelemetrySend.jsm:694
(Diff revision 3)
> +    for (let pref of this.OBSERVED_PREFERENCES) {
> +      // FIXME: When running tests this causes errors to be printed out if
> +      // TelemetrySend.shutdown() is called twice in a row without calling
> +      // TelemetrySend.setup() in-between.
> +      Preferences.ignore(pref, this.annotateCrashReport, this);
> +    }

Do we have a follow-up bug to fix this properly instead?

::: toolkit/components/telemetry/TelemetrySession.jsm
(Diff revision 3)
> -      crs.setTelemetrySessionId(sessionId);
> -
> -      // We do not annotate the crash if telemetry is disabled to prevent the
> -      // crashreporter client from sending the crash ping.
> -      if (Utils.isTelemetryEnabled) {
> -        crs.annotateCrashReport("TelemetryClientId", clientId);

Why did you move the TelemetryClientId annotation?
You should only need to move the TelemetryServerURL annotation.

::: toolkit/components/telemetry/TelemetrySession.jsm
(Diff revision 3)
> -        annotateCrashReport(this._sessionId, yield ClientID.getClientID(),
> -                            Preferences.get(PREF_SERVER, undefined));

Lets not change this here.
Not calling this here might change semantics on when the client id is annotated.

::: toolkit/crashreporter/test/unit/test_crashreporter_crash.js:66
(Diff revision 3)
> +  do_crash(function() {
> +    // Enable the FHR and specify a telemetry server & client ID
> +    let prefs = Components.classes["@mozilla.org/preferences-service;1"]
> +                          .getService(Components.interfaces.nsIPrefBranch);
> +    prefs.setBoolPref("datareporting.policy.dataSubmissionPolicyBypassNotification", true);
> +    prefs.setBoolPref("datareporting.healthreport.uploadEnabled", true);

What about the other scenarios, like disabling upload and policy notfications?
Do we cover them?
Attachment #8845074 - Flags: review?(gfritzsche)
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review123428

> This looks like unrelated cleanup.
> Do we need to do this in the same patch?

Yes, because _overrideOffcialCheck was set only once during setup() which meant we wouldn't have its true value when annotating during startup. With my change calling it always reads the actual pref so we get the real value even early in the startup process.

> From what i understand, empty strings here are used to signal "don't send ping" to the crash reporter?
> 
> This seems hard to understand.
> Lets just add an explicit boolean "ShouldSendCrashPing" field or so?

OK, I'll do something more readable though I still need to call annotateCrashReport() with an empty string because that's how annotations are stripped.

> Do we have a follow-up bug to fix this properly instead?

I don't think so, should I file one myself?

> Why did you move the TelemetryClientId annotation?
> You should only need to move the TelemetryServerURL annotation.

If sending the ping isn't enabled then we don't want either to be put in the crash annotation. I added the `TelemetryClientId` annotation specifically for use in the pingsender together with the `TelemetryServerURL` in bug 1310703 so what I'm doing here is just reverting that change since it's not needed anymore and we're moving all this within `TelemetrySend.jsm `.

> Lets not change this here.
> Not calling this here might change semantics on when the client id is annotated.

Likewise, this is just a revert of bug 1310703. I put that call there in that bug because I needed the annotations early in the startup process but with the changes in this patch it's not needed anymore.

> What about the other scenarios, like disabling upload and policy notfications?
> Do we cover them?

This test only covers the case where we have all the necessary bits enabled, which is the only scenario in which we add the annotation. I can add more tests to check the conditions where only one of those you mention is disabled to verify we don't add it.
(In reply to Gabriele Svelto [:gsvelto] from comment #19)
> Comment on attachment 8845074 [details]
> Bug 1341349 - Enable sending the crash ping from the crashreporter client
> only when the overall configuration would allow pings to be sent
> 
> https://reviewboard.mozilla.org/r/118260/#review123428
> 
> > This looks like unrelated cleanup.
> > Do we need to do this in the same patch?
> 
> Yes, because _overrideOffcialCheck was set only once during setup() which
> meant we wouldn't have its true value when annotating during startup. With
> my change calling it always reads the actual pref so we get the real value
> even early in the startup process.
> 
> > From what i understand, empty strings here are used to signal "don't send ping" to the crash reporter?
> > 
> > This seems hard to understand.
> > Lets just add an explicit boolean "ShouldSendCrashPing" field or so?
> 
> OK, I'll do something more readable though I still need to call
> annotateCrashReport() with an empty string because that's how annotations
> are stripped.
> 
> > Do we have a follow-up bug to fix this properly instead?
> 
> I don't think so, should I file one myself?

Please file it, then we can set things up from there.

> > Why did you move the TelemetryClientId annotation?
> > You should only need to move the TelemetryServerURL annotation.
> 
> If sending the ping isn't enabled then we don't want either to be put in the
> crash annotation. I added the `TelemetryClientId` annotation specifically
> for use in the pingsender together with the `TelemetryServerURL` in bug
> 1310703 so what I'm doing here is just reverting that change since it's not
> needed anymore and we're moving all this within `TelemetrySend.jsm `.
> 
> > Lets not change this here.
> > Not calling this here might change semantics on when the client id is annotated.
> 
> Likewise, this is just a revert of bug 1310703. I put that call there in
> that bug because I needed the annotations early in the startup process but
> with the changes in this patch it's not needed anymore.

This still leaves a concern with changes in semantics for when the client id is loaded.
Do we really need to move this?
How do you make sure that the client id is set properly later once it is delay-loaded?

> > What about the other scenarios, like disabling upload and policy notfications?
> > Do we cover them?
> 
> This test only covers the case where we have all the necessary bits enabled,
> which is the only scenario in which we add the annotation. I can add more
> tests to check the conditions where only one of those you mention is
> disabled to verify we don't add it.

Yes please, i think we should have expanded test coverage.
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review123932

::: toolkit/components/telemetry/TelemetrySend.jsm:650
(Diff revision 4)
> +    try {
> +      const cr = Cc["@mozilla.org/toolkit/crash-reporter;1"];
> +      if (cr) {
> +        const crs = cr.getService(Ci.nsICrashReporter);
> +
> +        let clientId = ClientID.getCachedClientID();

This is not guaranteed to have the client id yet when on earlyInit().

The client id is guaranteed to be loaded from a certain point in TelemetryController delayed init on:
https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/components/telemetry/TelemetryController.jsm#741

We should do one of:
- make sure that this is updated here once its loaded (trigger annotation from TelemetrySend.setup(), move it after client id loading in TelemetryController)
- leave the ClientId annotation as it was in TelemetrySession.jsm
Attachment #8845074 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] [away March 24 - April 2] from comment #21)
> > > What about the other scenarios, like disabling upload and policy notfications?
> > > Do we cover them?
> > 
> > This test only covers the case where we have all the necessary bits enabled,
> > which is the only scenario in which we add the annotation. I can add more
> > tests to check the conditions where only one of those you mention is
> > disabled to verify we don't add it.
> 
> Yes please, i think we should have expanded test coverage.

FWIW, given the time constraints, lets move that to a separate patch.
(In reply to Georg Fritzsche [:gfritzsche] [away March 24 - April 2] from comment #21)
> Please file it, then we can set things up from there.

Filed bug 1348890 for that.

> This still leaves a concern with changes in semantics for when the client id
> is loaded.
> Do we really need to move this?
> How do you make sure that the client id is set properly later once it is
> delay-loaded?

We set it properly because we're watching toolkit.telemetry.cachedClientID. So as soon as the real ID is available we immediately put it in the annotation as the preference is updated, see here:

https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/modules/ClientID.jsm#217

Note that the only realistic scenarios in which we don't have the cached client ID at startup is either during a first run or when migrating from a very old profile.

> Yes please, i think we should have expanded test coverage.

I've already added the tests in the latest patch; better safe than sorry.
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review123932

> This is not guaranteed to have the client id yet when on earlyInit().
> 
> The client id is guaranteed to be loaded from a certain point in TelemetryController delayed init on:
> https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/components/telemetry/TelemetryController.jsm#741
> 
> We should do one of:
> - make sure that this is updated here once its loaded (trigger annotation from TelemetrySend.setup(), move it after client id loading in TelemetryController)
> - leave the ClientId annotation as it was in TelemetrySession.jsm

I replied directly on the bug without realising you had asked the same question here. In a nutshell we don't need to explicitly wait for the client ID to be available because we're watching the related pref (toolkit.telemetry.cachedClientID). So as soon as the cached client ID is available we immediately update the annotation. This should be very robust since it doesn't rely on any specific point where the ID would be available.
(In reply to Gabriele Svelto [:gsvelto] from comment #24)
> > This still leaves a concern with changes in semantics for when the client id
> > is loaded.
> > Do we really need to move this?
> > How do you make sure that the client id is set properly later once it is
> > delay-loaded?
> 
> We set it properly because we're watching toolkit.telemetry.cachedClientID.

This is an implementation detail of ClientID.jsm, please don't do that.
We have a structured approach to this, which allows us to reason about it better.
(In reply to Georg Fritzsche [:gfritzsche] [away March 24 - April 2] from comment #26)
> This is an implementation detail of ClientID.jsm, please don't do that.
> We have a structured approach to this, which allows us to reason about it
> better.

OK, so I'm going with your suggestion from comment 22 of moving TelemetrySend.setup() after we load the client ID in TelemetryController.setupTelemetry() so that we're sure that the client ID has been loaded.
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review124912

This looks good to me now.
Alessio, can you also take pass to make sure i'm not missing anything?
Attachment #8845074 - Flags: review?(gfritzsche) → review+
Attachment #8845074 - Flags: review?(alessio.placitelli)
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review124934

Thanks Georg, Gabriele. This looks good, I just left a few observations below.

::: toolkit/crashreporter/test/unit/test_crashreporter_crash.js:76
(Diff revision 5)
> +
> +    // TelemetrySession setup will trigger the session annotation
> +    let scope = {};
> +    Components.utils.import("resource://gre/modules/TelemetryController.jsm", scope);
> +    Components.utils.import("resource://gre/modules/TelemetrySend.jsm", scope);
> +    scope.TelemetrySend.setTestModeEnabled(true);

Are you sure this line is needed?

::: toolkit/crashreporter/test/unit/test_crashreporter_crash.js:101
(Diff revision 5)
> +
> +    // TelemetrySession setup will trigger the session annotation
> +    let scope = {};
> +    Components.utils.import("resource://gre/modules/TelemetryController.jsm", scope);
> +    Components.utils.import("resource://gre/modules/TelemetrySend.jsm", scope);
> +    scope.TelemetrySend.setTestModeEnabled(true);

Are you sure this line is needed?

::: toolkit/crashreporter/test/unit/test_crashreporter_crash.js:122
(Diff revision 5)
> +
> +    // TelemetrySession setup will trigger the session annotation
> +    let scope = {};
> +    Components.utils.import("resource://gre/modules/TelemetryController.jsm", scope);
> +    Components.utils.import("resource://gre/modules/TelemetrySend.jsm", scope);
> +    scope.TelemetrySend.setTestModeEnabled(true);

Are you sure this line is needed?
Attachment #8845074 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review124934

> Are you sure this line is needed?

Yes, it's required otherwise the ping wouldn't be sent (we need to set that or the preference that overrides the official check):

https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/toolkit/components/telemetry/TelemetrySend.jsm#1048

> Are you sure this line is needed?

Likewise.

> Are you sure this line is needed?

Likewise.
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review124934

> Yes, it's required otherwise the ping wouldn't be sent (we need to set that or the preference that overrides the official check):
> 
> https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/toolkit/components/telemetry/TelemetrySend.jsm#1048

Isn't that already set by TelemetrySend.setup()? See [here](https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/toolkit/components/telemetry/TelemetrySend.jsm#187).
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review124934

> Isn't that already set by TelemetrySend.setup()? See [here](https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/toolkit/components/telemetry/TelemetrySend.jsm#187).

Yes it should... I hadn't noticed it because if I remove these lines the test fails. Let me double-check what's going on. It's possible that the delayed initialization is not happening.
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review124934

> Yes it should... I hadn't noticed it because if I remove these lines the test fails. Let me double-check what's going on. It's possible that the delayed initialization is not happening.

Yes, just like I thought, the delayed setup is never run so `TelemetrySend.setup(true)` is not being called. Let me see if I can fix that, it should make the tests more robust too.
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

https://reviewboard.mozilla.org/r/118260/#review124934

> Yes, just like I thought, the delayed setup is never run so `TelemetrySend.setup(true)` is not being called. Let me see if I can fix that, it should make the tests more robust too.

We cleared this up over IRC: even though TelemetryController.testSetup() should be enough in this case, due to the way the test is written using it it's troublesome. Let's keep this test as it is, land this, and file a follow-up.
Running the try run again before landing, just in case since this was modified and rebased: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4010b50ccb6a8381c5765aacc512e05139ff2e97
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 07bfe5c8d089 -d e7dbc2e8b2f9: rebasing 383793:07bfe5c8d089 "Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent r=Dexter,gfritzsche" (tip)
merging toolkit/components/telemetry/TelemetryController.jsm
merging toolkit/components/telemetry/TelemetrySession.jsm
warning: conflicts while merging toolkit/components/telemetry/TelemetrySession.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecffe9f79f0e
Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent r=Dexter,gfritzsche
https://hg.mozilla.org/mozilla-central/rev/ecffe9f79f0e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Abe, I know you're already working on this. I'm leaving this ni?^ around just as a reminder. Once you've validated it, we can move on with the uplifts.
Flags: needinfo?(amasresha)
QA verified as fixed.

Here is the link to test cases and runs: https://public.etherpad-mozilla.org/p/bug1341349

Please needinfo me or send me an email if you have questions about the document or the testing. Thanks
Status: RESOLVED → VERIFIED
Flags: needinfo?(amasresha)
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

Approval Request Comment
[Feature/Bug causing the regression]: bug 1310703
[User impact if declined]: the crashreporter will not respect the FHR upload settings when trying to upload pings to our servers, sending us pings even if it's not meant to.
[Is this code covered by automated tests?]: yes, new tests were added to test_crashreporter_crash.js
[Has the fix been verified in Nightly?]: yes, see comment 41
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: worst case we don't get crash pings faster, but respecting the user choice outweighs this 
[String changes made/needed]: None
Attachment #8845074 - Flags: approval-mozilla-aurora?
Attachment #8845074 - Flags: approval-mozilla-beta?
Attachment #8845074 - Flags: approval-mozilla-beta?
 [bugday-20170322]
status-firefox55: fixed → verified
Comment on attachment 8845074 [details]
Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent

This patch can make crash ping to be sent when only the FHR is enabled and was verified. Aurora54+.
Attachment #8845074 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
needs rebasing for aurora
Flags: needinfo?(gsvelto)
I'll handle the rebasing, as Gabriele is on PTO.
Flags: needinfo?(gsvelto)
The original patch wasn't applying cleanly due to bug 1344741 not being on Aurora :-) And we don't want to uplift that one too!
Attachment #8853894 - Attachment is patch: true
See Also: → 1399681
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: