Closed Bug 1407082 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Sync, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: markh, Assigned: tcsc)

Details

Attachments

(1 file)

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?
I'm just going to do this now since it's trivial.
Assignee: nobody → tchiovoloni
kit: requested review from you since mark indicated he probably won't have time for a lot more reviewing.
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.
Attachment #8917035 - Flags: review?(kit) → review+
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).
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/280e60237f2c
Handle more kinds of errors in SyncTelemetry.transformError. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/280e60237f2c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.