1426886 - Timer.jsm setTimeout doesn't check that aCallback is a function

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Alex Vincent [:WeirdAl]]

Attachment: Timer.jsm.diff

I've been getting a really dumb, hard-to-trace error because somewhere in our company code, we passed in a non-function to Timer.jsm's setTimeout.

CONSOLE: error [JavaScript Error: "TypeError: aCallback.apply is not a function" {file: "resource://gre/modules/Timer.jsm" line: 30}]

I realize some want to get rid of it (bug 1357731), but in the meantime can we at least sanity-check our arguments so they don't blow up later?

Patch attached but not tested...

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8938678 [details] [diff] [review]
Timer.jsm.diff

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

Thanks! r=me if we brace the one-liner.

::: toolkit/modules/Timer.jsm
@@ +24,5 @@
>  var gTimerTable = new Map(); // int -> nsITimer
>  
>  function _setTimeoutOrIsInterval(aCallback, aMilliseconds, aIsInterval,
>                                   aTarget, aArgs) {
> +  if (typeof aCallback !== "function")

Could you please brace this one-liner? That seems to be the convention throughout the rest of the file.

Comment 2

[Alex Vincent [:WeirdAl]]

I can - but I think I should also add an automated test somewhere... at the time I filed this bug, I was rather annoyed.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Alex Vincent [:WeirdAl] from comment #2)
> I can - but I think I should also add an automated test somewhere... at the
> time I filed this bug, I was rather annoyed.

Yes, that's a very good idea.

toolkit/modules/tests/xpcshell/test_timer.js is the right place.

Comment 4

[Alex Vincent [:WeirdAl]]

Attachment: Timer.jsm should validate callback argument is a function

Comment 5

[Alex Vincent [:WeirdAl]]

Comment on attachment 8945985 [details] [diff] [review]
Timer.jsm should validate callback argument is a function

with tests added and the braces corrected as desired

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8945985 [details] [diff] [review]
Timer.jsm should validate callback argument is a function

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

Looks great! Thanks for the tests!

Comment 7

[Alex Vincent [:WeirdAl]]

Comment on attachment 8945985 [details] [diff] [review]
Timer.jsm should validate callback argument is a function

I forgot, checkin-needed is still preferred.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00ef102abbc6
Timer.jsm should validate callback argument is a function. r=mconley

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/00ef102abbc6