607243 - Uncatchable exception with function proxy due to new'ing a builtin that returns a primitive

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

try { new (Proxy.createFunction({}, "".indexOf)); } catch(e) {} print("ok");

Does not print "ok".

Comment 1

[Igor Bukanov]

The bug has very little to do with proxies. Rather it comes from the interaction between  InvokeConstructor, http://hg.mozilla.org/tracemonkey/file/beb157e79468/js/src/jsinterp.cpp#l1215 , and JS_New http://hg.mozilla.org/tracemonkey/file/beb157e79468/js/src/jsapi.cpp#l4954 .

In the test case above InvokeConstructor ivokes String.prototype.indexOf as a constructor passing a newly created object as this. str_indexOf then normalizes it turning this into a string value and proceed to return -1 indicating failed search. As the function is a native InvokeConstructor does not treat it as an error. Rather InvokeConstructor sets the result to args.thisv() which is the normalized string value. Then JS_New see this primitive this and treats is as an error indicator.

I am not sure what is a good option here. We can fix JS_New signature to return a value and be compatible with the interpreter where typeof new "".indexOf is a string. Another possibility is to follow EcmaScript v5 and stop providing [Constructor] for built-in functions. Yet another possibility would be to use in InvokeConstructor the originally created object, not the normalized this value.

Comment 2

[Tom S [:evilpie]]

Removing the [Constructor] on built-ins is bug 202019.

Comment 3

[Brendan Eich [:brendan]]

I am going to fix bug 202019 this week, but it seems to me we have a latent bug to fix, under this bugzilla bug ideally, in JS_New, or in between it and InvokeConstructor. Silent failure is always a bug. Cc'ing luke.

/be

Comment 4

[Luke Wagner [:luke]]

Applying the SM-wide rule "if you take a cx and return false, you, or someone you called, must have already reported an error on the cx", the fault lies with JS_New, which is returning NULL and leaving cx->throwing unset.

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> Applying the SM-wide rule "if you take a cx and return false, you, or someone
> you called, must have already reported an error on the cx", the fault lies with
> JS_New, which is returning NULL and leaving cx->throwing unset.

The the trouble comes from the interaction between JS_New and InvokeConstructor. The latter currently can return a primitive result. With the bug 202019 fixed we can fix that and make sure that InvokeConstructor always return an object or error.

Comment 6

[Luke Wagner [:luke]]

I believe InvokeConstructor will continue to be able to return primitives, namely via calls to proxied constructors (bug 593277).

How is it not a bug for JS_New to return null without having cx->throwing set?  JSProxyHandler::construct sure seems to assume so.

Comment 7

[Brendan Eich [:brendan]]

Bug in JS_New -- who will fix? Needs fixing.

/be

Comment 8

[Igor Bukanov]

(In reply to comment #6)
> I believe InvokeConstructor will continue to be able to return primitives,
> namely via calls to proxied constructors (bug 593277).

Ok, then we should fix JS_New to return jsval, not JSObject. But it is not clear what to do with the usage of JS_New in ObjectWrapperChild::AnswerConstruct, http://mxr.mozilla.org/mozilla-central/source/js/ipc/ObjectWrapperChild.cpp#612 . Should that report a primitive result as an error?

Comment 9

[Luke Wagner [:luke]]

Alternatively, we could keep JS_New the same, have it report an error if it is returned a primitive, and, if necessary, add a JS_NewX (for some X) that does not report error for primitives.

Comment 10

[Brendan Eich [:brendan]]

In the interest of not fanning error-if-not-object checking out to JS_New callers, and getting ahead of ourselves supporting primitive returns from ctors, putting the check in JS_New seems best. Luke, can you knock this down fast? I will review. Thanks,

/be

Comment 11

[Luke Wagner [:luke]]

Attachment: patch

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 486942 [details] [diff] [review]
patch

>diff --git a/js/src/jit-test/tests/basic/testProxyConstructors.js b/js/src/jit-test/tests/basic/testProxyConstructors.js
>--- a/js/src/jit-test/tests/basic/testProxyConstructors.js
>+++ b/js/src/jit-test/tests/basic/testProxyConstructors.js
>@@ -1,9 +1,16 @@
>+// |jit-test| error: ExitCleanly
 . . .
>+// not an error
>+new (Proxy.createFunction({}, "".indexOf));
>+
>+throw "ExitCleanly"

What catches this thrown string and ignores it, or is it ignored? I couldn't find anything using this convention.

>+    JSObject *obj = NULL;
>+    if (ok) {
>+        if (args.rval().isObject()) {
>+            obj = &args.rval().toObject();
>+        } else {
>+            /*
>+             * Although constructors may return primitives (via proxies), this
>+             * API is asking for an object, so we report an error.
>+             */
>+            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                 JSMSG_BAD_NEW_RESULT,

Nit: could pull that JSMSG_BAD_NEW_RESULT up and go past column 80 ;-).

Looks great, thanks.

/be

Comment 13

[Luke Wagner [:luke]]

(In reply to comment #12)
> >+// |jit-test| error: ExitCleanly
>  . . .
> >+// not an error
> >+new (Proxy.createFunction({}, "".indexOf));
> >+
> >+throw "ExitCleanly"
> 
> What catches this thrown string and ignores it, or is it ignored? I couldn't
> find anything using this convention.

The magic cookie "|jit-test| error: ExitCleanly" means that this test will fail unless it exits via a thrown "ExitCleanly".  This is the only way I could think of to cause silent exit to be a jit-test failure.  This, of course, is me being lazy and not wanting to turn silent exit into an error and fixing any new failures that crop up, but I can file a follow-up bug.

Comment 14

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/04272020d873

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/04272020d873