Closed
Bug 481218
Opened 15 years ago
Closed 15 years ago
js_IsCallable returns false for functions
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: sayrer, Assigned: jorendorff)
Details
(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
7.02 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
There should be a better way to do this.
Assignee | ||
Comment 1•15 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.
Comment 2•15 years ago
|
||
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•15 years ago
|
||
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•15 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.
Comment 5•15 years ago
|
||
Blake is at least as particular as I am, based on scars from past bugs (bug 305959 and bug 61911 at least). /be
Comment 6•15 years ago
|
||
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•15 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•15 years ago
|
||
Attachment #366678 -
Attachment is obsolete: true
Attachment #366712 -
Flags: review?(brendan)
Attachment #366678 -
Flags: review?(brendan)
Comment 9•15 years ago
|
||
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•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/d6c7c044e927
Whiteboard: fixed-in-tracemonkey
Reporter | ||
Comment 11•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d6c7c044e927
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: in-testsuite-
Reporter | ||
Comment 12•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8246e109352a
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•