1222042 - TypeError: date is undefined when loading about:telemetry with Telemetry disabled

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[:Margaret Leibovic]

I noticed this with a local build while working on bug 1219240.

Here's the stack I saw in the console.

TypeError: date is undefined
this.TelemetryUtils.truncateToDays()
 TelemetryUtils.jsm:44
getMetadata()
 TelemetrySession.jsm:1037
getSessionPayload()
 TelemetrySession.jsm:1328
getPayload()
 TelemetrySession.jsm:1656
this.TelemetrySession<.getPayload()
 TelemetrySession.jsm:626
Impl.getCurrentPingData()
 TelemetryController.jsm:963
this.TelemetryController<.getCurrentPingData()
 TelemetryController.jsm:239
PingPicker._updateCurrentPingData()
 aboutTelemetry.js:327
PingPicker.update()
 aboutTelemetry.js:317
onLoad/<()
 aboutTelemetry.js:1463
onLoad()

Comment 1

[Chris H-C :chutten]

TelemetrySession._sessionStartDate is undefined. Which is odd, because (near as I can tell) it should be set to _subsessionStartDate which is set to Policy.now().

Did you override Policy.now for your testing? If not, it should've just been `new Date()`

Comment 2

[Georg Fritzsche [:gfritzsche]]

I think i've seen that come up before in local builds where Telemetry is not enabled, it is probably limited to that scenario.
(Longer-term i'd love to avoid those kind of issues by basically always having Telemetry recording enabled, but Telemetry upload disabled unless overridden for local builds).

Comment 3

[:Margaret Leibovic]

(In reply to Chris H-C :chutten from comment #1)
> TelemetrySession._sessionStartDate is undefined. Which is odd, because (near
> as I can tell) it should be set to _subsessionStartDate which is set to
> Policy.now().
> 
> Did you override Policy.now for your testing? If not, it should've just been
> `new Date()`

No, I didn't do anything in my testing, other than create a new histogram. This was with a fresh install, so could it be some bug where the session wasn't started immediately?

I can't reproduce now... maybe we should close this bug out, unless I can find a way for this to happen again.

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> I think i've seen that come up before in local builds where Telemetry is not
> enabled, it is probably limited to that scenario.
> (Longer-term i'd love to avoid those kind of issues by basically always
> having Telemetry recording enabled, but Telemetry upload disabled unless
> overridden for local builds).

Always recording telemetry makes sense to me! It's even necessary for testing new probes in local builds.

Comment 4

[Georg Fritzsche [:gfritzsche]]

> (In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> > I think i've seen that come up before in local builds where Telemetry is not
> > enabled, it is probably limited to that scenario.
> > (Longer-term i'd love to avoid those kind of issues by basically always
> > having Telemetry recording enabled, but Telemetry upload disabled unless
> > overridden for local builds).
> 
> Always recording telemetry makes sense to me! It's even necessary for
> testing new probes in local builds.

Yes, i think the setup is pretty confusing for developers right now (plus we don't really gain anything from having recording off).
Bug 1200164 should resolve this issue by cleaning this up.

Comment 6

[:Margaret Leibovic]

Let's reopen this, since Nick has also seen this. Until the error is fixed (whether that's by bug 1200164 or not), this isn't resolved.

Comment 8

[Alessio Placitelli [:Dexter]]

To fix this, we should:

- Return null from TelemetryController.getCurrentPingData in [0] if Telemetry.canRecordBase is false.
- Change the function comments to mention the return value is either the ping data or null
- Return in [1] if ping is null.

To check if this works:

- ./mach xpcshell-test toolkit/components/telemetry
- Manually check that there's no error in the browser console when opening about:telemetry with FHR+Telemetry off and on

[0] - https://dxr.mozilla.org/mozilla-central/rev/cc473fe5dc512c450634506f68cbacfb40a06a23/toolkit/components/telemetry/TelemetryController.jsm#961
[1] - https://dxr.mozilla.org/mozilla-central/rev/cc473fe5dc512c450634506f68cbacfb40a06a23/toolkit/content/aboutTelemetry.js#327

Comment 9

[Alessio Placitelli [:Dexter]]

Robert, are you interested in taking this one?

Comment 10

[rthyberg]

yes!

Comment 11

[rthyberg]

Attachment: bug-1222042

Comment 12

[Alessio Placitelli [:Dexter]]

Comment on attachment 8686296 [details] [diff] [review]
bug-1222042

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

Thank you for the patch, we're almost there! ;)

Could you change the commit message to something like: "Bug 1222042 - Don't try to display ping data in about:telemetry when Telemetry is disabled"

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +956,5 @@
>    },
>  
> +  /**
> +   * Gets current ping data
> +   * @return {ping} returns ping data if Telemetry.canRecordBase is true

Don't add this comment here, instead, change the one at: https://dxr.mozilla.org/mozilla-central/rev/84a7cf29f4f14c9b359db2f7f19c0abd6a8e178e/toolkit/components/telemetry/TelemetryController.jsm#238

Maybe to something like "@return {Object} the ping data if Telemetry is enabled, null otherwise"

@@ +963,3 @@
>    getCurrentPingData: function(aSubsession) {
>      this._log.trace("getCurrentPingData - subsession: " + aSubsession)
>  

We don't need to gather the ping data if Telemetry is disabled, as it would return broken data. We should move the check here.

@@ +968,5 @@
>      const payload = TelemetrySession.getPayload(reason);
>      const options = { addClientId: true, addEnvironment: true };
>      const ping = this.assemblePing(type, payload, options);
>  
> +    if (!Telemetry.canRecordBase) {

Move this to the previous comment.

Nit: could you also add an inline comment about why we're bailing out? e.g. "// Telemetry is disabled, don't gather any data."

Comment 13

[rthyberg]

Attachment: bug-1222042

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8686811 [details] [diff] [review]
bug-1222042

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

Could you address the nit below and change the comment to something like: "Bug 1222042 - Don't try to show ping data in about:telemetry if Telemetry is disabled"?

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +957,5 @@
>  
>    getCurrentPingData: function(aSubsession) {
>      this._log.trace("getCurrentPingData - subsession: " + aSubsession)
>  
> +    //Telemetry is disabled, don't gather any data.

Nit: Could you add a whitespace before "Telemetry"?

Comment 15

[rthyberg]

Attachment: bug-1222042

Comment 16

[Alessio Placitelli [:Dexter]]

Comment on attachment 8687836 [details] [diff] [review]
bug-1222042

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

This looks good for me, thank you rthyberg!
Georg, do you have any other suggestion?

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8687836 [details] [diff] [review]
bug-1222042

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

Thanks, this looks good to me, only one small nit below.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +960,5 @@
>  
> +    // Telemetry is disabled, don't gather any data.
> +    if (!Telemetry.canRecordBase) {
> +      return null;
> +    }

Nit: Add an empty line after this to help with readability.

Comment 18

[rthyberg]

Attachment: bug-1222042

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8688118 [details] [diff] [review]
bug-1222042

Robert, can you push this to try to make sure it's ok?

Comment 20

[rthyberg]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=63949fcada9b

Comment 21

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/2bd33b89c477e60aa80624a5edaa3bb74cbfe2df
Bug 1222042 - Dont try to show ping data in about:telemetry if Telemetry is disabled. r=dexter

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2bd33b89c477