js_IsCallable returns false for functions

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Robert Sayre, Assigned: jorendorff)

Tracking

({fixed1.9.1})

unspecified
x86
Mac OS X
fixed1.9.1
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 years ago
There should be a better way to do this.
(Assignee)

Comment 1

9 years ago
Per brendan, this shouldn't be null-safe and shouldn't take a cx parameter.

It can use STOBJ_GET_CLASS instead of OBJ_GET_CLASS.
Further discussion led to:

* make cx be a trailing param.
* JS_LOCK_OBJ the object before loading obj->map->ops since map could in an unlikely preemption race point to freed memory if obj is not locked.

Further discussion feeds into wiki and other bugs, mainly bug 408416.

/be
(Assignee)

Comment 3

9 years ago
Created attachment 366678 [details] [diff] [review]
v1

r?brendan because he's so particular. ;)

Note that in JS_TypeOfValue, the comment was incorrect.  The change corrects the comment and simplifies the code there, but doesn't change the behavior (except that it now locks, if obj is native).
Attachment #366678 - Flags: review?(brendan)
(Assignee)

Comment 4

9 years ago
Correction, this *is* a change in behavior, in the case of non-Script native objects that implement JSClass::call.  This seems like a bug fix to me, but could have unintended consequences.  Looking at it.

The patch passes js/tests.
Blake is at least as particular as I am, based on scars from past bugs (bug 305959 and bug 61911 at least).

/be
Comment on attachment 366678 [details] [diff] [review]
v1

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -655,26 +655,21 @@ JS_TypeOfValue(JSContext *cx, jsval v)
> #if JS_HAS_XML_SUPPORT
>             if (ops == &js_XMLObjectOps.base) {
>                 type = JSTYPE_XML;
>             } else
> #endif
>             {
>                 /*
>                  * ECMA 262, 11.4.3 says that any native object that implements
>-                 * [[Call]] should be of type "function". Note that RegExp and
>-                 * Script are both of type "function" for compatibility with
>-                 * older SpiderMonkeys.
>+                 * [[Call]] should be of type "function". However, RegExp is of
>+                 * type "object", not "function", for Web compatibility.
>                  */
>                 clasp = OBJ_GET_CLASS(cx, obj);
>-                if ((ops == &js_ObjectOps)
>-                    ? (clasp->call
>-                       ? clasp == &js_ScriptClass
>-                       : clasp == &js_FunctionClass)
>-                    : ops->call != NULL) {
>+                if (js_IsCallable(obj, cx) && clasp != &js_RegExpClass) {
>                     type = JSTYPE_FUNCTION;

Thanks for fixing the stale comment. But I'm pretty sure this change will cause problems (any XPC*Wrapper that has a call hook will be classified as a function by typeof).

> JSObject *
> js_ValueToCallableObject(JSContext *cx, jsval *vp, uintN flags)
> {
>-    JSObject *callable = JSVAL_IS_PRIMITIVE(*vp) ? NULL : JSVAL_TO_OBJECT(*vp);
>+    JSObject *callable = JSVAL_IS_OBJECT(*vp) ? JSVAL_TO_OBJECT(*vp) : NULL;
> 
>-    if (js_IsCallable(cx, callable)) {
>+    if (callable && !HAS_FUNCTION_CLASS(callable) && js_IsCallable(callable, cx))
>         *vp = OBJECT_TO_JSVAL(callable);
>-    } else {
>+    else
>         callable = js_ValueToFunctionObject(cx, vp, flags);

Looking over the code, there's no need for !HAS_FUNCTION_CLASS(callable) in the middle of the && chain since js_ValueToFunctionObject will return *vp as an object pointer straight away if it has Function class.

Perhaps it's better to let js_ValueToFunctionObject decide this, though. Or is there a stronger reason than avoiding duplicating its logic? Let me know if I'm missing something.

>+/* Infallible. */
>+extern JSBool
>+js_IsCallable(JSObject *obj, JSContext *cx);

Might be worth a few more words: "Infallible, therefore cx is last parameter instead of first."

/be
(Assignee)

Comment 7

9 years ago
(In reply to comment #6)
> > JSObject *
> > js_ValueToCallableObject(JSContext *cx, jsval *vp, uintN flags)
> > {
> >-    JSObject *callable = JSVAL_IS_PRIMITIVE(*vp) ? NULL : JSVAL_TO_OBJECT(*vp);
> >+    JSObject *callable = JSVAL_IS_OBJECT(*vp) ? JSVAL_TO_OBJECT(*vp) : NULL;
> > 
> >-    if (js_IsCallable(cx, callable)) {
> >+    if (callable && !HAS_FUNCTION_CLASS(callable) && js_IsCallable(callable, cx))
> >         *vp = OBJECT_TO_JSVAL(callable);
> >-    } else {
> >+    else
> >         callable = js_ValueToFunctionObject(cx, vp, flags);
> 
> Looking over the code, there's no need for !HAS_FUNCTION_CLASS(callable) in the
> middle of the && chain since js_ValueToFunctionObject will return *vp as an
> object pointer straight away if it has Function class.

I'll remove that.  I had misread the code in js_ValueToFunctionObject.

Thanks for the comments -- particularity is a virtue (in your case anyway).
(Assignee)

Comment 8

9 years ago
Created attachment 366712 [details] [diff] [review]
v2
Attachment #366678 - Attachment is obsolete: true
Attachment #366712 - Flags: review?(brendan)
Attachment #366678 - Flags: review?(brendan)
Comment on attachment 366712 [details] [diff] [review]
v2

> js_ValueToCallableObject(JSContext *cx, jsval *vp, uintN flags)
> {
>-    JSObject *callable = JSVAL_IS_PRIMITIVE(*vp) ? NULL : JSVAL_TO_OBJECT(*vp);
>+    JSObject *callable = JSVAL_IS_OBJECT(*vp) ? JSVAL_TO_OBJECT(*vp) : NULL;
> 
>-    if (js_IsCallable(cx, callable)) {
>+    if (callable && js_IsCallable(callable, cx))
>         *vp = OBJECT_TO_JSVAL(callable);
>-    } else {
>+    else
>         callable = js_ValueToFunctionObject(cx, vp, flags);
>-    }
>-
>     return callable;

This is almost all style mongering but it might help non-PGO'ed builds: the structure looks lopsided because only the else assigns to callable but the exit block is reached by both then and else paths. Instead,

    if (callable && js_IsCallable(callable, cx))
        *vp = OBJECT_TO_JSVAL(callable);
        return callable;
    }
    return js_ValueToFunctionObject(cx, vp, flags);

says to my reading more what is going on. r=me with this nit picked.

/be
Attachment #366712 - Flags: review?(brendan) → review+
(Assignee)

Comment 10

9 years ago
http://hg.mozilla.org/tracemonkey/rev/d6c7c044e927
Whiteboard: fixed-in-tracemonkey
(Reporter)

Comment 11

9 years ago
http://hg.mozilla.org/mozilla-central/rev/d6c7c044e927
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.