1407082 - "unexpectederror/[object Object]" in sync telemetry errors

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

[Mark Hammond [:markh] [:mhammond]]

Seen mainly for the history engine, but could possibly sneak into other places.

For history, Kit speculates it may be https://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/toolkit/components/places/History.jsm#1368, although that should only happen if no records were updated, and history.js already checks we have some records to update before making the call.

So maybe a first step is to have telemetry.js do some additional introspection on the object, and/or JSON.stringify it?

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

I'm just going to do this now since it's trivial.

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1407082 - Handle more kinds of errors in SyncTelemetry.transformError.

Review commit: https://reviewboard.mozilla.org/r/188056/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188056/

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

kit: requested review from you since mark indicated he probably won't have time for a lot more reviewing.

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8917035 [details]
Bug 1407082 - Handle more kinds of errors in SyncTelemetry.transformError.

https://reviewboard.mozilla.org/r/188056/#review193318

Modulo nits, this looks good to me!

::: services/sync/modules/telemetry.js:721
(Diff revision 1)
>      }
> -
> +    // It's probably an Error object, but it also could be some
> +    // other object that may or may not override toString to do
> +    // something useful.
> +    let msg = String(error);
> +    if (msg.startsWith("[object")) {

`error.toString == Object.prototype.toString` might be a clearer way to check custom representations here.

::: services/sync/modules/telemetry.js:723
(Diff revision 1)
> +    // other object that may or may not override toString to do
> +    // something useful.
> +    let msg = String(error);
> +    if (msg.startsWith("[object")) {
> +      // Nothing useful in the default...
> +      if (error.message) {

Let's just check if `typeof error.message == "string` here, and stringify if it's not. `message` is generic enough to where it could be something funky, like a nested object.

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8917035 [details]
Bug 1407082 - Handle more kinds of errors in SyncTelemetry.transformError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188056/diff/1-2/

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8917035 [details]
Bug 1407082 - Handle more kinds of errors in SyncTelemetry.transformError.

https://reviewboard.mozilla.org/r/188056/#review193318

> `error.toString == Object.prototype.toString` might be a clearer way to check custom representations here.

As discussed in IRC, this doesn't appear to work due to weird realm or compartment issues or something. TLDR plain objects appear to have a different toString if they come from tests (and possibly elsewhere).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/280e60237f2c
Handle more kinds of errors in SyncTelemetry.transformError. r=kitcambridge

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/280e60237f2c