Then call to GetDeferred expects a proper Promise (or subclass) but doesn't verify

[Nathan-Wall]

This is pretty out there, but the call to GetDeferred inside Then is the only place where GetDeferred is called and the [[Resolve]] and [[Reject]] return values are ignored. The assumption is made here that since Then only ever accepts true Promises (or subclasses) as an argument (p) that its constructor will create an object which is also a true Promise (passes IsPromise).

Here's a simple counterexample to this assumption (I think):

class SubPromise extends Promise { }

var sp = new SubPromise(resolve => {
    setTimeout(() => resolve('foo'), 2000);
});

Object.defineProperty(sp, 'constructor', {
    value(resolver) {
        var noop = () => { };
        resolver(noop, noop);
    }
});

var sp2 = sp.then(value => {
    console.log(value);
    return 'bar';
});

As far as I can tell, this will wait 2 seconds before failing the assertion in step 1 of SetValue (asserting that [[HasValue]] and [[HasReason]] are both false -- they'll actually not be set because sp2 isn't a true Promise).

An alternative situation that could cause this error would be a constructor that subclasses Promise but sometimes returns a promise and sometimes returns another value.

This error could be caught earlier by ensuring that the [[Promise]] value returned by GetDeferred is actually a true Promise. Alternatively, the [[Resolve]] and [[Reject]] functions returned by GetDeferred could be passed around to UpdateDerivedFromPromise so that whatever value the constructor created could try to be properly resolved with the first and second arguments it provided to its resolver function when it was created in GetDeferred with the C constructor (in case the constructor provides a true Promise sometimes and some other kind of Promise-like thenable other times).

[domenic]

Oh dear, this seems like a pretty large problem. In general, the idea of malicious promise subclasses is causing me headaches.

One fix might be to use [[PromiseConstructor]] instead of the .constructor property. But I believe that does not help the case of

a constructor that subclasses Promise but sometimes returns a promise and sometimes returns another value.

I am also curious about the idea of

Alternatively, the [[Resolve]] and [[Reject]] functions returned by GetDeferred could be passed around to UpdateDerivedFromPromise so that whatever value the constructor created could try to be properly resolved with the first and second arguments it provided to its resolver function when it was created in GetDeferred with the C constructor

which I had considered back when adding subclassing support but dismissed as too complex. If it's necessary to make things work however it may be the way to go. But I would want to be sure it solves these problems first.

---

But yes, now I am worried in general about the subclassing approach. I think it could be patched, e.g. with

This error could be caught earlier by ensuring that the [[Promise]] value returned by GetDeferred is actually a true Promise

or similar, perhaps in a few more places. But I am not sure it is a good idea, as it introduces more magic (related: #42). I am curious if there is another approach. It may be that experimenting with the testable implementation should point the way forward here. In any case, thanks for finding this, and further thoughts definitely appreciated.

[domenic]

I have solved this by adding an IsPromise check to GetDeferred. Failing this check will cause an exception that propagates outward. (I will open a new issue addressing exceptions.)