878543 - "Assertion failure: !cx->isExceptionPending()" with Proxy that throws itself

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

var grumpyCat = Proxy.create({
    has: function () {
        return true;
    },
    get: function () {
        throw grumpyCat;
    },
}, {});
grumpyCat.smile;

Assertion failure: !cx->isExceptionPending(), at js/src/jsinterp.cpp:3162

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

I think this is a regression from bug 739659. We're calling into getters and setters (and proxy hooks) with an exception already on cx. The code should probably be reworked to set aside the exception as it inspects the exception object and bail if trying to get any of the additional properties throws again.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

In particular, http://hg.mozilla.org/mozilla-central/annotate/8aca531ff163/js/src/jsexn.cpp#l1040 is where we re-set the exception and continue to inspect the object in case of failure.

Comment 3

[Luke Wagner [:luke]]

Hmm, it's kindof crazy that we are even running content-visible code here at all.  I was trying to figure out whether we could technically run user code before bug 739659: seems like 'yes' if the thrown Error object was Object.defineProperty'd to have a getter on 'message', so this isn't really worse.

IIUC, the fix here is just to JS_ClearPendingException again, right before we report the error?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Luke Wagner [:luke] from comment #3)
> IIUC, the fix here is just to JS_ClearPendingException again, right before
> we report the error?

The complete fix is going to be to JS_ClearPendingException after any of the calls that run user code (and there's quite a few of them before we actually try to report the exception).

Comment 5

[Jeff Walden [:Waldo]]

Well, really we shouldn't be doing anything user-visible at all when reporting an uncaught exception, aside from calling window.onerror if it's been set.  That's the primordial sin.  Not clearing pending exceptions after ToString, or whatever, does nothing about that incorrect behavior.

Comment 6

[Luke Wagner [:luke]]

(In reply to Blake Kaplan (:mrbkap) from comment #4)
Yes, what I said :)

Comment 7

[Luke Wagner [:luke]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
That was my first reaction, but I thought "eh, we've been doing it forever" ;)

But you're right.  Problem is: is using js::HasDataProperty enough, or do we have to whitelist a set of getters that we are willing to call?

Comment 8

[Jeff Walden [:Waldo]]

js::HasDataProperty would be enough to safely detect extant properties.  For error objects the stuff you might want, like message/etc., only appears via resolve hook -- bug 724768.  We'd need to fix that (it's really not too bad, I have half of a patch for it) if effects include .message-getting.

In this case it's a ToString() occurring.  We probably need some sort of EffectFreeToString method, or somesuch, to solve that case.  It should be easy enough to start it off with just doing obj_toString-like behavior, then extend it to error objects and similar with more informative details.  Or for this particular case, since errors are the likeliest thing to be seen, we can hold for correct handling of error objects.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Let's see if JSBugMon can track this..

Comment 10

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

var grumpyCat = Proxy.create({
    has: function () {
        return true;
    },
    get: function () {
        throw grumpyCat;
    },
}, {});
grumpyCat.smile;

Asserts on m-c rev 599100c4ebfe.

Retrying for JSBugMon's sake.

Comment 12

[Chris Peterson [:cpeterson]]

Gary: is this Proxy exception bug still relevant? I can't repro on a debug build of Nightly 31.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Chris Peterson (:cpeterson) from comment #12)
> Gary: is this Proxy exception bug still relevant? I can't repro on a debug
> build of Nightly 31.

Yes, it is still relevant, it still reproduces on m-c rev 5010b38abf18, and is being tracked on JSBugMon.

Comment 14

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 15

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision d070787de8f7).

Comment 16

[Jesse Ruderman]

I bet this was fixed by the patch in bug 1054243. The testcase is similar enough.