637941 - Change utils.waitFor method to throw distinct TimeoutErrors

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Right now the waitFor method throws a plain Error which makes it hard for any code to distinguish between timeouts and real failures.

I implement a workaround for that now in our api refactor repo on bug 634227.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - Throw a custom error message in waitFor

Create a new TimeoutError object which inherits from Error.prototype.

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.1 - Keep the err.stack this time

The last patch loses the stack property derived from the Error object.  I would have thought the proper way to do this was something along the lines of 'Error.apply(this, arguments)' but that didn't work for some reason.  This works around that issue and preserves all the good info found in the Error object.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 516743 [details] [diff] [review]
Patch 1.1 - Keep the err.stack this time

>     message = message || arguments.callee.name + ": Timeout exceeded for '" + callback + "'";

Please also fix that line. Right now it's pretty useless if a message has been specified.

Comment 4

[cmtalbert]

Comment on attachment 516743 [details] [diff] [review]
Patch 1.1 - Keep the err.stack this time

This looks good.  Henrik, I believe the "message = message ||" line should stay as is.  The thinking here is that if the test supplies a specific message, then we use that for the error.  If the test does not supply a message, we use our default message.  That's exactly how an API should work in my opinion -- assuming that the user provided values always trump default values.

Comment 5

[Andrew Halberstadt [:ahal]]

I tend to agree with Clint. That line is only as useless as the message you send it.  Otherwise you always have the option of not sending a message at all.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 516743 [details] [diff] [review]
Patch 1.1 - Keep the err.stack this time

>+  var err = new Error();
>+  if (err.stack) {
>+    this.stack = err.stack;
>+  }
>+  this.message = message === undefined ? err.message : message;
>+  this.fileName = fileName === undefined ? err.fileName : fileName;
>+  this.lineNumber = lineNumber === undefined ? err.lineNumber : lineNumber;
>+};
>+TimeoutError.prototype = new Error();
>+TimeoutError.prototype.constructor = TimeoutError;
>+TimeoutError.prototype.name = 'TimeoutError';

Please also check bug 634227 how I did that for our shared modules. I wonder if we find a consensus in the middle. At least a separate module would be nice to not clutter utils.js with more and more methods.

Also TimeoutError would have to be added to the EXPORTED_SYMBOLS list, otherwise it's pretty useless for us.

Re: 'message' you are right. With the given new error class we can identify errors more easily.

Comment 7

[Andrew Halberstadt [:ahal]]

> Please also check bug 634227 how I did that for our shared modules.
I did look at that patch, and it seems to me that it is even bigger than this one (i.e adding the findCallerFrame method).  I want to do this properly, which means inheriting from Error.prototype. It may not make a huge difference to us, but it will definitely change the way Firefox or other people handle this error.

> At least a separate module would be nice to
> not clutter utils.js with more and more methods.
I'm not opposed to moving it somewhere else, but I don't think we want an entire new module just for one custom Error. I also don't think we'll really find a need to make any other custom Errors either.  In most cases we should just be able to throw one of the six default errors: http://www.javascriptkit.com/javatutors/trycatch2.shtml

> Also TimeoutError would have to be added to the EXPORTED_SYMBOLS list,
> otherwise it's pretty useless for us.
Good catch.

Comment 8

[cmtalbert]

(In reply to comment #6)
> Comment on attachment 516743 [details] [diff] [review]
> Patch 1.1 - Keep the err.stack this time
> 
> >+  var err = new Error();
> >+  if (err.stack) {
> >+    this.stack = err.stack;
> >+  }
> >+  this.message = message === undefined ? err.message : message;
> >+  this.fileName = fileName === undefined ? err.fileName : fileName;
> >+  this.lineNumber = lineNumber === undefined ? err.lineNumber : lineNumber;
> >+};
> >+TimeoutError.prototype = new Error();
> >+TimeoutError.prototype.constructor = TimeoutError;
> >+TimeoutError.prototype.name = 'TimeoutError';
> 
> Please also check bug 634227 how I did that for our shared modules. I wonder if
> we find a consensus in the middle. At least a separate module would be nice to
> not clutter utils.js with more and more methods.
> 
> Also TimeoutError would have to be added to the EXPORTED_SYMBOLS list,
> otherwise it's pretty useless for us.
> 
> Re: 'message' you are right. With the given new error class we can identify
> errors more easily.

With that patch you did Henrik, you're doing a lot of munging the stack which we're not going to do at this level.  That will be factored out elsewhere.  You also added a "function" to the error message which is nice, but the error message already has a caller on its prototype [1] which it inherits, so that seems unneeded.  A lot of the other stuff you're doing with the error class must be a result of the inheritance model you're using because you're doing all the work of moving objects from the prototype to another object prototype, which Andrew does quite simply by inheriting directly from the "native" error object.

So I don't really see this as a huge departure from what you're doing.

Also, I concur that we don't need a separate errors module at the moment.  I also don't anticipate needing to define a bunch of specific error handlers.  If that becomes the case in the future then we'll refactor at that time.

[1]: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Error

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Clint, I talk about the updated and checked-in patch which doesn't use the inheritance module anymore but has a generator for custom errors. Also there is nothing about stack munging.

https://github.com/geoelectric/mozmill-api-refactor/blob/master/modules/errors.js#L50

Comment 10

[Andrew Halberstadt [:ahal]]

master: https://github.com/mozautomation/mozmill/commit/e55c0cee250e00ac991499a2eda8b12a9face548

I added the TimeoutError to EXPORTED_SYMBOLS though I realized that you could use e.name == "TimeoutError" instead of instanceof(e) == TimeoutError.  Either way should work.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The risk for backporting is kinda minimal and it would help us in the meantime to improve our own modules for the rewrite. Could we target it for an upcoming 1.5.x release?

Comment 12

[Jeff Hammel]

Reopening; we're backporting to 1.5.4

Comment 13

[Rain [:rain1] (inactive)]

We're looking at upgrading Thunderbird MozMill to 1.5.4, and need this to cleanly fix bug 662960.

Comment 14

[Jeff Hammel]

pulled to hotfix-1.5: https://github.com/mozautomation/mozmill/commit/11a105dc53eaa5e406ee1e34e74de432e138aabe

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks good with a Mozmill 1.5.4 build. Verified fixed.