Guidance on handling malicious/flawed [[Reject]] capability

[getify]

I have found a difference in behavior between the reference implementation and that in v8. I am not sure if the reference implementation is correct, but I suspect so. Nevertheless, I've checked the spec draft and am having trouble articulating from the spec exactly how this case should be handled, though (as I said), its handling is observable in the reference implementation.

First, can you validate that this case should be handled as the reference implementation does currently? Secondly, can you point me to which specific part of the spec language dictates that, so I know what text to quote in a bug report to v8/chromium?

---

The scenario: you provide a promise constructor that either on purpose or (more likely) by accident creates a [[Reject]] capability function that itself actually throws an error. The question is, how should that error be handled?

The reference implementation just has it thrown as an uncaught global (on a separate event loop). At first that seems very un-promise like, but then when you think about it, if you can't successfully call the [[Reject]] capability of the promise, and you're off the event cycle where it was created, what other option do you possibly have?

Note: in the v8 implementation, the error gets swallowed and is never reported, which seems much worse.

Here's my (crazy contrived) example:

var Promise = require("./lib/testable-implementation.js");
var atAtCreate = require("especially/well-known-symbols")["@@create"];
var OrdinaryConstruct = require("especially/abstract-operations").OrdinaryConstruct;
var set_slot = require("especially/meta").set_slot;



function evilPromise(executor) {
    function noop(){}
    function reject(err) { throw Error("Haha! " + err); }

    var self = this;

    // faking `InitialisePromise()`
    set_slot(self, "[[PromiseState]]", "pending");
    set_slot(self, "[[PromiseFulfillReactions]]", []);
    set_slot(self, "[[PromiseRejectReactions]]", []);

    // faking some chainable functions
    self.then = function $then$() { return Promise.prototype.then.apply(self,arguments); };
    self.catch = function $catch$() { return Promise.prototype.catch.apply(self,arguments); };

    executor(noop,reject);

    return self;
}

evilPromise[atAtCreate] = Promise[atAtCreate];
evilPromise.prototype = Object.create(Promise);
Object.defineProperty(evilPromise.prototype,"constructor",{
    writable: true,
    enumerable: false,
    configurable: true,
    value: evilPromise
});

var p = Promise.reject(42);

// now hijack `p` so any implicitly constructed promise is our evil one
p.constructor = evilPromise;

p.catch(function(err){
    console.log("err: " + err);
    throw "Oops";
})
// this promise returned here is one of the evil ones
.catch(function(err2){
    console.log("err2: " + err2);
});

// err: 42
// Unhandled exception: Haha! Oops

The err2: Oops you would normally want to print out here is prevented because the throw "Oops" triggers the evil promise's function reject(err) { throw Error("Haha! " + err); }, which itself then cannot signal the second catch(..) handler to fire.

I can't think of any other options besides "throw unhandled exception" (reference implementation) and "silently swallow" (v8). Is there?

I imagine the spec says exactly what to do in this case, but I was unsuccessful after several read-throughs to nail down exactly where. I started in 25.4.5.3, and then sub-step 13.b led me to 25.4.2.1. But then I got lost and wasn't certain I was on the right track. I didn't see clearly anything that led to the "throw unhandled exception" conclusion.

Appreciate any guidance/clarification.

[domenic]

This is not actually defined by the ES6 spec, since the behavior of Tasks is so implementation-dependent and fuzzy. In particular when you get an abrupt completion and call NextTask on it, you run into https://people.mozilla.org/~jorendorff/es6-draft.html#sec-nexttask-result step 1. In Node's microtask queue, "implementation-defined unhandled exception processing" triggers process.on('uncaughtException'). In v8's microtask queue, it apparently does nothing.

[getify]

Thanks for the clarification.

Does that mean if I browserify'd the reference implementation and ran in it with the above test code in chrome, you'd expect the error to also be silent there?

I'm surprised by that, because setTimeout(function(){ throw "wtf"; },0) in chrome definitely does throw as unhandled.

[domenic]

I don't know; it would depend on how browserify's process.nextTick works. If it was implemented in terms of promises, it would use the same microtask implementation that promises do, and I would expect that indeed. But last time I checked it used postMessage or MessageChannel or similar, which is a macrotask, and indeed behaves similarly to setTimeout(..., 0).

[domenic]

Oh, that said, I just re-reviewed your example, and noticed that you set internal slots to fake out the spec. That's not really possible since JavaScript never has access to internal slots. But I think a similar result can happen with subclassing, i.e.

function EvilPromise(executor) {
  Promise.call(this, function (ignored1, ignored2) { executor(noop, reject) });
}

EvilPromise.prototype.__proto__ = Promise.prototype;
EvilPromise.__proto__ = Promise;

[getify]

Yes, I did do that to fake out the ref impl, and yes I agree sub-classing could come across the same result. FTR, here's the code I was using in chrome to "test":

function evilPromise(executor) {
    function noop(){}
    function failure(err) { throw Error("Haha! " + err); }

    executor(noop,failure);

    return { then:noop, catch:noop };
}

var p = Promise.reject(42);
p.constructor = evilPromise;

p.catch(function(err){
    console.log("err: " + err);
    throw "Oops";
})
.catch(function(err2){
    console.log("err2: " + err2);
});

[allenwb]

On Jun 28, 2014, at 1:04 AM, Domenic Denicola wrote:

This is not actually defined by the ES6 spec, since the behavior of Tasks is so implementation-dependent and fuzzy. In particular when you get an abrupt completion and call NextTask on it, you run into https://people.mozilla.org/~jorendorff/es6-draft.html#sec-nexttask-result step 1. In Node's microtask queue, "implementation-defined unhandled exception processing" triggers process.on('uncaughtException'). In v8's microtask queue, it apparently does nothing.

No, that's not correct. Task scheduling is not an issue here.

Basically, an implementation of ES6 promises that throws an exception from a [[Reject]] function is not a conforming implementation. If you trace through the specification of "Promise Reject Functions" http://people.mozilla.org/~jorendorff/es6-draft.html#sec-promise-reject-functions , the abstract operations it calls, and the algorithms that actually schedule execution of a [[Reject]] function you shouldn't anyplace where an abrupt completion is generated. Also, there is no specified may to set the [[Reject]] fuction to any callable value other than the specified function. That all means that the corresponding implementation must not throw any exceptions. If yours does, it's non-conforming.

In other words, evilPromise is not valid implementation of Promise as specified in ES6.

Allen

[domenic]

Allen, the [[Reject]] function is derived from user code in the case of e.g. subclassing via the complicated NewPromiseCapability dance. Thus it can throw. There can indeed be subclasses that are not conformant spec implementations, but NewPromiseCapability will still be called on such subclass constructors.

[getify]

If it's something about spec compliance, it needs a test to verify, right?

[domenic]

Well, there's no way to verify that an implementation performs "implementation-defined unhandled exception processing," so I'm not sure how you could write such a test generically.

[allenwb]

@domenic You're right, the weird, GetCapabilitiesExecutor technique for could be used by a subclass to insert alternative function that could throw. Even so, the paths the abrupt completions will take should all be well defined. Any uncaught exceptions that occurs in Promise related tasks trigger the implementation defined NextTask unhandled exception handling in step 1, like you mentioned in an earlier comment. But, other than the actual implementation specific behavior I don't think there is anything fuzzy about that.

[getify]

I'm sure this is pointless spitting in the wind by now, but I'd say we shouldn't leave a gap where any error handling in promises is "implementation dependent", even in weird corner cases like evil/stupid sub-classes. How hard would it be to say "No error can be swallowed or lost" and then leave it up to the implementations to decide how they adhere?

[domenic]

Generally the strategy we use for such things is to let browsers compete on developer experience. If you can't normatively say how something affects the language semantics, then it doesn't belong in a normative specification.

[allenwb]

@getify It's exactly the specification path that is taken if you write a script that just says:

throw "foo";

It isn't clear what could be said that is more specific without creating a whole new unhanded exception mechanism. I gather that some people may be interested in doing that for ES7.

[domenic]

Oh, good point, I didn't realize that @allenwb, but yes now that general script execution semantics are encapsulated by the task model, that is definitely true.

I can't see how we could write tests for it, but that definitely does mean you've found a bug in V8, since they don't behave the same.

[anba]

I can't see how we could write tests for it, but that definitely does mean you've found a bug in V8, since they don't behave the same.

The term perform implementation defined unhandled exception processing [8.4.2 NextTask] does not imply to perform the same actions for every abrupt completion. So I'd say ignoring some abrupt completions is permitted by the spec as currently written. If this is not the intention, the spec text should be adjusted accordingly.

Personally I'd prefer [1, 2] if the uncaught exception handling in implementations always needs to perform the same actions. For example this will make tc39/test262#34 somewhat easier to implement, because you can use this function:

function reportFailure(reason) {
  let p = Promise.reject(reason);
  p.constructor = function(r) {
    r(() => {}, e => { throw e });
  };
  p.then();
}

To implement tests like this one:

Promise
  .resolve(0)
  .then(v => { assertSame(0, v) }, v => fail `fulfilled with ${v}`)
  .catch(reportFailure);

And the existing @negative attribute for test262 tests let's you define if errors are supposed to happen or not.

[1] tc39/test262#24 (comment)
[2] #107 (comment)

[getify]

ignoring some abrupt completions is permitted by the spec as currently written. If this is not the intention, the spec text should be adjusted accordingly.

my contention would be that, at least for promises, swallowing promises is never acceptable. in fact, i can't think of a single case where that should be allowed (promises or not), but i'm not actually making that assertion -- only specifically about promises. On the label of the box, promises have a pre-defined mechanism for catching errors, and if they don't catch errors, and those errors aren't statically thrown at construction or API call time, this creates foot-guns for devs. we should avoid those.

if that requires a spec wording change, i'd say one should be made. if it doesn't require a spec wording change, but the proper interpretation of existing terms, we need an authoritative statement to such that engines (like v8) can be pointed at.

i don't frankly care which. but i really hope the conclusion here is NOT to do nothing.