1218746 - Suppress "ERROR ClientID::getCachedClientID - invalid client id in preference" on fresh profiles

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

Since bug 1186488 landed, when creating a new profile or starting Firefox for the first time, this error appears in the browser console:

"Toolkit.Telemetry ERROR ClientID::getCachedClientID - invalid client id in preferences, resetting: null"

This is happening because we're checking for the cached clientID validity even when it is null [0].

To fix this, in [0], just after |let id = Preferences...|, we should check if the id is null and explicitly return null.

To check if this works, start Firefox with a fresh profile and check that the error message is no longer present in the browser console.

Moreover, we should also run other xpcshell tests to make sure nothing breaks:

./mach xpcshell-test toolkit/components/telemetry

[0] - https://dxr.mozilla.org/mozilla-central/rev/6c7c983bce46a460c2766fbdd73283f6d2b03a69/toolkit/modules/ClientID.jsm#177

Comment 1

[Eduard Hanu]

Attachment: rev1 - Invalid ClientID Fix in ClientID.jsm

Hello! Please find attached my patch for this bug.

Comment 2

[Alessio Placitelli [:Dexter]]

Comment on attachment 8684409 [details] [diff] [review]
rev1 - Invalid ClientID Fix in ClientID.jsm

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

Hi Eduard, thank you for submitting your patch!

::: toolkit/modules/ClientID.jsm
@@ +173,5 @@
>      }
>  
>      // Not yet loaded, return the cached client id if we have one.
>      let id = Preferences.get(PREF_CACHED_CLIENTID, null);
> +    if (id == null) {

The |if(!isValidClientID(..))| part should not be removed.

Instead, we should add a new if to strictly check ("===") that id is null and, if that's the case, we should return null instead of going on with the next isValidClientID check.

Comment 3

[Eduard Hanu]

Attachment: rev2 - Invalid ClientID Fix in ClientID.jsm

Hello Alessio! Sorry for the misunderstanding. Please find attached my updated patch.

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8684596 [details] [diff] [review]
rev2 - Invalid ClientID Fix in ClientID.jsm

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

We're almost there Eduard! Thank you for posting the updated patch.
This looks good, there are just a couple of minor stylistic changes left.

::: toolkit/modules/ClientID.jsm
@@ +173,5 @@
>      }
>  
>      // Not yet loaded, return the cached client id if we have one.
>      let id = Preferences.get(PREF_CACHED_CLIENTID, null);
> +	if (id === null) {

We don't use tabs for the indentation: please use 2 spaces per logic level as described in the Coding Style guide here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Indentation

@@ +176,5 @@
>      let id = Preferences.get(PREF_CACHED_CLIENTID, null);
> +	if (id === null) {
> +		return null;
> +	}
> +    else if (!isValidClientID(id)) {

Since we're returning in the previous if branch, we can drop the |else| here and just go to a new line with the |if (!isValidClientID(id)) {|. For more info, see: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices

Comment 5

[Eduard Hanu]

Attachment: rev3 - Invalid ClientID Fix in ClientID.jsm

No problem, Alessio. You will find attached my latest patch with the necessary indentation and separate if statement changes.

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8685180 [details] [diff] [review]
rev3 - Invalid ClientID Fix in ClientID.jsm

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

Thanks Eduard, this looks good now.

Just a small suggestion: whenever you submit an updated revision of the patch, be sure to mark the old revision as obsolete, as it may create confusion to have all those patches attached to the bug.

I'm moving the review to :gfritzsche.

::: toolkit/modules/ClientID.jsm
@@ +175,5 @@
>      // Not yet loaded, return the cached client id if we have one.
>      let id = Preferences.get(PREF_CACHED_CLIENTID, null);
> +    if (id === null) {
> +      return null;
> +    }

Nit: I would personally add a newline after this line, but it's a matter of taste.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8685180 [details] [diff] [review]
rev3 - Invalid ClientID Fix in ClientID.jsm

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

Thanks!

Comment 8

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7245388ffad7

Comment 9

[Alessio Placitelli [:Dexter]]

Eduard, I've pushed the patch to the try servers. We'll land this as soon as the tests are complete!

Comment 10

[Eduard Hanu]

Great news! Thank you, Alessio!

Comment 11

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/235ea6e281382979a28349b438ea70689f21ad38
Bug 1218746 - Invalid ClientID Fix in ClientID.jsm. r=gfritzsche

Comment 12

[Alessio Placitelli [:Dexter]]

Good news Eduard: the test failures look unrelated, so I've pushed this for you on fx-team. If all the tests pass, this will be merged soon!

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/235ea6e28138