665410 - waitFor's internal boolean comparison breaks Javascript truth for objects

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

This line is currently in utils.waitFor():

https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/utils.js#L459

  while((self.result != true) && (self.counter < timeout)) {
    thread.processNextEvent(true);
  }

(This is also in the mozmill 2.0 default branch.)

The first boolean comparison breaks truth for objects. Obj != true is always true, even though any object is true in Javascript.

Should be:

  while((!self.result) && (self.counter < timeout)) {
    thread.processNextEvent(true);
  }

Same goes for any other comparisons in that module (as well as assert/expect's ok) that are supposed to simply honor JS truth. We should never be comparing explicitly against a boolean, pretty much ever.

Comment 1

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

So this is somewhat invalid because the callback for waitFor always has to return a boolean value. So we should probably add a check if that's the case. Or as Geo suggests we really fallback and assume it could even be an object.

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

The only thing technically forcing a boolean return is the line I highlighted. In languages that support implicit truth (of which JS is one) our functions should also support implicit truth.

http://programmers.stackexchange.com/a/136933 explains my position on this pretty well.

IMO, you should make the fix suggested, then update the docs to indicate that waitFor exits when the callback returns "a true value."

Comment 3

[cmtalbert]

We're just going to enforce that we get back a boolean from the test callback.

Comment 4

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Clint Talbert ( :ctalbert ) from comment #3)
> We're just going to enforce that we get back a boolean from the test
> callback.

Out of curiosity, what's the rationale? The fix to allow JS-native truth handling is simple and explictly outlined above.

Comment 5

[cmtalbert]

(In reply to Geo Mealer [:geo] from comment #4)
> (In reply to Clint Talbert ( :ctalbert ) from comment #3)
> > We're just going to enforce that we get back a boolean from the test
> > callback.
> 
> Out of curiosity, what's the rationale? The fix to allow JS-native truth
> handling is simple and explictly outlined above.

Because the callbacks are not supposed to be sending us an object. They are supposed to return a boolean value.  If they are sending us an object, something is wrong with the test and the callback and they should be informed.

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

OK, thanks. Appreciate the followup comment.

Comment 7

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

Attachment: Patch v1

Pointer to Github pull-request

Comment 8

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

Comment on attachment 626238 [details]
Patch v1

Fixes the problem by checking the type of the callback return value. Also adds Mutt tests.

Comment 9

[cmtalbert]

Comment on attachment 626238 [details]
Patch v1

Looks good. thanks!

Comment 10

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

Landed as:
https://github.com/mozautomation/mozmill/commit/2ab2c1f19dd59c2dc64ea3902e1c469e952fc35b