In bug 1458230, I've found a few cases where we're doing "Assert.ok(foo, 1)" to attempt to check the length of foo. In general, I think we should check the message argument to all of the Assert methods, and verify that if it is defined, then it is a string. This would provide at least some level of protection against these types of mistakes.

Smallish try push to being with (including the patches for bug 1458230): https://treeherder.mozilla.org/#/jobs?repo=try&revision=95c4c38c7d6956ff48440039a3a7f2dedf025624

(I'm away today and as you know Monday is a bank holiday, so I likely won't get to this until Tuesday, sorry!)

Comment on attachment 8973172 [details] Bug 1458235 - Make Assert.* fail if the message argument is defined but not a string. https://reviewboard.mozilla.org/r/241668/#review248148

Comment on attachment 8973172 [details] Bug 1458235 - Make Assert.* fail if the message argument is defined but not a string. https://reviewboard.mozilla.org/r/241668/#review248150 ::: testing/modules/Assert.jsm:197 (Diff revision 1) > * (string) Operation qualifier used by the assertion method (ex: '==') > * @param truncate (optional) [true] > * (boolean) Whether or not `actual` and `expected` should be truncated when printing > */ > proto.report = function(failed, actual, expected, message, operator, truncate = true) { > + if (message && typeof message != "string") { Ah, note that this still won't error if `message === false` (or null, or NaN). Not sure if we should add a check for that or if that's unlikely to happen in this usage pattern. Maybe `Assert.ok(foo, null)` hits that path?

Comment on attachment 8973170 [details] Bug 1458235 - Fix various cases where Assert.ok or Assert.equal have been called wrongly. https://reviewboard.mozilla.org/r/241664/#review248152

Comment on attachment 8973171 [details] Bug 1458235 - Drop cases of Components.stack.caller passed as the msg to Assert.* as it isn't necessary as we already work out the stack correctly for these cases. https://reviewboard.mozilla.org/r/241666/#review248154

Comment on attachment 8973172 [details] Bug 1458235 - Make Assert.* fail if the message argument is defined but not a string. https://reviewboard.mozilla.org/r/241668/#review248150 > Ah, note that this still won't error if `message === false` (or null, or NaN). Not sure if we should add a check for that or if that's unlikely to happen in this usage pattern. Maybe `Assert.ok(foo, null)` hits that path? It turns out the extension tests do something weird that ends up passing `null` as message. So I've ended up changing this to `message !== undefined && message !== null && typeof message != "string"`

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9135bdd314c2 Fix various cases where Assert.ok or Assert.equal have been called wrongly. r=Gijs https://hg.mozilla.org/integration/autoland/rev/03ee745ee8e2 Drop cases of Components.stack.caller passed as the msg to Assert.* as it isn't necessary as we already work out the stack correctly for these cases. r=Gijs https://hg.mozilla.org/integration/autoland/rev/83fb6c53e740 Make Assert.* fail if the message argument is defined but not a string. r=Gijs

https://hg.mozilla.org/mozilla-central/rev/9135bdd314c2 https://hg.mozilla.org/mozilla-central/rev/03ee745ee8e2 https://hg.mozilla.org/mozilla-central/rev/83fb6c53e740