Closed Bug 489171 Opened 15 years ago Closed 15 years ago

js_SetPropertyHelper does not null *entryp for read-only properties

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

Details

(Keywords: fixed1.9.1, regression, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 3 obsolete files)

Attached file js shell test case
js_SetPropertyHelper, when it hits a read-only property, does not set the cache entry pointer to null. Thus, after the function returns to the JSOP_SETPROP case in the interpreter, that pointer still refers to the cache entry set in js_FullTestPropertyCache.

Without the jit, this has no consequences, but with the jit enabled that entry is passed to record_SetPropMis and at least can be used there to change a value of a read-only property of the global object. 

The attached example for js shell demonstrates that via changing the value of a global const in the optimized build. In a debug build it triggers the asserts in SetPropMis that verifies that the cache entry points to the right JSScopeProperty instance.
Flags: blocking1.9.1?
A simple fix would be to turn the assert into an abort check. Would you be ok with that simple fix and we fork out a bug to actually clear the cache entry (if we think we want to clear it?).
We don't need to clear the cache entry. The cache is purged only all at once, or for a manually created/destroyed script, or by capability (shape) regeneration.

/be
(In reply to comment #2)
> We don't need to clear the cache entry.

My initial title for the bug was misleading. The bug is just that js_SetPropertyHelper does not set *entryp to NULL to indicate no caching.
Assignee: general → igor
Summary: js_SetPropertyHelper does not null its cache entry argument for read-only properties → js_SetPropertyHelper does not null *entryp for read-only properties
Attached patch patch (obsolete) — Splinter Review
This only fixes the one specific case. igor is looking for other related problems.
Attached patch v1 (obsolete) — Splinter Review
There are two similar bugs in the code. Both js_SetPropertyHelper and js_DefineNativeProperty misses *entryp = NULL for shared properties.

To eliminate such bugs once and for all the patch changes js_FillPropertyCache and all its callers to return the filled entry via function's result and not via an optional out parameter.

As this bug demonstrates once again, such optional out-params are rather error prone. One way to make them less dangerous is to require the caller to initialize the optional out parameter to null or false with an assert at the beginning of the callee insisting on that. Even better if such parameters can be removed. 

The latter is possible here since to distinguish an OOM-error from no-cache-fills one can use a special sentinel value. For that the patch picks up ((JSPropCacheEntry *) NULL + 1).  

I have not tested the patch yet.
Comment on attachment 373706 [details] [diff] [review]
patch

>             if (attrs & JSPROP_READONLY) {
>                 if (!JS_HAS_STRICT_OPTION(cx)) {
>                     /* Just return true per ECMA if not in strict mode. */
>                     PCMETER(!entryp || JS_PROPERTY_CACHE(cx).rofills++);
>+                    if (entryp)
>+                        *entryp = NULL;
>                     return JS_TRUE;
>                 }
> 
>                 /* Strict mode: report a read-only strict warning. */
>                 flags = JSREPORT_STRICT | JSREPORT_WARNING;

This is not enough to fix this particular case fully: in the strict mode read_only_error generates just a warning, not an error.
Maybe split the refactoring into a separate bug? I think a narrow minimal fix of this problem and the two other sites might make the drivers happy.
(In reply to comment #7)
> Maybe split the refactoring into a separate bug? I think a narrow minimal fix
> of this problem and the two other sites might make the drivers happy.

I am biased in judging the answer because it was exactly the re-factoring that discivered the two other sites and a minimal fix would require to spend extra time resolving inevitable merge conflicts. 

But if a minimal fix is required, I can have it either tomorrow or can review it later today if it would be ready within 2 hours.
Attached patch v2 (obsolete) — Splinter Review
In the new version, besides whitespace and comment fixes, I fixed the bug in jstracer.cpp where in v1 I forgot to change the result of js_FillPropertyCache from JS_NO_PROP_CACHE_FILL to null. But that was exposed by the shell tests.

The new patch passes shell tests and does not regress on sunspider or trace-test.js timing. But I have not yet have results from the try server.
Attachment #373706 - Attachment is obsolete: true
Attachment #373712 - Attachment is obsolete: true
Attachment #373729 - Flags: review?(brendan)
Attachment #373729 - Flags: review?(gal)
Comment on attachment 373729 [details] [diff] [review]
v2

>Index: tm/js/src/jsobj.cpp
>===================================================================
>--- tm.orig/js/src/jsobj.cpp    2009-04-20 22:41:02.000000000 +0200
>+++ tm/js/src/jsobj.cpp    2009-04-20 22:41:27.000000000 +0200
>@@ -3678,18 +3678,18 @@ js_ChangeNativePropertyAttrs(JSContext *
>     return sprop;
> }
> 
> JSBool
> js_DefineProperty(JSContext *cx, JSObject *obj, jsid id, jsval value,
>                   JSPropertyOp getter, JSPropertyOp setter, uintN attrs,
>                   JSProperty **propp)
> {
>-    return js_DefineNativeProperty(cx, obj, id, value, getter, setter, attrs,
>-                                   0, 0, propp);
>+    return !!js_DefineNativeProperty(cx, obj, id, value, getter, setter, attrs,
>+                                     0, 0, propp);
> }
> 
> /*
>  * Backward compatibility requires allowing addProperty hooks to mutate the
>  * nominal initial value of a slot-full property, while GC safety wants that
>  * value to be stored before the call-out through the hook.  Optimize to do
>  * both while saving cycles for classes that stub their addProperty hook.
>  *
>@@ -3705,25 +3705,26 @@ js_DefineProperty(JSContext *cx, JSObjec
>             }                                                                 \
>             if (*(vp) != nominal_) {                                          \
>                 if (SPROP_HAS_VALID_SLOT(sprop, scope))                       \
>                     LOCKED_OBJ_WRITE_BARRIER(cx, obj, (sprop)->slot, *(vp));  \
>             }                                                                 \
>         }                                                                     \
>     JS_END_MACRO
> 
>-JSBool
>+JSPropCacheEntry *
> js_DefineNativeProperty(JSContext *cx, JSObject *obj, jsid id, jsval value,
>                         JSPropertyOp getter, JSPropertyOp setter, uintN attrs,
>                         uintN flags, intN shortid, JSProperty **propp,
>-                        JSPropCacheEntry** entryp /* = NULL */)
>+                        JSBool cacheResult /* = JS_FALSE */)
> {
>     JSClass *clasp;
>     JSScope *scope;
>     JSScopeProperty *sprop;
>+    JSPropCacheEntry *entry;
> 
>     js_LeaveTraceIfGlobalObject(cx, obj);
> 
>     /* Convert string indices to integers if appropriate. */
>     CHECK_FOR_STRING_INDEX(id);
> 
>     uint32 shape = OBJ_SHAPE(obj);
> 
>@@ -3742,17 +3743,17 @@ js_DefineNativeProperty(JSContext *cx, J
>          * If JS_THREADSAFE and id is found, js_LookupProperty returns with
>          * sprop non-null and pobj locked.  If pobj == obj, the property is
>          * already in obj and obj has its own (mutable) scope.  So if we are
>          * defining a getter whose setter was already defined, or vice versa,
>          * finish the job via js_ChangeScopePropertyAttributes, and refresh
>          * the property cache line for (obj, id) to map sprop.
>          */
>         if (!js_LookupProperty(cx, obj, id, &pobj, &prop))
>-            return JS_FALSE;
>+            return NULL;
>         sprop = (JSScopeProperty *) prop;
>         if (sprop &&
>             pobj == obj &&
>             (sprop->attrs & (JSPROP_GETTER | JSPROP_SETTER))) {
>             sprop = js_ChangeScopePropertyAttrs(cx, OBJ_SCOPE(obj), sprop,
>                                                 attrs, sprop->attrs,
>                                                 (attrs & JSPROP_GETTER)
>                                                 ? getter
>@@ -3809,28 +3810,29 @@ js_DefineNativeProperty(JSContext *cx, J
>     if (SPROP_HAS_VALID_SLOT(sprop, scope))
>         LOCKED_OBJ_WRITE_BARRIER(cx, obj, sprop->slot, value);
> 
>     /* XXXbe called with lock held */
>     ADD_PROPERTY_HELPER(cx, clasp, obj, scope, sprop, &value,
>                         js_RemoveScopeProperty(cx, scope, id);
>                         goto bad);

optional Nit: I would declare entryp here. Makes it easier to track its definitions. We are doing this patch to avoid missing exit paths, after all.

> 
>-    if (entryp) {
>+    entry = JS_NO_PROP_CACHE_FILL;
>+    if (cacheResult) {
>         JS_ASSERT_NOT_ON_TRACE(cx);
>         if (!(attrs & JSPROP_SHARED))
>-            js_FillPropertyCache(cx, obj, shape, 0, 0, obj, sprop, entryp);
>+            entry = js_FillPropertyCache(cx, obj, shape, 0, 0, obj, sprop);
>         else
>             PCMETER(JS_PROPERTY_CACHE(cx).nofills++);
>     }
>     if (propp)
>         *propp = (JSProperty *) sprop;
>     else
>         JS_UNLOCK_OBJ(cx, obj);
>-    return JS_TRUE;
>+    return entry;
> 
> bad:
>     JS_UNLOCK_OBJ(cx, obj);
>     return JS_FALSE;

Shouldn't this be NULL? Could you double check by grepping for all "return "s.

> }
> 
> JS_FRIEND_API(JSBool)
> js_LookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
>@@ -4027,44 +4029,44 @@ IsCacheableNonGlobalScope(JSObject *obj)
>     bool cacheable = (clasp == &js_CallClass ||
>                       clasp == &js_BlockClass ||
>                       clasp == &js_DeclEnvClass);
> 
>     JS_ASSERT_IF(cacheable, obj->map->ops->lookupProperty == js_LookupProperty);
>     return cacheable;
> }
> 
>-int
>-js_FindPropertyHelper(JSContext *cx, jsid id, JSObject **objp,
>-                      JSObject **pobjp, JSProperty **propp,
>-                      JSPropCacheEntry **entryp)
>+JSPropCacheEntry *
>+js_FindPropertyHelper(JSContext *cx, jsid id, JSBool cacheResult,
>+                      JSObject **objp, JSObject **pobjp, JSProperty **propp)
> {
>     JSObject *scopeChain, *obj, *parent, *pobj;
>     uint32 shape;
>+    JSPropCacheEntry *entry;
>     int scopeIndex, protoIndex;
>     JSProperty *prop;
>-    JSScopeProperty *sprop;
> 
>-    JS_ASSERT_IF(entryp, !JS_ON_TRACE(cx));
>+    JS_ASSERT_IF(cacheResult, !JS_ON_TRACE(cx));
>     scopeChain = js_GetTopStackFrame(cx)->scopeChain;
>     shape = OBJ_SHAPE(scopeChain);
> 
>     /* Scan entries on the scope chain that we can cache across. */
>+    entry = JS_NO_PROP_CACHE_FILL;
>     obj = scopeChain;
>     parent = OBJ_GET_PARENT(cx, obj);
>     for (scopeIndex = 0;
>          parent
>          ? IsCacheableNonGlobalScope(obj)
>          : obj->map->ops->lookupProperty == js_LookupProperty;
>          ++scopeIndex) {
>         protoIndex =
>             js_LookupPropertyWithFlags(cx, obj, id, cx->resolveFlags,
>                                        &pobj, &prop);
>         if (protoIndex < 0)
>-            return false;
>+            return NULL;
> 
>         if (prop) {
> #ifdef DEBUG
>             if (parent) {
>                 JSClass *clasp = OBJ_GET_CLASS(cx, obj);
>                 JS_ASSERT(OBJ_IS_NATIVE(pobj));
>                 JS_ASSERT(OBJ_GET_CLASS(cx, pobj) == clasp);
>                 if (clasp == &js_BlockClass) {
>@@ -4077,41 +4079,38 @@ js_FindPropertyHelper(JSContext *cx, jsi
>                     JS_ASSERT(protoIndex == 1);
>                 } else {
>                     /* Call and DeclEnvClass objects have no prototypes. */
>                     JS_ASSERT(!OBJ_GET_PROTO(cx, obj));
>                     JS_ASSERT(protoIndex == 0);
>                 }
>             }
> #endif
>-            if (entryp) {
>-                sprop = (JSScopeProperty *) prop;
>-                js_FillPropertyCache(cx, scopeChain, shape,
>-                                     scopeIndex, protoIndex, pobj, sprop,
>-                                     entryp);
>+            if (cacheResult) {
>+                entry = js_FillPropertyCache(cx, scopeChain, shape,
>+                                             scopeIndex, protoIndex, pobj,
>+                                             (JSScopeProperty *) prop);
>             }
>             SCOPE_DEPTH_ACCUM(&rt->scopeSearchDepthStats, scopeIndex);
>             goto out;
>         }
> 
>         if (!parent) {
>             pobj = NULL;
>             goto out;
>         }
>         obj = parent;
>         parent = OBJ_GET_PARENT(cx, obj);
>     }
> 
>     for (;;) {
>         if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &pobj, &prop))
>-            return false;
>+            return NULL;
>         if (prop) {
>             PCMETER(JS_PROPERTY_CACHE(cx).nofills++);
>-            if (entryp)
>-                *entryp = NULL;
>             goto out;
>         }
> 
>         /*
>          * We conservatively assume that a resolve hook could mutate the scope
>          * chain during OBJ_LOOKUP_PROPERTY. So we read parent here again.
>          */
>         parent = OBJ_GET_PARENT(cx, obj);
>@@ -4122,37 +4121,35 @@ js_FindPropertyHelper(JSContext *cx, jsi
>         obj = parent;
>     }
> 
>   out:
>     JS_ASSERT(!!pobj == !!prop);
>     *objp = obj;
>     *pobjp = pobj;
>     *propp = prop;
>-    return true;
>+    return entry;
> }
> 
> JS_FRIEND_API(JSBool)
> js_FindProperty(JSContext *cx, jsid id, JSObject **objp, JSObject **pobjp,
>                 JSProperty **propp)
> {
>-    return js_FindPropertyHelper(cx, id, objp, pobjp, propp, NULL) >= 0;
>+    return !!js_FindPropertyHelper(cx, id, false, objp, pobjp, propp);
> }
> 
> JSObject *
>-js_FindIdentifierBase(JSContext *cx, JSObject *scopeChain, jsid id,
>-                      JSPropCacheEntry *entry)
>+js_FindIdentifierBase(JSContext *cx, JSObject *scopeChain, jsid id)
> {
>     /*
>      * This function should not be called for a global object or from the
>      * trace and should have a valid cache entry for native scopeChain.
>      */
>     JS_ASSERT(OBJ_GET_PARENT(cx, scopeChain));
>     JS_ASSERT(!JS_ON_TRACE(cx));
>-    JS_ASSERT_IF(OBJ_IS_NATIVE(scopeChain), entry);
> 
>     JSObject *obj = scopeChain;
> 
>     /*
>      * Loop over cacheable objects on the scope chain until we find a
>      * property. We also stop when we reach the global object skipping any
>      * farther checks or lookups. For details see the JSOP_BINDNAME case of
>      * js_Interpret.
>@@ -4163,19 +4160,23 @@ js_FindIdentifierBase(JSContext *cx, JSO
>         int protoIndex = js_LookupPropertyWithFlags(cx, obj, id,
>                                                     cx->resolveFlags,
>                                                     &pobj, &prop);
>         if (protoIndex < 0)
>             return NULL;
>         if (prop) {
>             JS_ASSERT(OBJ_IS_NATIVE(pobj));
>             JS_ASSERT(OBJ_GET_CLASS(cx, pobj) == OBJ_GET_CLASS(cx, obj));
>+#ifdef DEBUG
>+            JSPropCacheEntry *entry =
>+#endif

This might need some (void) insanity or something like that to make MSVC happy.

>             js_FillPropertyCache(cx, scopeChain, OBJ_SHAPE(scopeChain),
>                                  scopeIndex, protoIndex, pobj,
>-                                 (JSScopeProperty *) prop, &entry);
>+                                 (JSScopeProperty *) prop);
>+            JS_ASSERT(entry);
>             JS_UNLOCK_OBJ(cx, pobj);
>             return obj;
>         }
> 
>         /* Call and other cacheable objects always have a parent. */
>         obj = OBJ_GET_PARENT(cx, obj);
>         if (!OBJ_GET_PARENT(cx, obj))
>             return obj;
>@@ -4305,45 +4306,42 @@ js_NativeSet(JSContext *cx, JSObject *ob
>   set_slot:
>         LOCKED_OBJ_WRITE_BARRIER(cx, obj, slot, *vp);
>     }
> 
>     return JS_TRUE;
> }
> 
> JSBool
>-js_GetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, jsval *vp,
>-                     JSPropCacheEntry **entryp)
>+js_GetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, JSBool cacheResult,
>+                     jsval *vp)
> {
>     JSObject *aobj, *obj2;
>     uint32 shape;
>     int protoIndex;
>     JSProperty *prop;
>     JSScopeProperty *sprop;
> 
>-    JS_ASSERT_IF(entryp, !JS_ON_TRACE(cx));
>+    JS_ASSERT_IF(cacheResult, !JS_ON_TRACE(cx));
>     /* Convert string indices to integers if appropriate. */
>     CHECK_FOR_STRING_INDEX(id);
> 
>     aobj = js_GetProtoIfDenseArray(cx, obj);
>     shape = OBJ_SHAPE(aobj);
>     protoIndex = js_LookupPropertyWithFlags(cx, aobj, id, cx->resolveFlags,
>                                             &obj2, &prop);
>     if (protoIndex < 0)
>         return JS_FALSE;
>     if (!prop) {
>         *vp = JSVAL_VOID;
> 
>         if (!OBJ_GET_CLASS(cx, obj)->getProperty(cx, obj, ID_TO_VALUE(id), vp))
>             return JS_FALSE;
> 
>-        if (entryp) {
>-            PCMETER(JS_PROPERTY_CACHE(cx).nofills++);
>-            *entryp = NULL;
>-        }
>+        PCMETER(cacheResult && JS_PROPERTY_CACHE(cx).nofills++);
> 
>         /*
>          * Give a strict warning if foo.bar is evaluated by a script for an
>          * object foo with no property named 'bar'.
>          */
>         jsbytecode *pc;
>         if (JSVAL_IS_VOID(*vp) && ((pc = js_GetCurrentBytecodePC(cx)) != NULL)) {
>             JSOp op;
>@@ -4396,39 +4394,39 @@ js_GetPropertyHelper(JSContext *cx, JSOb
>         OBJ_DROP_PROPERTY(cx, obj2, prop);
>         return OBJ_GET_PROPERTY(cx, obj2, id, vp);
>     }
> 
>     sprop = (JSScopeProperty *) prop;
>     if (!js_NativeGet(cx, obj, obj2, sprop, vp))
>         return JS_FALSE;
> 
>-    if (entryp) {
>+    if (cacheResult) {
>         JS_ASSERT_NOT_ON_TRACE(cx);
>-        js_FillPropertyCache(cx, aobj, shape, 0, protoIndex, obj2, sprop, entryp);
>+        js_FillPropertyCache(cx, aobj, shape, 0, protoIndex, obj2, sprop);
>     }
>     JS_UNLOCK_OBJ(cx, obj2);
>     return JS_TRUE;
> }
> 
> JSBool
> js_GetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
> {
>-    return js_GetPropertyHelper(cx, obj, id, vp, NULL);
>+    return js_GetPropertyHelper(cx, obj, id, false, vp);
> }
> 
> JSBool
>-js_GetMethod(JSContext *cx, JSObject *obj, jsid id, jsval *vp,
>-             JSPropCacheEntry **entryp)
>+js_GetMethod(JSContext *cx, JSObject *obj, jsid id, JSBool cacheResult,
>+             jsval *vp)
> {
>     if (obj->map->ops == &js_ObjectOps ||
>         obj->map->ops->getProperty == js_GetProperty) {
>-        return js_GetPropertyHelper(cx, obj, id, vp, entryp);
>+        return js_GetPropertyHelper(cx, obj, id, cacheResult, vp);
>     }
>-    JS_ASSERT_IF(entryp, OBJ_IS_DENSE_ARRAY(cx, obj));
>+    JS_ASSERT_IF(cacheResult, OBJ_IS_DENSE_ARRAY(cx, obj));
> #if JS_HAS_XML_SUPPORT
>     if (OBJECT_IS_XML(cx, obj))
>         return js_GetXMLMethod(cx, obj, id, vp);
> #endif
>     return OBJ_GET_PROPERTY(cx, obj, id, vp);
> }
> 
> JS_FRIEND_API(JSBool)
>@@ -4447,30 +4445,31 @@ js_CheckUndeclaredVarAssignment(JSContex
> 
>     const char *bytes = js_AtomToPrintableString(cx, atom);
>     return bytes &&
>            JS_ReportErrorFlagsAndNumber(cx, JSREPORT_WARNING | JSREPORT_STRICT,
>                                         js_GetErrorMessage, NULL,
>                                         JSMSG_UNDECLARED_VAR, bytes);
> }
> 
>-JSBool
>-js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, jsval *vp,
>-                     JSPropCacheEntry **entryp)
>+JSPropCacheEntry *
>+js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, JSBool cacheResult,
>+                     jsval *vp)
> {
>     uint32 shape;
>     int protoIndex;
>     JSObject *pobj;
>     JSProperty *prop;
>     JSScopeProperty *sprop;
>     JSScope *scope;
>     uintN attrs, flags;
>     intN shortid;
>     JSClass *clasp;
>     JSPropertyOp getter, setter;
>+    JSPropCacheEntry *entry;
> 
>     /* Convert string indices to integers if appropriate. */
>     CHECK_FOR_STRING_INDEX(id);
> 
>     /*
>      * We peek at OBJ_SCOPE(obj) without locking obj. Any race means a failure
>      * to seal before sharing, which is inherently ambiguous.
>      */
>@@ -4478,17 +4477,17 @@ js_SetPropertyHelper(JSContext *cx, JSOb
>         flags = JSREPORT_ERROR;
>         goto read_only_error;
>     }
> 
>     shape = OBJ_SHAPE(obj);
>     protoIndex = js_LookupPropertyWithFlags(cx, obj, id, cx->resolveFlags,
>                                             &pobj, &prop);
>     if (protoIndex < 0)
>-        return JS_FALSE;
>+        return NULL;
>     if (prop) {
>         if (!OBJ_IS_NATIVE(pobj)) {
>             OBJ_DROP_PROPERTY(cx, pobj, prop);
>             prop = NULL;
>         }
>     } else {
>         /* We should never add properties to lexical blocks.  */
>         JS_ASSERT(OBJ_GET_CLASS(cx, obj) != &js_BlockClass);
>@@ -4533,18 +4532,18 @@ js_SetPropertyHelper(JSContext *cx, JSOb
>              * JS_ReportErrorFlagsAndNumberUC at label read_only_error.  We
>              * must likewise re-task flags further below for the other 'goto
>              * read_only_error;' case.
>              */
>             flags = JSREPORT_ERROR;
>             if (attrs & JSPROP_READONLY) {
>                 if (!JS_HAS_STRICT_OPTION(cx)) {
>                     /* Just return true per ECMA if not in strict mode. */
>-                    PCMETER(!entryp || JS_PROPERTY_CACHE(cx).rofills++);
>-                    return JS_TRUE;
>+                    PCMETER(cacheResult && JS_PROPERTY_CACHE(cx).rofills++);
>+                    return JS_NO_PROP_CACHE_FILL;
>                 }
> 
>                 /* Strict mode: report a read-only strict warning. */
>                 flags = JSREPORT_STRICT | JSREPORT_WARNING;
>             }
>             goto read_only_error;
>         }
> 
>@@ -4562,27 +4561,25 @@ js_SetPropertyHelper(JSContext *cx, JSOb
>              * Don't clone a shared prototype property. Don't fill it in the
>              * property cache either, since the JSOP_SETPROP/JSOP_SETNAME code
>              * in js_Interpret does not handle shared or prototype properties.
>              * Shared prototype properties require more hit qualification than
>              * the fast-path code for those ops, which is targeted on direct,
>              * slot-based properties.
>              */
>             if (attrs & JSPROP_SHARED) {
>-                if (entryp) {
>-                    PCMETER(JS_PROPERTY_CACHE(cx).nofills++);
>-                    *entryp = NULL;
>-                }
>-
>+                PCMETER(cacheResult && JS_PROPERTY_CACHE(cx).nofills++);
>                 if (SPROP_HAS_STUB_SETTER(sprop) &&
>                     !(sprop->attrs & JSPROP_GETTER)) {
>-                    return JS_TRUE;
>+                    return JS_NO_PROP_CACHE_FILL;
>                 }
> 
>-                return js_SetSprop(cx, sprop, obj, vp);
>+                return js_SetSprop(cx, sprop, obj, vp)
>+                       ? JS_NO_PROP_CACHE_FILL
>+                       : NULL;
>             }
> 
>             /* Restore attrs to the ECMA default for new properties. */
>             attrs = JSPROP_ENUMERATE;
> 
>             /*
>              * Preserve the shortid, getter, and setter when shadowing any
>              * property that has a shortid.  An old API convention requires
>@@ -4616,65 +4613,69 @@ js_SetPropertyHelper(JSContext *cx, JSOb
>          */
>         js_PurgeScopeChain(cx, obj, id);
> 
>         /* Find or make a property descriptor with the right heritage. */
>         JS_LOCK_OBJ(cx, obj);
>         scope = js_GetMutableScope(cx, obj);
>         if (!scope) {
>             JS_UNLOCK_OBJ(cx, obj);
>-            return JS_FALSE;
>+            return NULL;
>         }
>         if (clasp->flags & JSCLASS_SHARE_ALL_PROPERTIES)
>             attrs |= JSPROP_SHARED;
>         sprop = js_AddScopeProperty(cx, scope, id, getter, setter,
>                                     SPROP_INVALID_SLOT, attrs, flags, shortid);
>         if (!sprop) {
>             JS_UNLOCK_SCOPE(cx, scope);
>-            return JS_FALSE;
>+            return NULL;
>         }
> 
>         /*
>          * Initialize the new property value (passed to setter) to undefined.
>          * Note that we store before calling addProperty, to match the order
>          * in js_DefineNativeProperty.
>          */
>         if (SPROP_HAS_VALID_SLOT(sprop, scope))
>             LOCKED_OBJ_SET_SLOT(obj, sprop->slot, JSVAL_VOID);
> 
>         /* XXXbe called with obj locked */
>         ADD_PROPERTY_HELPER(cx, clasp, obj, scope, sprop, vp,
>                             js_RemoveScopeProperty(cx, scope, id);
>                             JS_UNLOCK_SCOPE(cx, scope);
>-                            return JS_FALSE);
>+                            return NULL);
>     }
> 
>     if (!js_NativeSet(cx, obj, sprop, vp))
>-        return JS_FALSE;
>+        return NULL;
> 
>-    if (entryp) {
>+    entry = JS_NO_PROP_CACHE_FILL;
>+    if (cacheResult) {
>         JS_ASSERT_NOT_ON_TRACE(cx);
>         if (!(attrs & JSPROP_SHARED))
>-            js_FillPropertyCache(cx, obj, shape, 0, 0, obj, sprop, entryp);
>+            entry = js_FillPropertyCache(cx, obj, shape, 0, 0, obj, sprop);
>         else
>             PCMETER(JS_PROPERTY_CACHE(cx).nofills++);
>+
>     }
>     JS_UNLOCK_SCOPE(cx, scope);
>-    return JS_TRUE;
>+    return entry;
> 
>   read_only_error:
>     return js_ReportValueErrorFlags(cx, flags, JSMSG_READ_ONLY,
>                                     JSDVG_IGNORE_STACK, ID_TO_VALUE(id), NULL,
>-                                    NULL, NULL);
>+                                    NULL, NULL)
>+           ? JS_NO_PROP_CACHE_FILL
>+           : NULL;
> }
> 
> JSBool
> js_SetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
> {
>-    return js_SetPropertyHelper(cx, obj, id, vp, NULL);
>+    return !!js_SetPropertyHelper(cx, obj, id, false, vp);
> }
> 
> JSBool
> js_GetAttributes(JSContext *cx, JSObject *obj, jsid id, JSProperty *prop,
>                  uintN *attrsp)
> {
>     JSBool noprop, ok;
>     JSScopeProperty *sprop;
>@@ -5570,17 +5571,17 @@ js_TryMethod(JSContext *cx, JSObject *ob
>     /*
>      * Report failure only if an appropriate method was found, and calling it
>      * returned failure.  We propagate failure in this case to make exceptions
>      * behave properly.
>      */
>     older = JS_SetErrorReporter(cx, NULL);
>     id = ATOM_TO_JSID(atom);
>     fval = JSVAL_VOID;
>-    ok = js_GetMethod(cx, obj, id, &fval, NULL);
>+    ok = js_GetMethod(cx, obj, id, false, &fval);
>     if (!ok)
>         JS_ClearPendingException(cx);
>     JS_SetErrorReporter(cx, older);
> 
>     if (JSVAL_IS_PRIMITIVE(fval))
>         return JS_TRUE;
>     return js_InternalCall(cx, obj, fval, argc, argv, rval);
> }
>Index: tm/js/src/jsapi.cpp
>===================================================================
>--- tm.orig/js/src/jsapi.cpp    2009-04-20 22:41:24.000000000 +0200
>+++ tm/js/src/jsapi.cpp    2009-04-20 22:41:27.000000000 +0200
>@@ -2980,18 +2980,18 @@ JS_ConstructObjectWithArguments(JSContex
> 
> static JSBool
> DefinePropertyById(JSContext *cx, JSObject *obj, jsid id, jsval value,
>                    JSPropertyOp getter, JSPropertyOp setter, uintN attrs,
>                    uintN flags, intN tinyid)
> {
>     if (flags != 0 && OBJ_IS_NATIVE(obj)) {
>         JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED | JSRESOLVE_DECLARING);
>-        return js_DefineNativeProperty(cx, obj, id, value, getter, setter,
>-                                       attrs, flags, tinyid, NULL);
>+        return !!js_DefineNativeProperty(cx, obj, id, value, getter, setter,
>+                                         attrs, flags, tinyid, NULL);
>     }
>     return OBJ_DEFINE_PROPERTY(cx, obj, id, value, getter, setter, attrs,
>                                NULL);   
> }
> 
> static JSBool
> DefineProperty(JSContext *cx, JSObject *obj, const char *name, jsval value,
>                JSPropertyOp getter, JSPropertyOp setter, uintN attrs,
>@@ -3024,19 +3024,19 @@ DefineUCProperty(JSContext *cx, JSObject
> {
>     JSAtom *atom;
> 
>     atom = js_AtomizeChars(cx, name, AUTO_NAMELEN(name, namelen), 0);
>     if (!atom)
>         return JS_FALSE;
>     if (flags != 0 && OBJ_IS_NATIVE(obj)) {
>         JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED | JSRESOLVE_DECLARING);
>-        return js_DefineNativeProperty(cx, obj, ATOM_TO_JSID(atom), value,
>-                                       getter, setter, attrs, flags, tinyid,
>-                                       NULL);
>+        return !!js_DefineNativeProperty(cx, obj, ATOM_TO_JSID(atom), value,
>+                                         getter, setter, attrs, flags, tinyid,
>+                                         NULL);
>     }
>     return OBJ_DEFINE_PROPERTY(cx, obj, ATOM_TO_JSID(atom), value,
>                                getter, setter, attrs, NULL);
> }
> 
> JS_PUBLIC_API(JSObject *)
> JS_DefineObject(JSContext *cx, JSObject *obj, const char *name, JSClass *clasp,
>                 JSObject *proto, uintN attrs)
>@@ -3543,17 +3543,17 @@ JS_GetPropertyById(JSContext *cx, JSObje
> 
> JS_PUBLIC_API(JSBool)
> JS_GetMethodById(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
>                  jsval *vp)
> {
>     JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED);
> 
>     CHECK_REQUEST(cx);
>-    if (!js_GetMethod(cx, obj, id, vp, NULL))
>+    if (!js_GetMethod(cx, obj, id, false, vp))
>         return JS_FALSE;
>     if (objp)
>         *objp = obj;
>     return JS_TRUE;
> }
> 
> JS_PUBLIC_API(JSBool)
> JS_GetMethod(JSContext *cx, JSObject *obj, const char *name, JSObject **objp,
>Index: tm/js/src/jsinterp.cpp
>===================================================================
>--- tm.orig/js/src/jsinterp.cpp    2009-04-20 22:41:02.000000000 +0200
>+++ tm/js/src/jsinterp.cpp    2009-04-20 22:41:27.000000000 +0200
>@@ -102,21 +102,20 @@ js_GenerateShape(JSContext *cx, JSBool g
>          * have a chance to wrap around shapeGen to zero.
>          */
>         rt->shapeGen = SHAPE_OVERFLOW_BIT;
>         js_TriggerGC(cx, gcLocked);
>     }
>     return shape;
> }
> 
>-JS_REQUIRES_STACK void
>+JS_REQUIRES_STACK JSPropCacheEntry *
> js_FillPropertyCache(JSContext *cx, JSObject *obj, jsuword kshape,
>                      uintN scopeIndex, uintN protoIndex,
>-                     JSObject *pobj, JSScopeProperty *sprop,
>-                     JSPropCacheEntry **entryp)
>+                     JSObject *pobj, JSScopeProperty *sprop)
> {
>     JSPropertyCache *cache;
>     jsbytecode *pc;
>     JSScope *scope;
>     JSOp op;
>     const JSCodeSpec *cs;
>     jsuword vword;
>     ptrdiff_t pcoff;
>@@ -125,30 +124,28 @@ js_FillPropertyCache(JSContext *cx, JSOb
>     JSPropCacheEntry *entry;
> 
>     JS_ASSERT(!cx->runtime->gcRunning);
>     cache = &JS_PROPERTY_CACHE(cx);
> 
>     /* FIXME bug 489098: consider enabling the property cache for eval. */
>     if (js_IsPropertyCacheDisabled(cx) || (cx->fp->flags & JSFRAME_EVAL)) {
>         PCMETER(cache->disfills++);
>-        *entryp = NULL;
>-        return;
>+        return JS_NO_PROP_CACHE_FILL;
>     }
> 
>     /*
>      * Check for fill from js_SetPropertyHelper where the setter removed sprop
>      * from pobj's scope (via unwatch or delete, e.g.).
>      */
>     scope = OBJ_SCOPE(pobj);
>     JS_ASSERT(scope->object == pobj);
>     if (!SCOPE_HAS_PROPERTY(scope, sprop)) {
>         PCMETER(cache->oddfills++);
>-        *entryp = NULL;
>-        return;
>+        return JS_NO_PROP_CACHE_FILL;
>     }
> 
>     /*
>      * Check for overdeep scope and prototype chain. Because resolve, getter,
>      * and setter hooks can change the prototype chain using JS_SetPrototype
>      * after js_LookupPropertyWithFlags has returned the nominal protoIndex,
>      * we have to validate protoIndex if it is non-zero. If it is zero, then
>      * we know thanks to the SCOPE_HAS_PROPERTY test above, and from the fact
>@@ -171,28 +168,26 @@ js_FillPropertyCache(JSContext *cx, JSOb
> 
>             /*
>              * We cannot cache properties coming from native objects behind
>              * non-native ones on the prototype chain. The non-natives can
>              * mutate in arbitrary way without changing any shapes.
>              */
>             if (!tmp || !OBJ_IS_NATIVE(tmp)) {
>                 PCMETER(cache->noprotos++);
>-                *entryp = NULL;
>-                return;
>+                return JS_NO_PROP_CACHE_FILL;
>             }
>             if (tmp == pobj)
>                 break;
>             ++protoIndex;
>         }
>     }
>     if (scopeIndex > PCVCAP_SCOPEMASK || protoIndex > PCVCAP_PROTOMASK) {
>         PCMETER(cache->longchains++);
>-        *entryp = NULL;
>-        return;
>+        return JS_NO_PROP_CACHE_FILL;
>     }
> 
>     /*
>      * Optimize the cached vword based on our parameters and the current pc's
>      * opcode format flags.
>      */
>     pc = cx->fp->regs->pc;
>     op = js_GetOpcode(cx, cx->fp->script, pc);
>@@ -236,18 +231,17 @@ js_FillPropertyCache(JSContext *cx, JSOb
>                             kshape);
> #endif
>                         SCOPE_MAKE_UNIQUE_SHAPE(cx, scope);
>                         if (js_IsPropertyCacheDisabled(cx)) {
>                             /*
>                              * js_GenerateShape could not recover from
>                              * rt->shapeGen's overflow.
>                              */
>-                            *entryp = NULL;
>-                            return;
>+                            return JS_NO_PROP_CACHE_FILL;
>                         }
>                         SCOPE_SET_BRANDED(scope);
>                         if (OBJ_SCOPE(obj) == scope)
>                             kshape = scope->shape;
>                     }
>                     vword = JSVAL_OBJECT_TO_PCVAL(v);
>                     break;
>                 }
>@@ -317,20 +311,20 @@ js_FillPropertyCache(JSContext *cx, JSOb
> 
>     entry = &cache->table[khash];
>     PCMETER(if (entry != *entryp) cache->modfills++);
>     PCMETER(if (!PCVAL_IS_NULL(entry->vword)) cache->recycles++);
>     entry->kpc = pc;
>     entry->kshape = kshape;
>     entry->vcap = PCVCAP_MAKE(scope->shape, scopeIndex, protoIndex);
>     entry->vword = vword;
>-    *entryp = entry;
> 
>     cache->empty = JS_FALSE;
>     PCMETER(cache->fills++);
>+    return entry;
> }
> 
> JS_REQUIRES_STACK JSAtom *
> js_FullTestPropertyCache(JSContext *cx, jsbytecode *pc,
>                          JSObject **objp, JSObject **pobjp,
>                          JSPropCacheEntry **entryp)
> {
>     JSOp op;
>@@ -986,17 +980,17 @@ js_OnUnknownMethod(JSContext *cx, jsval 
>     JSBool ok;
> 
>     JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[1]));
>     obj = JSVAL_TO_OBJECT(vp[1]);
>     JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
> 
>     MUST_FLOW_THROUGH("out");
>     id = ATOM_TO_JSID(cx->runtime->atomState.noSuchMethodAtom);
>-    ok = js_GetMethod(cx, obj, id, &tvr.u.value, NULL);
>+    ok = js_GetMethod(cx, obj, id, false, &tvr.u.value);
>     if (!ok)
>         goto out;
>     if (JSVAL_IS_PRIMITIVE(tvr.u.value)) {
>         vp[0] = tvr.u.value;
>     } else {
> #if JS_HAS_XML_SUPPORT
>         /* Extract the function name from function::name qname. */
>         if (!JSVAL_IS_PRIMITIVE(vp[0])) {
>@@ -3643,17 +3637,17 @@ js_Interpret(JSContext *cx)
>                         JS_UNLOCK_OBJ(cx, obj2);
>                         break;
>                     }
>                 } else {
>                     entry = NULL;
>                     LOAD_ATOM(0);
>                 }
>                 id = ATOM_TO_JSID(atom);
>-                obj = js_FindIdentifierBase(cx, fp->scopeChain, id, entry);
>+                obj = js_FindIdentifierBase(cx, fp->scopeChain, id);
>                 if (!obj)
>                     goto error;
>             } while (0);
>             PUSH_OPND(OBJECT_TO_JSVAL(obj));
>           END_CASE(JSOP_BINDNAME)
> 
>           BEGIN_CASE(JSOP_IMACOP)
>             JS_ASSERT(JS_UPTRDIFF(fp->imacpc, script->code) < script->length);
>@@ -4153,21 +4147,20 @@ js_Interpret(JSContext *cx)
>                             len = JSOP_INCNAME_LENGTH;
>                             DO_NEXT_OP(len);
>                         }
>                     }
>                     JS_UNLOCK_OBJ(cx, obj2);
>                     LOAD_ATOM(0);
>                 }
>             } else {
>-                entry = NULL;
>                 LOAD_ATOM(0);
>             }
>             id = ATOM_TO_JSID(atom);
>-            if (!js_FindPropertyHelper(cx, id, &obj, &obj2, &prop, &entry))
>+            if (!js_FindPropertyHelper(cx, id, true, &obj, &obj2, &prop))
>                 goto error;
>             if (!prop)
>                 goto atom_not_defined;
>             OBJ_DROP_PROPERTY(cx, obj2, prop);
>           }
> 
>           do_incop:
>           {
>@@ -4411,17 +4404,17 @@ js_Interpret(JSContext *cx)
>                     entry = NULL;
>                     if (i < 0)
>                         atom = rt->atomState.lengthAtom;
>                     else
>                         LOAD_ATOM(i);
>                 }
>                 id = ATOM_TO_JSID(atom);
>                 if (entry
>-                    ? !js_GetPropertyHelper(cx, obj, id, &rval, &entry)
>+                    ? !js_GetPropertyHelper(cx, obj, id, true, &rval)
>                     : !OBJ_GET_PROPERTY(cx, obj, id, &rval)) {
>                     goto error;
>                 }
>             } while (0);
> 
>             STORE_OPND(-1, rval);
>             JS_ASSERT(JSOP_GETPROP_LENGTH + i == js_CodeSpec[op].length);
>             len = JSOP_GETPROP_LENGTH + i;
>@@ -4507,23 +4500,23 @@ js_Interpret(JSContext *cx)
> 
>             /*
>              * Cache miss: use the immediate atom that was loaded for us under
>              * PROPERTY_CACHE_TEST.
>              */
>             id = ATOM_TO_JSID(atom);
>             PUSH(JSVAL_NULL);
>             if (!JSVAL_IS_PRIMITIVE(lval)) {
>-                if (!js_GetMethod(cx, obj, id, &rval, entry ? &entry : NULL))
>+                if (!js_GetMethod(cx, obj, id, !!entry, &rval))
>                     goto error;
>                 STORE_OPND(-1, OBJECT_TO_JSVAL(obj));
>                 STORE_OPND(-2, rval);
>             } else {
>                 JS_ASSERT(obj->map->ops->getProperty == js_GetProperty);
>-                if (!js_GetPropertyHelper(cx, obj, id, &rval, &entry))
>+                if (!js_GetPropertyHelper(cx, obj, id, true, &rval))
>                     goto error;
>                 STORE_OPND(-1, lval);
>                 STORE_OPND(-2, rval);
>             }
> 
>           end_callprop:
>             /* Wrap primitive lval in object clothing if necessary. */
>             if (JSVAL_IS_PRIMITIVE(lval)) {
>@@ -4742,28 +4735,25 @@ js_Interpret(JSContext *cx)
>                         }
>                     }
>                 }
> 
>                 if (!atom)
>                     LOAD_ATOM(0);
>                 id = ATOM_TO_JSID(atom);
>                 if (entry) {
>-                    if (!js_SetPropertyHelper(cx, obj, id, &rval, &entry))
>+                    entry = js_SetPropertyHelper(cx, obj, id, true, &rval);
>+                    if (!entry)
>                         goto error;
>-#ifdef JS_TRACER
>-                    if (entry)
>-                        TRACE_1(SetPropMiss, entry);
>-#endif
>+                    TRACE_1(SetPropMiss, entry);
>                 } else {
>                     if (!OBJ_SET_PROPERTY(cx, obj, id, &rval))
>                         goto error;
>+                    ABORT_RECORDING(cx, "Non-native set");
>                 }
>-                if (!entry)
>-                    ABORT_RECORDING(cx, "SetPropUncached");
>             } while (0);
>           END_SET_CASE_STORE_RVAL(JSOP_SETPROP, 2);
> 
>           BEGIN_CASE(JSOP_GETELEM)
>             /* Open-coded ELEMENT_OP optimized for strings and dense arrays. */
>             lval = FETCH_OPND(-2);
>             rval = FETCH_OPND(-1);
>             if (JSVAL_IS_STRING(lval) && JSVAL_IS_INT(rval)) {
>@@ -4804,17 +4794,17 @@ js_Interpret(JSContext *cx)
>             if (!OBJ_GET_PROPERTY(cx, obj, id, &rval))
>                 goto error;
>           end_getelem:
>             regs.sp--;
>             STORE_OPND(-1, rval);
>           END_CASE(JSOP_GETELEM)
> 
>           BEGIN_CASE(JSOP_CALLELEM)
>-            ELEMENT_OP(-1, js_GetMethod(cx, obj, id, &rval, NULL));
>+            ELEMENT_OP(-1, js_GetMethod(cx, obj, id, false, &rval));
> #if JS_HAS_NO_SUCH_METHOD
>             if (JS_UNLIKELY(JSVAL_IS_VOID(rval))) {
>                 regs.sp[-2] = regs.sp[-1];
>                 regs.sp[-1] = OBJECT_TO_JSVAL(obj);
>                 if (!js_OnUnknownMethod(cx, regs.sp - 2))
>                     goto error;
>             } else
> #endif
>@@ -5240,22 +5230,21 @@ js_Interpret(JSContext *cx)
>                         goto do_push_rval;
>                     }
> 
>                     JS_ASSERT(PCVAL_IS_SPROP(entry->vword));
>                     sprop = PCVAL_TO_SPROP(entry->vword);
>                     goto do_native_get;
>                 }
>             } else {
>-                entry = NULL;
>                 LOAD_ATOM(0);
>             }
> 
>             id = ATOM_TO_JSID(atom);
>-            if (!js_FindPropertyHelper(cx, id, &obj, &obj2, &prop, &entry))
>+            if (!js_FindPropertyHelper(cx, id, true, &obj, &obj2, &prop))
>                 goto error;
>             if (!prop) {
>                 /* Kludge to allow (typeof foo == "undefined") tests. */
>                 endpc = script->code + script->length;
>                 op2 = js_GetOpcode(cx, script, regs.pc + JSOP_NAME_LENGTH);
>                 if (op2 == JSOP_TYPEOF) {
>                     PUSH_OPND(JSVAL_VOID);
>                     len = JSOP_NAME_LENGTH;
>@@ -5264,17 +5253,16 @@ js_Interpret(JSContext *cx)
>                 goto atom_not_defined;
>             }
> 
>             /* Take the slow path if prop was not found in a native object. */
>             if (!OBJ_IS_NATIVE(obj) || !OBJ_IS_NATIVE(obj2)) {
>                 OBJ_DROP_PROPERTY(cx, obj2, prop);
>                 if (!OBJ_GET_PROPERTY(cx, obj, id, &rval))
>                     goto error;
>-                entry = NULL;
>             } else {
>                 sprop = (JSScopeProperty *)prop;
>           do_native_get:
>                 NATIVE_GET(cx, obj, obj2, sprop, &rval);
>                 OBJ_DROP_PROPERTY(cx, obj2, (JSProperty *) sprop);
>             }
> 
>           do_push_rval:
>@@ -6390,27 +6378,25 @@ js_Interpret(JSContext *cx)
>                 LOAD_ATOM(0);
>                 id = ATOM_TO_JSID(atom);
> 
>                 /* Set the property named by obj[id] to rval. */
>                 if (!js_CheckRedeclaration(cx, obj, id, JSPROP_INITIALIZER,
>                                            NULL, NULL)) {
>                     goto error;
>                 }
>-                if (JS_UNLIKELY(atom == cx->runtime->atomState.protoAtom)
>-                    ? !js_SetPropertyHelper(cx, obj, id, &rval, &entry)
>-                    : !js_DefineNativeProperty(cx, obj, id, rval, NULL, NULL,
>-                                               JSPROP_ENUMERATE, 0, 0, NULL,
>-                                               &entry)) {
>+
>+                entry = JS_UNLIKELY(atom == cx->runtime->atomState.protoAtom)
>+                        ? js_SetPropertyHelper(cx, obj, id, true, &rval)
>+                        : js_DefineNativeProperty(cx, obj, id, rval, NULL, NULL,
>+                                                  JSPROP_ENUMERATE, 0, 0, NULL,
>+                                                  true);
>+                if (!entry)
>                     goto error;
>-                }
>-#ifdef JS_TRACER
>-                if (entry)
>-                    TRACE_1(SetPropMiss, entry);
>-#endif
>+                TRACE_1(SetPropMiss, entry);
>             } while (0);
> 
>             /* Common tail for property cache hit and miss cases. */
>             regs.sp--;
>           END_CASE(JSOP_INITPROP);
> 
>           BEGIN_CASE(JSOP_INITELEM)
>             /* Pop the element's value into rval. */
>Index: tm/js/src/jsinterp.h
>===================================================================
>--- tm.orig/js/src/jsinterp.h    2009-04-20 22:41:02.000000000 +0200
>+++ tm/js/src/jsinterp.h    2009-04-20 22:41:27.000000000 +0200
>@@ -252,16 +252,22 @@ js_GenerateShape(JSContext *cx, JSBool g
> 
> struct JSPropCacheEntry {
>     jsbytecode          *kpc;           /* pc if vcap tag is <= 1, else atom */
>     jsuword             kshape;         /* key shape if pc, else obj for atom */
>     jsuword             vcap;           /* value capability, see above */
>     jsuword             vword;          /* value word, see PCVAL_* below */
> };
> 
>+/*
>+ * Special value for functions returning JSPropCacheEntry * to distinguish
>+ * between failure and no no-cache-fill cases.
>+ */
>+#define JS_NO_PROP_CACHE_FILL ((JSPropCacheEntry *) NULL + 1)
>+
> #if defined DEBUG_brendan || defined DEBUG_brendaneich
> #define JS_PROPERTY_CACHE_METERING 1
> #endif
> 
> typedef struct JSPropertyCache {
>     JSPropCacheEntry    table[PROPERTY_CACHE_SIZE];
>     JSBool              empty;
> #ifdef JS_PROPERTY_CACHE_METERING
>@@ -333,22 +339,24 @@ typedef struct JSPropertyCache {
> #define PCVAL_IS_SPROP(v)       (PCVAL_TAG(v) == PCVAL_SPROP)
> #define PCVAL_TO_SPROP(v)       ((JSScopeProperty *) PCVAL_CLRTAG(v))
> #define SPROP_TO_PCVAL(sprop)   PCVAL_SETTAG(sprop, PCVAL_SPROP)
> 
> /*
>  * Fill property cache entry for key cx->fp->pc, optimized value word computed
>  * from obj and sprop, and entry capability forged from 24-bit OBJ_SHAPE(obj),
>  * 4-bit scopeIndex, and 4-bit protoIndex.
>+ *
>+ * Return the filled cache entry or JS_NO_PROP_CACHE_FILL if caching was not
>+ * possible.
>  */
>-extern JS_REQUIRES_STACK void
>+extern JS_REQUIRES_STACK JSPropCacheEntry *
> js_FillPropertyCache(JSContext *cx, JSObject *obj, jsuword kshape,
>                      uintN scopeIndex, uintN protoIndex,
>-                     JSObject *pobj, JSScopeProperty *sprop,
>-                     JSPropCacheEntry **entryp);
>+                     JSObject *pobj, JSScopeProperty *sprop);
> 
> /*
>  * Property cache lookup macros. PROPERTY_CACHE_TEST is designed to inline the
>  * fast path in js_Interpret, so it makes "just-so" restrictions on parameters,
>  * e.g. pobj and obj should not be the same variable, since for JOF_PROP-mode
>  * opcodes, obj must not be changed because of a cache miss.
>  *
>  * On return from PROPERTY_CACHE_TEST, if atom is null then obj points to the
>Index: tm/js/src/jsiter.cpp
>===================================================================
>--- tm.orig/js/src/jsiter.cpp    2009-04-20 22:41:02.000000000 +0200
>+++ tm/js/src/jsiter.cpp    2009-04-20 22:41:27.000000000 +0200
>@@ -370,17 +370,17 @@ js_ValueToIterator(JSContext *cx, uintN 
>     if ((clasp->flags & JSCLASS_IS_EXTENDED) &&
>         (xclasp = (JSExtendedClass *) clasp)->iteratorObject) {
>         iterobj = xclasp->iteratorObject(cx, obj, !(flags & JSITER_FOREACH));
>         if (!iterobj)
>             goto bad;
>         *vp = OBJECT_TO_JSVAL(iterobj);
>     } else {
>         atom = cx->runtime->atomState.iteratorAtom;
>-        if (!js_GetMethod(cx, obj, ATOM_TO_JSID(atom), vp, NULL))
>+        if (!js_GetMethod(cx, obj, ATOM_TO_JSID(atom), false, vp))
>             goto bad;
>         if (JSVAL_IS_VOID(*vp)) {
>           default_iter:
>             /*
>              * Fail over to the default enumerating native iterator.
>              *
>              * Create iterobj with a NULL parent to ensure that we use the
>              * correct scope chain to lookup the iterator's constructor. Since
>Index: tm/js/src/jsobj.h
>===================================================================
>--- tm.orig/js/src/jsobj.h    2009-04-20 22:41:02.000000000 +0200
>+++ tm/js/src/jsobj.h    2009-04-20 22:41:27.000000000 +0200
>@@ -642,21 +642,25 @@ js_ChangeNativePropertyAttrs(JSContext *
>  * drop the held property, and to release the lock on obj.
>  */
> extern JSBool
> js_DefineProperty(JSContext *cx, JSObject *obj, jsid id, jsval value,
>                   JSPropertyOp getter, JSPropertyOp setter, uintN attrs,
>                   JSProperty **propp);
> 
> #ifdef __cplusplus /* FIXME: bug 442399 removes this LiveConnect requirement. */
>-extern JSBool
>+
>+/*
>+ * If cacheResult is false, return JS_NO_PROP_CACHE_FILL on success.
>+ */
>+extern JSPropCacheEntry *
> js_DefineNativeProperty(JSContext *cx, JSObject *obj, jsid id, jsval value,
>                         JSPropertyOp getter, JSPropertyOp setter, uintN attrs,
>                         uintN flags, intN shortid, JSProperty **propp,
>-                        JSPropCacheEntry** entryp = NULL);
>+                        JSBool cacheResult = JS_FALSE);
> #endif
> 
> /*
>  * Unlike js_DefineProperty, propp must be non-null. On success, and if id was
>  * found, return true with *objp non-null and locked, and with a held property
>  * stored in *propp. If successful but id was not found, return true with both
>  * *objp and *propp null. Therefore all callers who receive a non-null *propp
>  * must later call OBJ_DROP_PROPERTY(cx, *objp, *propp).
>@@ -669,32 +673,33 @@ js_LookupProperty(JSContext *cx, JSObjec
>  * Specialized subroutine that allows caller to preset JSRESOLVE_* flags and
>  * returns the index along the prototype chain in which *propp was found, or
>  * the last index if not found, or -1 on error.
>  */
> extern int
> js_LookupPropertyWithFlags(JSContext *cx, JSObject *obj, jsid id, uintN flags,
>                            JSObject **objp, JSProperty **propp);
> 
>-extern JSBool
>-js_FindPropertyHelper(JSContext *cx, jsid id, JSObject **objp,
>-                      JSObject **pobjp, JSProperty **propp,
>-                      JSPropCacheEntry **entryp);
>+/*
>+ * If cacheResult is false, return JS_NO_PROP_CACHE_FILL on success.
>+ */
>+extern JSPropCacheEntry *
>+js_FindPropertyHelper(JSContext *cx, jsid id, JSBool cacheResult,
>+                      JSObject **objp, JSObject **pobjp, JSProperty **propp);
> 
> /*
>  * Return the index along the scope chain in which id was found, or the last
>  * index if not found, or -1 on error.
>  */
> extern JS_FRIEND_API(JSBool)
> js_FindProperty(JSContext *cx, jsid id, JSObject **objp, JSObject **pobjp,
>                 JSProperty **propp);
> 
> extern JS_REQUIRES_STACK JSObject *
>-js_FindIdentifierBase(JSContext *cx, JSObject *scopeChain, jsid id,
>-                      JSPropCacheEntry *entry);
>+js_FindIdentifierBase(JSContext *cx, JSObject *scopeChain, jsid id);
> 
> extern JSObject *
> js_FindVariableScope(JSContext *cx, JSFunction **funp);
> 
> /*
>  * NB: js_NativeGet and js_NativeSet are called with the scope containing sprop
>  * (pobj's scope for Get, obj's for Set) locked, and on successful return, that
>  * scope is again locked.  But on failure, both functions return false with the
>@@ -703,36 +708,39 @@ js_FindVariableScope(JSContext *cx, JSFu
> extern JSBool
> js_NativeGet(JSContext *cx, JSObject *obj, JSObject *pobj,
>              JSScopeProperty *sprop, jsval *vp);
> 
> extern JSBool
> js_NativeSet(JSContext *cx, JSObject *obj, JSScopeProperty *sprop, jsval *vp);
> 
> extern JSBool
>-js_GetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, jsval *vp,
>-                     JSPropCacheEntry **entryp);
>+js_GetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, JSBool cacheResult,
>+                     jsval *vp);
> 
> extern JSBool
> js_GetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp);
> 
> extern JSBool
>-js_GetMethod(JSContext *cx, JSObject *obj, jsid id, jsval *vp,
>-             JSPropCacheEntry **entryp);
>+js_GetMethod(JSContext *cx, JSObject *obj, jsid id, JSBool cacheResult,
>+             jsval *vp);
> 
> /*
>  * Check whether it is OK to assign an undeclared property of the global
>  * object at the current script PC.
>  */
> extern JS_FRIEND_API(JSBool)
> js_CheckUndeclaredVarAssignment(JSContext *cx);
> 
>-extern JSBool
>-js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, jsval *vp,
>-                     JSPropCacheEntry **entryp);
>+/*
>+ * If cacheResult is false, return JS_NO_PROP_CACHE_FILL on success.
>+ */
>+extern JSPropCacheEntry *
>+js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, JSBool cacheResult,
>+                     jsval *vp);
> 
> extern JSBool
> js_SetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp);
> 
> extern JSBool
> js_GetAttributes(JSContext *cx, JSObject *obj, jsid id, JSProperty *prop,
>                  uintN *attrsp);
> 
>Index: tm/js/src/jsparse.cpp
>===================================================================
>--- tm.orig/js/src/jsparse.cpp    2009-04-20 22:41:02.000000000 +0200
>+++ tm/js/src/jsparse.cpp    2009-04-20 22:41:27.000000000 +0200
>@@ -3628,24 +3628,24 @@ CheckDestructuring(JSContext *cx, BindDa
>      *
>      * Note that we add such a property even if the block has locals due to
>      * later let declarations in it. We optimize for code simplicity here,
>      * not the fastest runtime performance with empty [] or {}.
>      */
>     if (data &&
>         data->binder == BindLet &&
>         OBJ_BLOCK_COUNT(cx, tc->blockChain) == 0) {
>-        ok = js_DefineNativeProperty(cx, tc->blockChain,
>-                                     ATOM_TO_JSID(cx->runtime->
>-                                                  atomState.emptyAtom),
>-                                     JSVAL_VOID, NULL, NULL,
>-                                     JSPROP_ENUMERATE |
>-                                     JSPROP_PERMANENT |
>-                                     JSPROP_SHARED,
>-                                     SPROP_HAS_SHORTID, 0, NULL);
>+        ok = !!js_DefineNativeProperty(cx, tc->blockChain,
>+                                       ATOM_TO_JSID(cx->runtime->
>+                                                    atomState.emptyAtom),
>+                                       JSVAL_VOID, NULL, NULL,
>+                                       JSPROP_ENUMERATE |
>+                                       JSPROP_PERMANENT |
>+                                       JSPROP_SHARED,
>+                                       SPROP_HAS_SHORTID, 0, NULL);
>         if (!ok)
>             goto out;
>     }
> 
>     ok = JS_TRUE;
> 
>   out:
>     if (fpvd.table.ops)
>Index: tm/js/src/jstracer.cpp
>===================================================================
>--- tm.orig/js/src/jstracer.cpp    2009-04-20 22:41:24.000000000 +0200
>+++ tm/js/src/jstracer.cpp    2009-04-20 22:41:27.000000000 +0200
>@@ -6016,33 +6016,41 @@ TraceRecorder::test_property_cache(JSObj
>     } else {
>         // Miss: pre-fill the cache for the interpreter, as well as for our needs.
>         // FIXME: 452357 - correctly propagate exceptions into the interpreter from
>         // js_FindPropertyHelper, js_LookupPropertyWithFlags, and elsewhere.
>         jsid id = ATOM_TO_JSID(atom);
>         JSProperty* prop;
>         if (JOF_OPMODE(*pc) == JOF_NAME) {
>             JS_ASSERT(aobj == obj);
>-            if (!js_FindPropertyHelper(cx, id, &obj, &obj2, &prop, &entry))
>+            entry = js_FindPropertyHelper(cx, id, true, &obj, &obj2, &prop);
>+
>+            /* FIXME bug 488018 - this treats OOM as no-cache-fill. */
>+            if (!entry || entry == JS_NO_PROP_CACHE_FILL)
>                 ABORT_TRACE("failed to find name");
>         } else {
>             int protoIndex = js_LookupPropertyWithFlags(cx, aobj, id,
>                                                         cx->resolveFlags,
>                                                         &obj2, &prop);
>+
>+            /* FIXME bug 488018 - this treats OOM as no-cache-fill. */
>             if (protoIndex < 0)
>                 ABORT_TRACE("failed to lookup property");
> 
>             if (prop) {
>                 if (!OBJ_IS_NATIVE(obj2)) {
>                     OBJ_DROP_PROPERTY(cx, obj2, prop);
>                     ABORT_TRACE("property found on non-native object");
>                 }
>-
>-                js_FillPropertyCache(cx, aobj, OBJ_SHAPE(aobj), 0, protoIndex, obj2,
>-                                     (JSScopeProperty*) prop, &entry);
>+                entry = js_FillPropertyCache(cx, aobj, OBJ_SHAPE(aobj), 0,
>+                                             protoIndex, obj2,
>+                                             (JSScopeProperty*) prop);
>+                JS_ASSERT(entry);
>+                if (entry == JS_NO_PROP_CACHE_FILL)
>+                    entry = NULL;
>             }
>         }
> 
>         if (!prop) {
>             // Propagate obj from js_FindPropertyHelper to record_JSOP_BINDNAME
>             // via our obj2 out-parameter. If we are recording JSOP_SETNAME and
>             // the global it's assigning does not yet exist, create it.
>             obj2 = obj;
>@@ -7551,17 +7559,17 @@ TraceRecorder::record_SetPropHit(JSPropC
>     if (*pc != JSOP_INITPROP && pc[JSOP_SETPROP_LENGTH] != JSOP_POP)
>         set(&l, v_ins);
>     return true;
> }
> 
> JS_REQUIRES_STACK bool
> TraceRecorder::record_SetPropMiss(JSPropCacheEntry* entry)
> {
>-    if (entry->kpc != cx->fp->regs->pc || !PCVAL_IS_SPROP(entry->vword))
>+    if (entry == JS_NO_PROP_CACHE_FILL || entry->kpc != cx->fp->regs->pc || !PCVAL_IS_SPROP(entry->vword))
>         ABORT_TRACE("can't trace uncacheable property set");
> 
>     JSScopeProperty* sprop = PCVAL_TO_SPROP(entry->vword);
> 
> #ifdef DEBUG
>     jsval& l = stackval(-2);
>     JSObject* obj = JSVAL_TO_OBJECT(l);
>     JSScope* scope = OBJ_SCOPE(obj);
Attachment #373729 - Flags: review?(gal) → review+
Attached patch v3Splinter Review
The new version fixes that "return false" where it should be "return null" -  thanks for spotting this! With an extra search for the returns it seems that was the only bad one. But note that such misinterpretation of false as null are harmless. AFAIK MSVC does warn about them. 

I have not moved the declaration of the "entry" local in js_DefineNativeProperty or js_SetPropertyHelper from the begining of the function closer to the code that sets the local. Due to "goto bad" in both functions the declaration must happen rather early and will separated from the code that uses in any case.

An alternative here is to put the local into own local block. That works but looks very foreign to the rest of the code. So for now I suggest to rely on compiler warnings about not-initialized variables to detect missing assignments. Now days GCC at -O2 warns about them rather well.

I also kept that 

#ifdef DEBUG
    result_that_is_used_for_assert = 
#endif
    function_call();

as is. I have used such pattern in few other places and so far I have not heard about MSVC over-warnings.
Attachment #373729 - Attachment is obsolete: true
Attachment #373742 - Flags: review?(brendan)
Attachment #373729 - Flags: review?(brendan)
To Andreas Gal:

On http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry my build is marked ibukanov@mozilla.com 489171
Comment on attachment 373742 [details] [diff] [review]
v3

Looks pretty good, but we have avoided nearly-null return pointers as hackish. Is this good for a code size win? How about perf effects (I'd expect none).

/be
Attachment #373742 - Flags: review?(brendan) → review+
We could make a static variable of that type and use it's address as cookie.
Landed for igor with the near-null cookie. Try server still spinning, getting close to 4h. I am using the idle tree to push this in.

http://hg.mozilla.org/tracemonkey/rev/cbd386ceb77f
Whiteboard: fixed-in-tracemonkey
Comment on attachment 373742 [details] [diff] [review]
v3


>@@ -317,20 +311,20 @@ js_FillPropertyCache(JSContext *cx, JSOb
> 
>     entry = &cache->table[khash];
>     PCMETER(if (entry != *entryp) cache->modfills++);

The patch needed to remove this line and therefore modfills. I should have caught this but got distracted during review.

/be
http://hg.mozilla.org/tracemonkey/rev/c0f9b256e474

Note this second patch needed for anyone cherry-picking this bug's main patch.

/be
(In reply to comment #13)
> (From update of attachment 373742 [details] [diff] [review])
> Looks pretty good, but we have avoided nearly-null return pointers as hackish.

Nearly null pointer gives something that crashes on any read or write dereference on a sane platform. This minimizes the chance of unnoticed bugs. 

> Is this good for a code size win? How about perf effects (I'd expect none).

The code size changes are tiny but the patch does shrink, for example,  js_Interpret by 21 bytes, js_FillPropertyCache by 34 and js_SetPropertyHelper by 43. This is with GCC 4.2.4 at -O3 for i386.
(In reply to comment #16)
> >     entry = &cache->table[khash];
> >     PCMETER(if (entry != *entryp) cache->modfills++);
> 

I should have compiled the version submitted for a review with PCMETER enabled. 

> The patch needed to remove this line and therefore modfills. I should have
> caught this but got distracted during review.

It is straightforward to restore modfills. AFAICS in all cases the interpreter calls js_FillPropertyCache after calling js_FullTestPropertyCache. Thus the condition entry != *entryp means that the entry for the fill is different from the one calculated in js_FullTestPropertyCache. That can reliably be recalculated.


The field counts calls to js_FillPropertyCache that fills that either follows js_FullTestPropertyCache misses or

except for the corner case when the full test test id-based hash collides with pc-based one (this is the case that makes this bug harmful).

So modfils is just a counter of filling calls to js_FillPropertyCache corrected for the above collision case. The latter on everage should happen once in 4096 calls.


> 
> /be
Were you gonna do a patch restoring modfills?

/be
(In reply to comment #20)
> Were you gonna do a patch restoring modfills?

I will restore modfills in a minimal patch for the bug 487846 - I have learned how to make such patch while fixing this bug :)
http://hg.mozilla.org/mozilla-central/rev/cbd386ceb77f
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: