547314 - Introduce ObjectOps for typeof

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

This would make the current code simpler and strictly faster since we already call js_GetWrappedObject, which we wouldn't do any more (the wrapper can unwrapper in the typeof hook). This helps proxies and other non-native objects to masquerade as functions vs objects etc.

Comment 1

[Brendan Eich [:brendan]]

I did this on the ssllab.org (UCI research, Andreas's old group) clone of tm. Patch coming up.

This suggests to look out for other js_GetWrappedObject callsites and see if they correspond to potential new object-ops.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: with bonus value type support (I will excise into another bug's patch)

Comment 3

[Andreas Gal :gal]

This needs tracing support.

Comment 4

[Brendan Eich [:brendan]]

This is great!

/be

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> This is great!

Hah! I updated the wrong bug -- that was meant for bug 547327.

/be

Comment 6

[Andreas Gal :gal]

Attachment: patch

Comment 7

[Andreas Gal :gal]

Attachment: patch

Make typeOf a mandatory part of ObjectOps so we can bypass JS_TypeOfValue and always go to obj->typeOf(cx). This might be controversial.

Comment 8

[Brendan Eich [:brendan]]

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

>+JSType
>+js_TypeOf(JSContext *cx, JSObject *obj)
>+{
>+    /*
>+     * Wrappers should also intercept js_TypeOf and answer accordingly.
>+     */
>+    JS_ASSERT(js_GetWrappedObject(cx, obj) == obj);
>+
>+    const JSObjectOps *ops = obj->map->ops;

Blank line here, or better: move this down under the comment and above the clasp decl.

>+    /*
>+     * ECMA 262, 11.4.3 says that any native object that implements
>+     * [[Call]] should be of type "function". However, RegExp is of
>+     * type "object", not "function", for Web compatibility.
>+     */
>+    JSClass *clasp = OBJ_GET_CLASS(cx, obj);
>+    if ((ops == &js_ObjectOps)
>+        ? (clasp->call
>+           ? clasp == &js_ScriptClass
>+           : clasp == &js_FunctionClass)
>+        : ops->call != NULL) {
>+        return JSTYPE_FUNCTION;
>+    } else {
>+#ifdef NARCISSUS
>+        JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED);
>+
>+        if (!obj->getProperty(cx, ATOM_TO_JSID(cx->runtime->atomState.__call__Atom),
>+                              &v)) {

Should this use js_GetMethod? Almost certainly, this code (from jsapi.cpp:JS_TypeOfValue) predates that.

>+            JS_ClearPendingException(cx);
>+        } else if (VALUE_IS_FUNCTION(cx, v)) {
>+           return JSTYPE_FUNCTION;
>+        }
>+#endif
>+    }
>+    return JSTYPE_OBJECT;
>+}
>+
> #ifdef JS_THREADSAFE
> void
> js_DropProperty(JSContext *cx, JSObject *obj, JSProperty *prop)
> {
>     JS_UNLOCK_OBJ(cx, obj);
> }
> #endif
> 
>diff --git a/js/src/jsobj.h b/js/src/jsobj.h
>--- a/js/src/jsobj.h
>+++ b/js/src/jsobj.h
>@@ -149,16 +149,17 @@ struct JSObjectOps {
>     JSPropertyIdOp      getProperty;
>     JSPropertyIdOp      setProperty;
>     JSAttributesOp      getAttributes;
>     JSAttributesOp      setAttributes;
>     JSPropertyIdOp      deleteProperty;
>     JSConvertOp         defaultValue;
>     JSNewEnumerateOp    enumerate;
>     JSCheckAccessIdOp   checkAccess;
>+    JSTypeOfOp          typeOf;
> 
>     /* Optionally non-null members start here. */
>     JSObjectOp          thisObject;
>     JSPropertyRefOp     dropProperty;
>     JSNative            call;
>     JSNative            construct;
>     JSHasInstanceOp     hasInstance;
>     JSTraceOp           trace;

It looks like trace is no longer optional -- move it up!

/be

Comment 9

[Andreas Gal :gal]

Attachment: patch

Comment 10

[Brendan Eich [:brendan]]

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

Nice -- the make-over (bug 408416) has begun!

/be

Comment 11

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/65eeef03da7c

Comment 12

[Andreas Gal :gal]

The patch crashes mochitests. All minidumps are useless (awesome, seriously). I am building a fresh build and try to reproduce. If that's not successful soon, I will back out.

Comment 13

[Andreas Gal :gal]

We can't bypass unwrapping in the default handler. XOWs don't overwrite object ops (???). Need to talk to Blake about this.

http://hg.mozilla.org/tracemonkey/rev/7024444d4ad1

Comment 14

[Andreas Gal :gal]

Another follow-up fix. I hit this on facebook. typeof null tried to do (JSObject*)null->typeOf on trace.

http://hg.mozilla.org/tracemonkey/rev/6538f7739a9f

Comment 15

[Brendan Eich [:brendan]]

Another followup, for an else-after-return nit that broke the common return at exit block structure of JS_TypeOfValue:

http://hg.mozilla.org/tracemonkey/rev/2925f17695e3

/be

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/65eeef03da7c