Closed Nathan-Wall closed 10 years ago
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.
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.)
This is pretty out there, but the call to
GetDeferred
insideThen
is the only place whereGetDeferred
is called and the[[Resolve]]
and[[Reject]]
return values are ignored. The assumption is made here that sinceThen
only ever accepts true Promises (or subclasses) as an argument (p
) that itsconstructor
will create an object which is also a true Promise (passesIsPromise
).Here's a simple counterexample to this assumption (I think):
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 bothfalse
-- 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 byGetDeferred
is actually a true Promise. Alternatively, the[[Resolve]]
and[[Reject]]
functions returned byGetDeferred
could be passed around toUpdateDerivedFromPromise
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 inGetDeferred
with theC
constructor (in case the constructor provides a true Promise sometimes and some other kind of Promise-like thenable other times).