675130 - Throw meaningful errors

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect, P4)

Description

[Philipp von Weitershausen [:philikon]]

Instead of throwing strings or other random stuff, we should throw exceptions with according to a defined format, containing as much information about the error as possible. Ultimately this should also replace a lot of the Status flag tracking. Because incidents like "login failed" aren't a state, they're an error condition and should be treated as such.

In terms of implementation, we could go with the XPCOM style Components.Exception and invent a bunch of new NS_ERROR_* flags. I don't feel like that's particularly useful.

My proposal is to use the built-in Error objects and annotate them as needed. Some options:

a) use the type of error as the message, e.g.:

  throw new Error(LOGIN_FAILED_WRONG_PASSPHRASE);

b) use a more descriptive string as the message and annotate the object accordingly:

  let ex = new Error("Could not login: invalid Sync Key");
  ex.result = LOGIN_FAILED_WRONG_PASSPHRASE

In any case, a lot of times we might be masking or annotating an underlying XPCOM exception or an HTTP error (e.g. a Necko or Places exception, or an HTTP error) in which case we can annotate the exception object, e.g.:

  let meta, error;
  try {
    meta = new Resource(this.metaURL).get();
  } catch (ex) {
    error = ex;
  }
  if (error || meta.status != 200) {
    let ex = new Error(COULD_NOT_FETCH_METARECORD);
    ex.error = error;
    ex.httpResponse = meta;
    throw ex;
  }

or if you prefer async goodness:

  request = new SyncStorageRequest(function (error) {
    if (error || request.response.status != 200) {
      let ex = new Error(COULD_NOT_FETCH_METARECORD);
      ex.error = error;
      ex.httpResponse = request.response;
      return callback(ex);
    }
    ...
  });

Comment 1

[Richard Newman [:rnewman]]

(In reply to comment #0)

+1.

> In terms of implementation, we could go with the XPCOM style
> Components.Exception and invent a bunch of new NS_ERROR_* flags. I don't
> feel like that's particularly useful.

Agreed: only JS will be consuming our errors, so C.E seems like needless work.
 
> b) use a more descriptive string as the message and annotate the object
> accordingly:

This seems like the best option to me: it preserves a readable string for immediate logging, but is no less expressive. I would perhaps use "reason" as the attribute, rather than "result", to preserve some of the English terms around errors.

(It's almost tempting to define these things as an IDL, just for completeness and documentation...)

> In any case, a lot of times we might be masking or annotating an underlying
> XPCOM exception or an HTTP error (e.g. a Necko or Places exception, or an
> HTTP error) in which case we can annotate the exception object, e.g.:

This is actually similar to what we already do in parts of Sync; e.g., Resource, where we set .result, .stack, etc.

(I also took a similar approach for some backchannel communication in my aborted async rewrite, but I'm too lazy to dig up the code.)

The key is to carefully document what goes where, of course. Internal APIs.

There are lots of great areas for immediate improvement in this bug: Utils.isHMACMismatch comes to mind. I volunteer to turn patches into docs as I review :)

Comment 2

[Marina Samuel [:emtwo]]

(In reply to comment #0)
> Instead of throwing strings or other random stuff, we should throw
> exceptions with according to a defined format, containing as much
> information about the error as possible. 

Yes, this is definitely what we need.

> b) use a more descriptive string as the message and annotate the object
> accordingly:
> 
>   let ex = new Error("Could not login: invalid Sync Key");
>   ex.result = LOGIN_FAILED_WRONG_PASSPHRASE

I approve of this option (and the examples you gave below it). The more info we're able to convey the better :)

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #1)
> > b) use a more descriptive string as the message and annotate the object
> > accordingly:
> 
> This seems like the best option to me: it preserves a readable string for
> immediate logging, but is no less expressive. I would perhaps use "reason"
> as the attribute, rather than "result", to preserve some of the English
> terms around errors.

I was mostly trying to preserve the XPCOM nomenclature. But I don't feel strongly about the wording.

> (It's almost tempting to define these things as an IDL, just for
> completeness and documentation...)

Or we do the same thing we did for other things and define an actually useful base implementation:

/**
 * A sync error
 *
 * @param result
 *        The result classification of the error
 * @param message
 *        Human readable error message, mainly for logging
 * @param error [optional]
 *        Underlying error/exception object
 */
function SyncError(result, message, error) {
  Error.call(this, message);
  this.result = result;
  this.error = error;
}
SyncError.prototype = {

  /**
   * ...
   */
  result: null,

  /**
   * ...
   */
  error: null,

  /**
   * ...
   */
  httpResponse: null,

}

And then we just have to throw new SyncError() ...

> There are lots of great areas for immediate improvement in this bug:
> Utils.isHMACMismatch comes to mind. I volunteer to turn patches into docs as
> I review :)

Yep. I was thinking of that one too. Another one is Status.sync = METARECORD_DOWNLOAD_FAIL which clearly isn't a status.

Comment 4

[Philipp von Weitershausen [:philikon]]

Oh, I forgot to set __proto__: Error.prototype, but you get the idea.

Comment 5

[Richard Newman [:rnewman]]

(In reply to comment #3)

> And then we just have to throw new SyncError() ...

Yup.

I think our Repositories rewrite has codified some useful engineering patterns...


> Yep. I was thinking of that one too. Another one is Status.sync =
> METARECORD_DOWNLOAD_FAIL which clearly isn't a status.

Gotta Catch 'Em All®!

Comment 7

[:Atoll]

I am uncertain if this is what was intended by meaningful errors or not:

services/sync/modules/service.js
1348:          throw "aborting sync, process commands said so";

It is remarkably meaningful, and even looks up somehow in constants.js:

ABORT_SYNC_COMMAND:                    "aborting sync, process commands said so",

But it's quite a, um, different path from the rest of the error constants (which are a structured hierarchy without spaces).

I have no vested interest in the color of this bikeshed.

Comment 8

[Richard Newman [:rnewman]]

(In reply to Richard Soderberg [:atoll] from comment #7)

> It is remarkably meaningful, and even looks up somehow in constants.js:

In the context of this bug, "meaningful" means "an error object with useful attributes, rather than just a string".

When this is all done, that line will be changed, and the constant might go away or become something else.

> I have no vested interest in the color of this bikeshed.

:D

Comment 9

[Richard Newman [:rnewman]]

Philipp, do you have code hanging around? Otherwise, let's drop this back in the queue.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Richard Newman [:rnewman] from comment #9)
> Philipp, do you have code hanging around?

I don't.

Comment 11

[Chris Karlof [:ckarlof]]

This is an old bug. Sync (and the error reporting) has changed a lot since it was opened. Let's open new bugs to address specific improvements in error reporting going forward.