Thenable coercion weak map logic is still not quite right

[domenic]

• As pointed out in Thenable coercion should use thenable.then(deferred.resolve, deferred.reject), not deferred.resolve(thenable) #77, only one of its two entry points is currently guarded by an IsPromise, which is inconsistent and bad. So in some cases real promises get into it and sometimes they don't.
• Why are we handling real promises differently from thenables in the first place? Real promises can have their then methods overriden to do unspeakable things anyway. I am pretty sure (but should check) that this is handled fine; we don't use real-promiseness to give any new capabilities, but only to bypass the memoization step.

I am tempted to scrap the thenable coercion weak map entirely, but need to have some discussions with various stakeholders first to better understand its purpose and how it should behave under various degenerate situations. Filing this to track that.

[domenic]

I think it would be best to scrap it for now as it is not coherent; having it in the spec is misleading to implementers. But I have an idea for a replacement that may fulfill all its use cases and just be better.

What was the purpose of the thenable conversion weak map? It had a few facets:

1. Promises should never change their fulfillment value, i.e. if you do promise.then(f1) and in some other turn promise.then(f2), then f1 and f2 should always be called with the same value.
2. You should not access untrusted and possibly ill-behaved then methods more than you have to (@erights can expand on this).
3. It provided some optimization potential, possibly premature optimization.

It notably fails at 1 because of the issues noted in the OP, i.e. it is not used for real promises with overriden then methods (or, equivalently, subclass instances). So you could do

var p1 = Promise.resolve(5);
var p2 = Promise.resolve(p1);

setTimeout(() => p1.then = f => f(10), 5000);

and, for five seconds, p2.then(f) will call f with 5, but after five seconds, p2.then(f) will call f with 10.

It also more or less fails at 3 because of the same issue; implementers cannot internally collapse promise chains because at any time someone could modify the then method of some link in the chain.

---

What if, instead of a global weak map, we instead cached a promise's fulfillment value?

If you recall, we store the resolution value internally to support the monad use case, and only unwrap it when then is called, to get the fulfillment value to pass to onFulfilled. But what if, after unwrapping, we stored that unwrapped value internally, and re-used it in future calls to then?

This seems like it solves all the desired problems, with a slight regression on 2, in that putting a thenable into multiple separate promise chains will result in its then method being called once for each of those chains. But it fixes 1, and seems like a clear win for 3, essentially codifying that optimization into the spec.

[erights]

@domenic
If the only disadvantage is

with a slight regression on 2, in that putting a thenable into multiple separate promise chains will result in its then method being called once for each of those chains

and assuming you mean only non-promise thenables, then I think it is fine. In fact, I had not thought of that case before. (Apologies if it has been discussed and I missed it.) For this case, I find the new behavior you describe as less surprising. I was only ever concerned about stable behavior of promises.

[sicking]

I don't understand this bug. Why can't we trust that a thenable is correctly implemented? It seems to me that we're making a much bigger leap in assuming that any object with a callable .then property is a thenable. I understand why we are making this big leap, and I've been convinced that it's a necessity. But if we're making that big leap, why can't we also take the small step of assuming that the promise is well behaved and actually follows the contract that we've laid out for thenables?

I.e. it seems like a much bigger risk to me that a function with a callable .then property isn't a thenable at all, than that it will at a later point morph into something that isn't a thenable.

Please keep in mind that all of the complexity that we're adding has significant penalties in the form of performance and code size (which slows down engine development and maintenance).

Also note that even an object which passes the currently defined IsPromise(x) test isn't actually "trustable" since it could have had any of its functions overridden. So we'd need to cache the resolved value of even such objects.

[domenic]

@sicking Promises come with certain guarantees, e.g. that when you do p.then(onFulfilled, onRejected), then onFulfilled will only ever be called once; or that if you call p.then(f1) and then p.then(f2) in five seconds, f1 and f2 will receive the same value. Enforcing those guarantees is the point of this bug.

Please keep in mind that all of the complexity that we're adding has significant penalties in the form of performance

I think this, at least, has been proven false by Bluebird.

Also note that even an object which passes the currently defined IsPromise(x) test isn't actually "trustable" since it could have had any of its functions overridden. So we'd need to cache the resolved value of even such objects.

If you note the solution outlined, we do not cache the fulfillment value in a weak map anymore. The new solution is to cache the promise's fulfillment value on the promise itself.

[sicking]

There is no way that you can create useful guarantees. First off, calling the .then function on something that isn't actually a thenable could cause all sorts of side effects which completely breaks the program. So if you ever get an object passed to a resolve function which has a .then function, but that isn't actually intended to be a thenable, then the program will likely break.

Second, if someone has a thenable object, you can't prevent that calling thenable.then(fun1, fun2) might call both fun1 and fun2 at any inappropriate opportunity.

As currently defined, even Promise.cast(x).then(fun1, fun2) doesn't provide any guarantees. Consider for example

var x = new Promise(()=>{});
x.then = function(f1, f2) {
  f1(); f1(); f2();
};

Storing resolved value internally in x the object doesn't do anything to help that problem.

If an object is intended to be a thenable, and we trust it to behave as such during even a single call to it's .then function, then why can't we trust it to behave as such during a second call?

The whole point of supporting thenables was to keep existing code working. This issue seems like feature creep beyond that.

The only way to provide guarantees about an object is to create a new object. If a caller wants real guarantees then it can simply do var trustable = new Promise(r => r(untrusted)).

[domenic]

Second, if someone has a thenable object, you can't prevent that calling thenable.then(fun1, fun2) might call both fun1 and fun2 at any inappropriate opportunity.

Yes, which is why you don't pass the onFulfilled and onRejected handlers directly.

As currently defined, even Promise.cast(x).then(fun1, fun2) doesn't provide any guarantees.

The intent is to combine Promise.cast with Object.freeze(Promise.prototype); Object.freeze(Promise).

This issue seems like feature creep beyond that.

This is not feature creep. Promises have always been intended to provide guarantees, since their earliest appearance in strawman:concurrency.

var trustable = new Promise(r => r(untrusted))

The changes in this issue are necessary in order to enforce guarantees from this code. Promise.cast vs. Promise.resolve (or the longhand form you give) is irrelevant. Without some form of fulfillment value caching (not resolution caching, as you keep misspeaking), the guarantees can be broken during the unwrapping process. For example, see the recent attack patched in 2f128fd, especially the test illustrating it.

[sicking]

If someone has a direct reference to the thenable then you can't prevent them from calling p.then(fun1, fun2) directly, at which point you don't have an opportunity to prevent the callbacks from reaching the untrusted code.

You can request that everyone always calls Promise.cast. But keep in mind that this means that that means that you'll have to either mutate the passed in object by freezing it, which seems unexpected, or always freeze all Promise instances at time of creation, which seems very different from how all other built-ins behave.

You'll also have to create new promise instances anytime that Promise.cast receives a non-native thenable, which is yet another performance cost.

All this to try to cover up bugs when people implement their own thenables? Again, what's the use case? If people write buggy code, they will get buggy programs. This is not an API that is hard to implement. It's not even an API that anyone needs to implement given that it'll soon be shipping natively!

For cases when you for security reasons want to get the guarantees, there's even a function that provides safety like you point out: Promise.resolve(). What's the purpose of having Promise.cast() provide the same functionality? If it's performance, then you're sacrificing performance, consistency and complexity elsewhere.

[domenic]

(Sorry for the delay.) I think you're missing the point: Promise.resolve provides exactly the same security guarantees as Promise.cast. There is no difference. They are both susceptible to the same attacks when passed a malicious thenable. For example, see 2f128fd, which uses Promise.resolve only.

To be even more explicit, here is a test that currently fails. Note that it doesn't use Promise.cast at all (but it could, with the same result); Promise.resolve and Promise.cast have exactly the same security properties here.

specify("If resolved to a thenable which calls back with different values each time", function (done) {
    var thenable = {
        i: 0,
        then: function (f) {
            f(this.i++);
        }
    };
    var p = Promise.resolve(thenable);

    p.then(function (value) {
        assert.strictEqual(value, 0);

        p.then(function (value) {
            assert.strictEqual(value, 0); // fails, 1 !== 0

            p.then(function (value) {
                assert.strictEqual(value, 0); // fails, 2 !== 0
                done();
            });
        });
    });
});

Notice what happens in this test: a real promise, p, created via Promise.resolve, was made to hand out different fulfillment values depending on who calls it. This is a big problem! Such malicious entities cannot be allowed to be introduced into the promise graph.

This issue tracks progress toward fixing that problem, by caching fulfillment values within the promise.

---

For anyone watching at home, I'm targeting New Years Day as the time I can fix this. It's been made hard by all the monad stuff baked into the spec, and also it's probably best to do #78 first, since the related sections of the spec will be changed (well, obfuscated, really) drastically by that work.