Two objects that share an empty scope can have different JSClasses. If one has stubs for the resolve and addProperty hooks, and the other has custom hooks, the property cache can cause the custom hooks to be skipped. It looks like there are two bugs around addProperty: 1. The property cache is filled even when the object in question has a nonstub addProperty hook. 2. Objects with addProperty hooks are allowed to share scopes with other objects that use the stub. Hard to test. :-\

Here's the test case. See bug 505798 for the test harness. (The patch there already contains this test case.) static int g_counter; static JSBool CounterAdd(JSContext *cx, JSObject *obj, jsval idval, jsval *vp) { g_counter++; return JS_TRUE; } static JSClass CounterClass = { "Counter", /* name */ 0, /* flags */ CounterAdd, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, JS_FinalizeStub, JSCLASS_NO_OPTIONAL_MEMBERS }; BEGIN_TEST(addProperty) { g_counter = 0; eval("var x = {};"); if (!JS_DefineObject(cx, global, "y", &CounterClass, NULL, JSPROP_ENUMERATE)) throwError(); eval("var arr = [x, y];\n" "for (var i = 0; i < arr.length; i++)\n" " arr[i].p = 1;\n"); assertEq(g_counter, 1); } END_TEST(addProperty)

Without the patch: Own Scope : 45781 Not Own Scope : 110287 Not Native Scope: 69511 Total : 225579 With the patch: Own Scope : 45782 Not Own Scope: 110288 Not Native Scope: 69511 Total : 225581

Comment on attachment 396574 [details] [diff] [review] patch Beautiful. Please also commit this, since your patch makes the test pass: diff --git a/js/src/jsapi-tests/testPropCache.cpp b/js/src/jsapi-tests/testPropCache.cpp --- a/js/src/jsapi-tests/testPropCache.cpp +++ b/js/src/jsapi-tests/testPropCache.cpp @@ -12,21 +12,20 @@ CounterAdd(JSContext *cx, JSObject *obj, static JSClass CounterClass = { "Counter", /* name */ 0, /* flags */ CounterAdd, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, JS_FinalizeStub, JSCLASS_NO_OPTIONAL_MEMBERS }; -BEGIN_TEST(testPropCache_bug505798) +BEGIN_TEST(testPropCache_bug505523) { g_counter = 0; EXEC("var x = {};"); CHECK(JS_DefineObject(cx, global, "y", &CounterClass, NULL, JSPROP_ENUMERATE)); EXEC("var arr = [x, y];\n" "for (var i = 0; i < arr.length; i++)\n" " arr[i].p = 1;\n"); - knownFail = true; CHECK(g_counter == 1); return true; } -END_TEST(testPropCache_bug505798) +END_TEST(testPropCache_bug505523)

Note that this fixes only the *second* bug described in comment 0. The first bug is still there but it's something different, so I'll file a separate bug.

XPConnect relies on the conditions in js_ObjectIsSimilarToProto to make all wrappers with the same proto share a scope. Do we know what the effect is of this change in XPConnect, seems like we'd create a whole lot more scopes? The other way that XPConnect sort of relies on those conditions is that we change the JSClass when morphing from a slimwrapper to a XCWrappedNative, with the existing conditions the JSClass change didn't mean a new scope was needed. If I understand this change correctly we need to either find a way to stop changing the JSClass or we need a way to signal the change to the JS engine so it can give the object a new scope?

Did we want to merge this before we figured out how to avoid the regression on dealing with slimwrapper promotion?

I think we want to back this out at least from trunk given the bugs linked to in the dependency list.

http://hg.mozilla.org/mozilla-central/rev/bc337dee7e68 backs out this on mozilla-central per comment 9 and comment 10.

What's next here?

This was backed out from TM too: http://hg.mozilla.org/tracemonkey/rev/bc337dee7e68

Stealing.

New try, keeping a JSClass* in each emptyScope.

Review ping.

Comment on attachment 409197 [details] [diff] [review] v1 >@@ -290,18 +290,17 @@ js_FillPropertyCache(JSContext *cx, JSOb > * matching empty scope. In unusual cases involving > * __proto__ assignment we may not find one. > */ > JSObject *proto = STOBJ_GET_PROTO(obj); > if (!proto || !OBJ_IS_NATIVE(proto)) > return JS_NO_PROP_CACHE_FILL; > JSScope *protoscope = OBJ_SCOPE(proto); > if (!protoscope->emptyScope || >- !js_ObjectIsSimilarToProto(cx, obj, obj->map->ops, OBJ_GET_CLASS(cx, obj), >- proto)) { >+ protoscope->emptyScope->clasp != obj->getClass()) { > return JS_NO_PROP_CACHE_FILL; >+ /* >+ * Make sure proto's scope's emptyScope is available to be shared by >+ * objects of this class. JSScope::emptyScope is a one-slot cache. If we >+ * omit this, some other class could snap it up. (The risk is particularly >+ * great for Object.prototype.) >+ * >+ * All callers of JSObject::initSharingEmptyScope depend on this. >+ */ >+ JSScope *scope; >+ scope = OBJ_SCOPE(proto)->getEmptyScope(cx, clasp); Initialize scope and you get a warning, but perhaps it's time to change the control flow to use if-then so prefetching gets the hot path. Then we lose ugly gotos and win initialization. > public: > explicit JSScope(const JSObjectOps *ops, JSObject *obj = NULL) > : JSObjectMap(ops, 0), object(obj) {} [snip] >+struct JSEmptyScope : public JSScope { Nit: we've started putting the { on its own line for subclass/substruct decls. >+ JSClass * const clasp; >+ >+ explicit JSEmptyScope(const JSObjectOps *ops, JSClass *clasp) >+ : JSScope(ops), clasp(clasp) {} Nit: half-indent the last line. >+inline JSEmptyScope * >+JSScope::getEmptyScope(JSContext *cx, JSClass *clasp) >+{ >+ if (emptyScope) { >+ JS_ASSERT(clasp == emptyScope->clasp); >+ emptyScope->hold(); >+ return emptyScope; >+ } >+ return createEmptyScope(cx, clasp); >+} The patch removes a call to getEmptyScopeShape from JSScope::clear, instead generating a fresh shape. Is that optimal for window objects (which are one of the few objects that are JS_ClearScope'ed)? The immediate prototype object of a window object may not be a class proto. This seems like a good approach but it shifts some costs around. Any noticeable perf effects? /be

Comment on attachment 409197 [details] [diff] [review] v1 Brendan's comments are good and I'll address them. No time right now. Shifting review to Graydon.

Graydon pointed out that in js_String_tn -> js_NewObjectWithClassProto -> initSharingEmptyScope, it is not obvious that the proto there is in fact the result of js_InitClass. It turns out that assumption *can* be wrong, if the global object's class does not have JSCLASS_GLOBAL_FLAGS; or if the user has been foolishly playing with JS_SetReservedSlot and the global object. What should I do about that?

Comment on attachment 409197 [details] [diff] [review] v1 I bothered jorendorff on IRC and he explained this patch to me laboriously over an hour or so, and it seems to make sense. I can't vouch for it catching all cases or even necessarily maintaining important SM invariants (few of which I have any clue about), but it at least makes as much sense to me as anything else in this code. Prototype scopes can only have their empty scope reused if the reuser is of the same class as that empty scope. What could go wrong?

(In reply to comment #19) Answer: Check at record time that the proto object we're baking into the trace already has the right OBJ_SCOPE(proto)->emptyScope->clasp. Will push this tomorrow.

Comment on attachment 413125 [details] [diff] [review] v2 >+ closure->initSharingEmptyScope(&js_FunctionClass, proto, parent, >+ reinterpret_cast<jsval>(fun)); Shouldn't that last arg by OBJECT_TO_JSVAL(FUN_OBJECT(fun)) -- or OBJECT_TO_JSVAL(fun) if the new inline C++ helper means the upcast can be implicit? >+ /* >+ * Make sure proto's scope's emptyScope is available to be shared by >+ * objects of this class. JSScope::emptyScope is a one-slot cache. If we >+ * omit this, some other class could snap it up. (The risk is particularly >+ * great for Object.prototype.) >+ * >+ * All callers of JSObject::initSharingEmptyScope depend on this. >+ */ >+ if (JSScope *scope = OBJ_SCOPE(proto)->getEmptyScope(cx, clasp)) >+ scope->drop(cx, NULL); >+ else >+ goto bad; The else-goto is unusual style, it oddly splits the normal control flow between the consequent and the if/else's continuation. Lose the declaration in condition and invert? Or make an inline helper to hide the drop, even better. r=me with these addressed -- happy to look at a final patch or just read the hgweb (bugzilla interdiff failed between v1 and v2 but if it works that's easier than hgweb). /be

Comment on attachment 413125 [details] [diff] [review] v2 >+ // Since the compiled code may end up passing the proto to >+ // JSObject::initSharingEmptyScope, we can only proceed if proto has a "proceed only" >+ // matching emptyScope. >+ if (key != JSProto_Array) { Mention why JSProto_Array is excluded from the logic being commented upon here? >+ if (!OBJ_IS_NATIVE(proto)) >+ RETURN_STOP("non-native class prototype"); >+ JSEmptyScope *emptyScope = OBJ_SCOPE(proto)->emptyScope; >+ if (!emptyScope || JSCLASS_CACHED_PROTO_KEY(emptyScope->clasp) != key) >+ RETURN_STOP("class prototype is not the standard one"); >+ } >+ > proto_ins = INS_CONSTOBJ(proto); > return RECORD_CONTINUE; > } Thanks for the approx. interdiff, it helped. /be

(In reply to comment #23) > (From update of attachment 413125 [details] [diff] [review]) > >+ closure->initSharingEmptyScope(&js_FunctionClass, proto, parent, > >+ reinterpret_cast<jsval>(fun)); > > Shouldn't that last arg by OBJECT_TO_JSVAL(FUN_OBJECT(fun)) -- or > OBJECT_TO_JSVAL(fun) if the new inline C++ helper means the upcast can be > implicit? It's the value to store in the private slot of `closure`. It's not really a jsval; we should change all those to void * someday.

Comment on attachment 413155 [details] [diff] [review] v3 Looks good, thanks -- brute force narrow scope via explicit block works for me. /be

I had to back this out of the branch because it caused plugin crashes on the mac tinderboxen only. Will attach branch patch and investigate.

JS_Assert + 67 (jsutil.cpp:69) OBJ_SCOPE + 95 (jsscope.h:347) js_InitClass + 1169 (jsobj.cpp:2973) JS_InitClass + 157 (jsapi.cpp:2734) jsj_init_JavaObject + 97 (jsj_JavaObject.c:1055) JSJ_InitJSContext + 24 (jsj.c:538) nsJVMManager::InitLiveConnectClasses(JSContext*, JSObject*) + 32 (nsJVMManager.cpp:478)

Waldo theorizes: <Waldo> so, I wonder if that patch just happened to break all classes with non-native protos <Waldo> and we never noticed because we don't have any <Waldo> sayrer: what happens if you prefix the new block in js_InitClass with |if (OBJ_IS_NATIVE(proto))| ? If I add that check, as in this patch, plugins do work.

Nice -- jsapi-test?