1228239 - More Gaia unit tests misuse Promise static methods

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Boris Zbarsky [:bzbarsky]]

+++ This bug was initially created as a clone of Bug #1223004 +++

In Gecko right now, Promise.resolve.call(undefined, foo) has the same behavior as Promise.resolve.call(Promise, foo), and some Gaia tests rely on that.  However per spec the former should throw while the latter should work.  As a result, fixing bug 1170760 breaks Gaia unit tests.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] bzbarsky:more-promise-bustage > mozilla-b2g:master

Comment 2

[Chris Lord [:cwiiis]]

Comment on attachment 8692370 [details] [review]
[gaia] bzbarsky:more-promise-bustage > mozilla-b2g:master

Lgtm, but I'm a lot more worried about where this mistake has been made in the app code...

Comment 3

[Gregor Wagner [:gwagner]]

Thanks bz!

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Lgtm, but I'm a lot more worried about where this mistake has been made in the app code...

Certainly some places.  I pushed some more updates to the PR today, based on more Gu test runs.  The shared/js/icons_helper.js bit is app code, right.

I also redid my greps for Promise.resolve/reject not followed by '(' or '.' and I think at this point I've got everything.  I'll know for sure once I have the latest try results.

Comment 5

[Chris Lord [:cwiiis]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> > Lgtm, but I'm a lot more worried about where this mistake has been made in the app code...
> 
> Certainly some places.  I pushed some more updates to the PR today, based on
> more Gu test runs.  The shared/js/icons_helper.js bit is app code, right.
> 
> I also redid my greps for Promise.resolve/reject not followed by '(' or '.'
> and I think at this point I've got everything.  I'll know for sure once I
> have the latest try results.

There are places where we return Promise.resolve()/Promise.reject() from a callback, I take it that's ok? And on those rejects that you've commented, can we just remove them? Would having an empty second parameter be equivalent? (I remember adding them because I was seeing things hang when they should've rejected, but maybe I was conflating that with some other issue that was fixed simultaneously) I thought there may be more places than those you've addressed, but I misremembered.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> There are places where we return Promise.resolve()/Promise.reject() from a callback

That should be fine.  The key part is that the call to resolve/reject should have Promise as the "this" value.  So doing this:

  var f = Promise.resolve;
  f();

is not OK, because that's the same as Promise.resolve.call(undefined), which is passing undefined as this value.

> And on those rejects that you've commented, can we just remove them? 

I believe so, yes.  The current structure of those lines is:

  return somePromise.then(something, Promise.reject);

This returns a Promise X which has the following behavior:

1)  If somePromise is resolved with a value V, then X will be resolved with the return
    value of something(V).
2)  If somePromise is rejected with a value V, then X will be resolved with the return
    value of Promise.reject(V), which is a rejected promise, so the upshot is that X gets
    rejected with the value V.

So I think doing this:

  return somePromise.then(something);

should have exactly the same behavior, no?  But I'm happy to do that in a separate PR, just to keep the risk down.

Comment 7

[Chris Lord [:cwiiis]]

Thanks for the explanation - If you feel like doing that in the same PR, I wouldn't object - but I think it's fine to leave it there too to minimise risk. Thanks for sorting this out!

Comment 8

[Boris Zbarsky [:bzbarsky]]

I filed bug 1229034 on that issue.

Comment 9

[Chris Lord [:cwiiis]]

Merged: https://github.com/mozilla-b2g/gaia/commit/83ab258bea992aa391c86e1ad2c94458b4b30a92