for (x in function(){}) (<x><y/></x>); this.__defineGetter__("x", Function); __proto__ = x; prototype += <x><y/></x>; causes an assertion in 1.9.0.x branch: Assertion failure: pobj_ == obj2, at jsinterp.c:4156 Trace/BPT trap Seems harmless in opt.

This occurs in debug TM too, both with and without the "-j" option.

JS assertions should be critical.

This should be a regression of bug 365851. Seems to work as expected with cvs js shell checkout at 2008-02-07 15:17 PST Asserts with cvs js shell checkout at 2008-02-07 15:19 PST with: Assertion failure: !entry || entry->kpc == (PCVCAP_TAG(entry->vcap) ? (jsbytecode *) JSID_TO_ATOM(id) : cx->fp->pc), at jsobj.c:3437 and later morphed to: (by around the 2008-02-10 00:00 GMT timeframe) Assertion failure: pobj_ == obj2, at jsinterp.c:3943 http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-07+15%3A17%3A00&maxdate=2008-02-07+15%3A19%3A00&cvsroot=%2Fcvsroot Bonsai message: Return of the property cache (365851, r=shaver).

(In reply to comment #3) > This should be a regression of bug 365851. I could not reproduce the bug on Linux 32/64 builds with TM tip.

for (x in function(){}) ([]); this.__defineGetter__("x", Function); __proto__ = x; prototype += []; is a smaller testcase that excludes e4x. Definitely still occurs on TM tip with Mac OS X 10.5. $ cat 462734.js for (x in function(){}) ([]); this.__defineGetter__("x", Function); __proto__ = x; prototype += []; $ ./js-dbg-tm-intelmac 462734.js Assertion failure: pobj_ == obj2, at ../jsinterp.cpp:4334 Trace/BPT trap ===== Backtrace: Assertion failure: pobj_ == obj2, at ../jsinterp.cpp:4334 Program received signal SIGTRAP, Trace/breakpoint trap. JS_Assert (s=0x1a62e2 "pobj_ == obj2", file=0x1a75cc "../jsinterp.cpp", ln=4334) at ../jsutil.cpp:69 69 abort(); (gdb) bt #0 JS_Assert (s=0x1a62e2 "pobj_ == obj2", file=0x1a75cc "../jsinterp.cpp", ln=4334) at ../jsutil.cpp:69 #1 0x0007d5cf in js_Interpret (cx=0x30bc60) at ../jsinterp.cpp:4334 #2 0x0009643f in js_Execute (cx=0x30bc60, chain=0x2a6000, script=0x30d7d0, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1602 #3 0x0001db7a in JS_ExecuteScript (cx=0x30bc60, obj=0x2a6000, script=0x30d7d0, rval=0x0) at ../jsapi.cpp:5070 #4 0x00007f69 in Process (cx=0x30bc60, obj=0x2a6000, filename=0xbffff909 "462734.js", forceTTY=0) at ../../shell/js.cpp:412 #5 0x0000979e in ProcessArgs (cx=0x30bc60, obj=0x2a6000, argv=0xbffff81c, argc=1) at ../../shell/js.cpp:806 #6 0x0000ac2d in main (argc=1, argv=0xbffff81c, envp=0xbffff824) at ../../shell/js.cpp:4755 (gdb)

(In reply to comment #4) > (In reply to comment #3) > > This should be a regression of bug 365851. > > I could not reproduce the bug on Linux 32/64 builds with TM tip. I can reproduce the bug on Ubuntu Linux 32bit builds with TM tip. I can get a core dump if so required. === $ uname -a Linux fuzz4-ubuntu 2.6.27-11-generic #1 SMP Thu Jan 29 19:24:39 UTC 2009 i686 GNU/Linux $ cat 462734.js for (x in function(){}) ([]); this.__defineGetter__("x", Function); __proto__ = x; prototype += []; $ ./js-dbg-tm-intelmac 462734.js Assertion failure: pobj_ == obj2, at ../jsinterp.cpp:4346 Trace/breakpoint trap (core dumped)

This isn't right, but it suppresses the fun_resolve call that causes the assertion. Again __proto__ is to blame, although thankfully not the DELEGATE bit. Unrelated assertion fix to jsparse.h crept in, no worries. /be

(In reply to comment #7) > Created an attachment (id=371370) [details] > attempt to fix ASSERT_VALID_PROPERTY_CACHE_HIT > > This isn't right, but it suppresses the fun_resolve call that causes the > assertion. Again __proto__ is to blame, Settable __proto__, specifically. /be

Here is a simpler test case to expose the assert problem: Function.prototype.prototype; __proto__ = Function(); prototype = prototype; It generates the same assert in a !JS_THREADSAFE build. This assert is *not* bogus. Here prototype = prototype corresponds to the following code: bindname "prototype" name "prototype" setname "prototype" When the interpreter is about to execute this code the prototype chain for the global objects includes a function with the unresolved "prototype" property. For that function its __proto__ points to Function.prototype. That object does have the resolved "prototype" property. Now bindname is executed. The interpreter eventually calls the resolve hook for the intermediate function passing JSRESOLVE_ASSIGNING flag there. Due to this flag the hook does nothing. So the code eventually finds the value of "prototype" property from Function.prototype and caches it. Then the interpreter executes [name]. That checks the cache, finds the "prototype" for Function.prototype there, see that shapes are the same and uses that value. But this is wrong. Without the cache [name] would call the resolve hook for the intermediate function without JSRESOLVE_ASSIGNING flag. That, in turn, would initialize the "prototype" property with a fresh object. Here is an example confirming this missed call to the resolve hook in the optimized build: @watson~> cat ~/s/x2.js Function.prototype.prototype = function() { return 42; } __proto__ = Function(); prototype = prototype; assertEq(typeof prototype, "object"); @watson~> ~/build/js32.tm.opt/js ~/s/x2.js /home/igor/s/x2.js:4: TypeError: Assertion failed: got "function", expected "object"

General question: why does JSOP_BIND need to search for a property in the top of the scope chain? I.e. once the top is reached it is known that it is the top that will be the target of JSOP_SETNAME. So the code does not need to do any lookup at the top. Or do I miss something?

JSOP_BINDNAME has to resolve. If the global object's resolve hooks is the stub, then it could avoid the lookup, and assume the property exists or else it will be created by JSOP_SETNAME. The property cache test for JSOP_BINDNAME avoids a lookup and jumps to the right object, so is this bug's testcase demonstrating a JSOP_BINDNAME cache miss? /be

Duh, of course JSOP_BINDNAME misses the cache and calls resolve. The original test used +=, which uses JSOP_GETXPROP, which does re-resolve. ECMA-262 says to GetValue the Reference evaluated on the left of +=, which calls [[Get]]. Our [[Get]] is not idempotent. Eliminating +=, as the prototype = prototype test does, shows how not resolving for ASSIGNING ops breaks the E262 semantics. The bug is suppressed resolution of the LHS property when binding the name, prior to JSOP_SETNAME. This suggests the fix: remove JOF_ASSIGNING from JSOP_BINDNAME's opcode format flags. Not sure what will break, probably lots. /be

(In reply to comment #11) > JSOP_BINDNAME has to resolve. But why? JSOP_BINDNAME always follow by JSOP_SETNAME which can do all the necessary resolution. And if in between these two bytecodes anything accesses the same property, then code still calls the resolve hook. > The property cache test for JSOP_BINDNAME avoids a lookup and jumps to the > right object, so is this bug's testcase demonstrating a JSOP_BINDNAME cache > miss? It is a bug in JSOP_SETNAME that skips the resolve.

(In reply to comment #13) > (In reply to comment #11) > > JSOP_BINDNAME has to resolve. > > But why? JSOP_BINDNAME always follow by JSOP_SETNAME which can do all the > necessary resolution. And if in between these two bytecodes anything accesses > the same property, then code still calls the resolve hook. This is required by ECMA-262, so that in lhs = rhs; any effects of rhs on the scope chain do not change the meaning of lhs. E262 specifies, since Edition 1, evaluating lhs's "Reference" (its address, in memory safe scope- and prototype-chain terms) *before* evaluating rhs. Consider this global code: var x = 1; var o = {}; with (o) x = o.x = 2; print(x); After this runs, o.x should be 2, but the global variable x should also be 2. Yet if you do not bind (including resolve) x on the left of the first =, you will get two assignments to o.x, the first unqualified by implicitly based on o via the with. > > The property cache test for JSOP_BINDNAME avoids a lookup and jumps to the > > right object, so is this bug's testcase demonstrating a JSOP_BINDNAME cache > > miss? > > It is a bug in JSOP_SETNAME that skips the resolve. See above, though -- it's not that simple. /be

(In reply to comment #14) > This is required by ECMA-262, so that in > > lhs = rhs; > > any effects of rhs on the scope chain do not change the meaning of lhs. My claim is that if JSOP_BINDNAME skips the resolve stage for the top of the prototype chain (and only for top!), then it is not observable from ECMA-262 point of view. The question is if such optimization would be compatible with the resolve hooks? AFAICS it should be compatible with fun_resolve and other hooks that checks for the flags in js/src/*.cpp.

(In reply to comment #15) > My claim is that if JSOP_BINDNAME skips the resolve stage for the top of the > prototype chain (and only for top!), This comes from the fact that non-existing properties are added to the top of the chain, so if the property does not exist before the top, then we know that it will be set or created there.

(In reply to comment #14) > var x = 1; > var o = {}; > with (o) > x = o.x = 2; > print(x); > > After this runs, o.x should be 2, but the global variable x should also be 2. Yet I do not see why JSOP_BIND needs to do anything special when it reaches the top of the chain - it knows that one way or another x would be stored there, so no need to do any property lookup on the global object. Surely, it has to do the lookup for with, but then "with" is not cached by the property cache.

Agreed, so remove JOF_ASSIGNING from JSOP_BINDNAME's format -- and optimize JSOP_BINDNAME if necessary (not a big deal given the cache, I'm betting, but may be worth it). I predict regressions, in spite of your nice argument based on non-existence. Not sure where, maybe the DOM. Just a hunch! /be

If that skip-top-lookup optimization is a valid one, then it would allow to address this and the bug 487039 in the following way. JSOP_BINDNAME checks if the scope object is a top one. If so, it just stops pushing the scope to the stack. Otherwise JSOP_BINDNAME checks if the class for the scope is Call or Block. If so, it should do very optimized lookup for the property just in the scope object with the knowledge that Call or Block objects has nothing on the prototype and have no resolve hooks. If the property is found, then the result of the lookup is cached. If not, then we repeat the procedure with the parent of Call or Block. If at some stage of this loop JSOP_BINDNAME finds an object that is neither Call nor Block but that has a parent, then it means either With object or the case of chained globals like when executing DOM event listener or specially crafted script shell code to demo the cache bugs. In this case the lookup is performed normally including calling all the necessary resolve hooks. If property is found, then the JSOP_BINDNAME stops *without* caching the result deoptimizing the with blocks or names coming from DOM nodes. Since this rare case is not cached, there would be no need to skip JOF_ASSIGNING from the bind resolution stage.

That should work, but it seems like more code than we want for the same effect as not setting JOF_ASSIGNING in JSOP_BINDNAME's format, but still using the cache in JSOP_BINDNAME's interpreter guts. Cache hit for global code from BINDNAME is non-trivially faster than specialized id-keyed hash table lookup, IIRC from the 3.0 propcache dev cycle. /be

(In reply to comment #20) > Cache hit for global code from BINDNAME is non-trivially faster than specialized id-keyed hash table lookup. I am not proposing to disable the cache for Call or Block objects! The idea to check first if the current scope has no parent. If so, do nothing and just push. It must be faster than any cache lookup! Then check the cache for cached Call or Blocks. Since their structure is so simple (no prototype to worry), the cache lookup and checks for no changes could be shorter than the current ones.

(In reply to comment #21) > (In reply to comment #20) > > Cache hit for global code from BINDNAME is non-trivially faster than specialized id-keyed hash table lookup. > > I am not proposing to disable the cache for Call or Block objects! The idea to > check first if the current scope has no parent. If so, do nothing and just > push. It must be faster than any cache lookup! Oh I see -- yes, this will have great performance -- predicted not-taken branch around prefetched code to push and get on to the next opcode. Got a patch? /be

(In reply to comment #22) > this will have great performance -- predicted not-taken branch > around prefetched code to push and get on to the next opcode. Got a patch? It is little bit late now at +2 UTC timezone for any sound programming - any patching from me must wait until tomorrow ;)

This is what I am going to test.

The new version passes shell tests, lets see what mochi ones show.

In the new version of the patch I fixed harmless typo and added better comments.

Comment on attachment 371917 [details] [diff] [review] v3 The patch implements that idea of skipping any property lookup in the global object for JSOP_BINDNAME. It also makes sure that only lookup for Call and Block objects is cached to make sure that for with blocks and dom nodes no caching is done. This way any issues with caching the results of lookup with wrong set of the resolve flags is avoided. With this change it is JSOP_SETNAME, not JSOP_BINDNAME, that reports strict warnings about assigning undeclared variables. This is visible for the following script: this.a = 0; a=(delete a, 1); Without the patch the strict mode generates no warnings here, but with the patch the result is: this.a = 0; a=(delete a, 1); @watson~> ~/build/js32.tm.opt/js -s ~/s/x1.js /home/igor/s/x1.js:2: strict warning: assignment to undeclared variable a With the patch I observed no differences in SunSpider results. This is expected since declared globals are optimized using gvars and this JSOP_BINDNAME optimization cannot affect the benchmark.

Comment on attachment 371917 [details] [diff] [review] v3 > BEGIN_CASE(JSOP_BINDNAME) > do { > JSPropCacheEntry *entry; > >+ /* >+ * We can skip the property lookup for the global object. If >+ * the property does not exist anywhere on the scope chain, >+ * JSOP_SETNAME adds the property to the global. Talk about the case where the property does exist, but on a prototype of the global object: it either will be shadowed by JSOP_SETNAME, or it'll have JSPROP_READONLY and so not be shadowed. But this shows a compatibility problem: a resolve hook on a prototype could lazily define a read-only property, and with strict option this would make an error here, before the RHS is evaluated for effects. Not sure this matters. In the global object, little can be read-only without an opt-in such as ES5's strict mode. Plus, as noted, resolve hooks used JSRESOLVE_ASSIGNING to suppress resolution, to allow overriding via assignment. Still this all seems worth commenting on. Resolve can be modeled in Ecma as what should happen during [[Get]], with laziness converted to eagerness so you can't tell the difference. By not resolving in JSOP_BINDNAME where we used to, even with ASSIGNING suppression, we are making a subtly incompatible change. >+js_FindIdentifierBase(JSContext *cx, JSObject *scopeChain, jsid id, >+ JSPropCacheEntry *entry) > { >+ /* This function should not be called for a global object. */ >+ JSObject *parent = OBJ_GET_PARENT(cx, scopeChain); >+ JS_ASSERT(parent); >+ JS_ASSERT(!JS_ON_TRACE(cx)); > > /* >+ * Optimize and cache only for classes that do not have resolve hooks and >+ * were the prototype is used only to implement a shared scope, bug 462734 s/were/where/ >+ * and bug 487039. > */ >+ JSObject *obj = scopeChain; >+ for (int scopeIndex = 0; ; scopeIndex++) { >+ JSClass *clasp = OBJ_GET_CLASS(cx, obj); >+ if (clasp != &js_CallClass && clasp != &js_BlockClass) >+ break; > >+ /* >+ * Call and Block objects always have a parent and, since they are >+ * native, JSOP_BINDNAME should supply a cache entry for them. >+ */ >+ JS_ASSERT(parent); Move this down to after the assignment to parent, since you asserted on entry to the function for the first obj's parent. >+ JS_ASSERT(entry); This is loop invariant -- assert on entry. > >+ JSObject *pobj; >+ JSProperty *prop; >+ int protoIndex = >+ js_LookupPropertyWithFlags(cx, obj, id, 0, &pobj, &prop); A blank line here to breathe a little? >+ if (protoIndex < 0) > return NULL; >+ if (prop) { >+ JS_ASSERT(OBJ_IS_NATIVE(pobj)); >+ JS_ASSERT(OBJ_GET_CLASS(cx, pobj) == clasp); >+ JSScopeProperty *sprop = (JSScopeProperty *) prop; Here too? > js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, jsval *vp, >- JSPropCacheEntry **entryp) >+ JSPropCacheEntry **entryp, JSBool warnIfNewInGlobal) s/warnIfNewInGlobal/unqualified/ to show the condition leading to the action of warning if new in global. Prefer in before in/out before out params, so unqualified before vp and entryp. r=me with these nits picked. Nice case analysis, I hope we can stand the subtle incompatibility. /be

(In reply to comment #28) > But this shows a compatibility problem: > a resolve hook on a prototype could lazily define a read-only property, and > with strict option this would make an error here, before the RHS is evaluated > for effects. The read-only status of the property does not matter since JSOP_BIND does not assign the property or even check its flags. The incompatible change indeed is that now the ASSIGNING resolve hook is only called once in JSOP_SETNAME and the strict mode error is also thrown there. That mean that with the patch a = this.a = 1; no longer throws in the strict mode but this.a = 1; a = delete a; throws an error while without the patch no error is reported. Depending on once opinion, that can be considered a plus or minus.

That was a good point in the comment 28 about JS_ASSERT(parent). It revealed the loop inefficiency in js_FindIdentifierBase since the loop checks if the class of the global object is Call or Block. That is completely unnecessary given the idea behind the optimization not to do anything when binding the global. The new patch, besides addressing the nits, fixes that. It also moves the recently introduced assert in js_SetPropertyHelper that we do not add properties to Block objects. This is to ensure the assert is checked even if the code reports strict mode error.

Comment on attachment 372105 [details] [diff] [review] v4 asking for an extra review given the scope of changes

For a synthetic test case like: function f() { for (var i = 0; i != 1 <<20; ++i) { a = i; b = a; c = b; d = c; } } f(); The patch makes it run about 15% faster with jit disabled.

Comment on attachment 372105 [details] [diff] [review] v4 >+ * As a consequence of this optimization for the global object >+ * we run its JSRESOLVE_ASSIGNING resolve hooks only in >+ * JSOP_SETNAME, after the interpreter evaluates the right- >+ * hand-side of the assignment, and not here. This should be >+ * transparent to the hooks as the script, instead of >+ * name = rhs, could use global_object_reference.name = rhs, >+ * which also calls the hooks only after evaluating the rhs. Suggest this minor rewording/wrapping: * As a consequence of this optimization for the global object * we run its JSRESOLVE_ASSIGNING-tolerant resolve hooks only * in JSOP_SETNAME, after the interpreter evaluates the right- * hand-side of the assignment, and not here. * * This should be transparent to the hooks because the script, * instead of name = rhs, could have used global.name = rhs * given a global object reference, which also calls the hooks * only after evaluating the rhs. We desire such resolve hook * equivalence between the two forms. r=me with that, thanks for the followup review opportunity. /be

patch with updated comments for JSOP_BINDNAME

landed to TM - http://hg.mozilla.org/tracemonkey/rev/f35f9f0e8314

I renominate this as a 1.9.1 blocker given the issue from the comment 9.

In js_FindIdentifierBase, when we have found a property on a Block or Call, can we also assert that protoIndex is zero or one? We're relying on the fact that these have "boring" prototype chains; we could verify that that's so.

(In reply to comment #39) > In js_FindIdentifierBase, when we have found a property on a Block or Call, can > we also assert that protoIndex is zero or one? Good point: for Call or DeclEnv protoIndex should be zero and for Block protoIndex should always be one.

If js_Interpret's code for JSOP_BINDNAME trusts the cache hit it finds, then that means that anything that might supply such cache entries needs to apply the same restrictions that js_FindIdentifierBase does. So, shouldn't js_FindPropertyHelper become more like js_FindIdentifierBase?

(In reply to comment #41) > If js_Interpret's code for JSOP_BINDNAME trusts the cache hit it finds, then > that means that anything that might supply such cache entries needs to apply > the same restrictions that js_FindIdentifierBase does. So, shouldn't > js_FindPropertyHelper become more like js_FindIdentifierBase? That is what I am going to do to fix the bug 487039.

Verified fixed with testcase given in comment 9 on trunk and 1.9.1 with the following debug builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090422 Minefield/3.6a1pre ID:20090422224452 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre ID:20090422122043

ecma_3/Statements/12.10-01.js e4x/extensions/regress-462734-01.js js1_5/extensions/regress-462734-02.js js1_5/extensions/regress-462734-03.js js1_5/extensions/regress-462734-04.js

rev for previous comment was http://hg.mozilla.org/tracemonkey/rev/e558194d9530 /cvsroot/mozilla/js/tests/e4x/extensions/regress-462734-01.js,v <-- regress-462734-01.js initial revision: 1.1 /cvsroot/mozilla/js/tests/ecma_3/Statements/12.10-01.js,v <-- 12.10-01.js initial revision: 1.1 /cvsroot/mozilla/js/tests/js1_5/extensions/regress-462734-02.js,v <-- regress-462734-02.js initial revision: 1.1 /cvsroot/mozilla/js/tests/js1_5/extensions/regress-462734-03.js,v <-- regress-462734-03.js initial revision: 1.1 /cvsroot/mozilla/js/tests/js1_5/extensions/regress-462734-04.js,v <-- regress-462734-04.js initial revision: 1.1