Closed Bug 1218746 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Telemetry, defect, P3)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: Dexter, Assigned: hanue, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client][lang=js][good first bug])

Attachments

(1 file, 2 obsolete files)

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
Blocks: 1201022
Points: --- → 1
Priority: -- → P4
Whiteboard: [measurement:client][lang=js][good first bug]
Priority: P4 → P3
Hello! Please find attached my patch for this bug.
Attachment #8684409 - Flags: review?(alessio.placitelli)
Assignee: nobody → hanue
Status: NEW → ASSIGNED
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.
Attachment #8684409 - Flags: review?(alessio.placitelli)
Hello Alessio! Sorry for the misunderstanding. Please find attached my updated patch.
Attachment #8684596 - Flags: review?(alessio.placitelli)
Attachment #8684409 - Attachment is obsolete: true
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
Attachment #8684596 - Flags: review?(alessio.placitelli) → feedback+
No problem, Alessio. You will find attached my latest patch with the necessary indentation and separate if statement changes.
Attachment #8685180 - Flags: review?(alessio.placitelli)
Attachment #8684596 - Attachment is obsolete: true
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.
Attachment #8685180 - Flags: review?(gfritzsche)
Attachment #8685180 - Flags: review?(alessio.placitelli)
Attachment #8685180 - Flags: feedback+
Comment on attachment 8685180 [details] [diff] [review]
rev3 - Invalid ClientID Fix in ClientID.jsm

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

Thanks!
Attachment #8685180 - Flags: review?(gfritzsche) → review+
Eduard, I've pushed the patch to the try servers. We'll land this as soon as the tests are complete!
Great news! Thank you, Alessio!
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!
https://hg.mozilla.org/mozilla-central/rev/235ea6e28138
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
See Also: → 1407234
You need to log in before you can comment on or make changes to this bug.