We've gone back and forth a dozen times now about whether this is a security hole. I believe it is. The ins_choose calls at the end of TR::getThis are no-ops if `this` is not the global object at record time. But we should be guarding against `this` later being the inner window (which can happen various ways -- trust us). Blake is going to write a bunch of trace-tests for this.

The fix might be to leave fp->thisp null until its first use, then call js_ComputeThis, instead of temporarily populating it with the global object and using JSFRAME_COMPUTED_THIS. IIUC, JSOP_THIS would then be able to emit trivial code (just a stack load from fp->thisp) if thisp is not null at record time (because a TT_OBJECT stack slot will never be null at run time; we already guarded on that).

#2 is pretty clever. I like it.

I <3 TT_NULL :-). /be

Kind of a red herring, but .. the do-nothing code that Jason spotted looks like this eq8 = eq ld4, inner cmov3 = cmov eq8 ? inner : ld4 cmov4 = cmov eq8 ? inner : cmov3 Adding the following rewrite rules in ExprFilter::ins3 (y == x) ? x : y => y (x == y) ? x : y => y reduces the instruction count on v8-richards from 3499 million to 3227 million. So it would be extremely cool (positively freezing) if the fix to this bug also had the same effect.

Note also, the numbers in comment #5 first require application of the patch attached to bug 504587 comment #96.

Blake and I had a long conversation about this. 1. Blake confirmed you can a reference to the inner object can escape this way. (Incidentally, the shell should have a way to check and see if you've got the inner object or not--Blake looked in the debugger.) 2. The plan in comment 2 would make us trace the loop twice, once with thisp:TT_NULL and once with thisp:TT_OBJECT. 3. Blake's counteroffer was: this_ins = insChoose(getfslot(thisp_ins, PARENT), thisp_ins, INSCONST(wrapped outer window)) which is fine but adds a scary new invariant that the only parentless object that can ever be "really exposed to script" is the parent object. 4. I think we have a better plan, about which more in a moment.

The plan is: compute fp->thisp eagerly so the slot is always correct. JSOP_THIS should just do PUSH(fp->thisp). I think we used to do that but it was slow. The JS_ComputeThis machinery is an attempt to optimize via laziness. Instead we should cache. The pobj calculated by js_FindPropertyHelper (and js_FullTestPropertyCache) should be an object that can be used as thisp without any further fuss. That means unwrapping With objects, replacing Call and Block objects with the global, and outerizing split global objects. The trick is to make the property cache cover this work. If (innerWindow -> wrappedOuterWindow) is a fixed relation, we can cache the end-to-end result of the whole lookup, both pobj-as-ammended and sprop. It will require some hacking, maybe a new kind of JSPropCacheEntry or some out-of-line data. JS_ComputeThis would go away, which would save us a function call on most fast natives. JIT would win, interpreter might win big, and it seems simpler to me overall than what we're doing.

If we can cache it, we can burn it onto trace. That will be nice and fast.

Jason, great thinking -- JSFastNative and JS_THIS came in before the return of the property cache, and we didn't think to use it instead of the COMPUTED_THIS frame flag and laziness. You don't need any new propcache entry, just use an object-valued entry keyed by JSOP_THIS etc. /be

Comment on attachment 397303 [details] [diff] [review] patch to remove silly cmov cases as per comment #5 patch is obsoleted by "patch v2" on bug 513407

I'll file a followup bug on making computing this be BLAZINGLY FAST.

That would be bug 515496.

Comment on attachment 399603 [details] [diff] [review] Immediate fix for this bug >diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp In TraceRecorder::getThis: >+ JSObject* wrappedGlobal = globalObj->thisObject(cx); >+ if (!wrappedGlobal) >+ return JSRS_ERROR; ABORT_TRACE_ERROR("helpful message"); This also wants a test. I think you were able to get hold of an inner object in the shell using this bug, so a trace-test seems appropriate. r+ with those changes.

bug 516826 blocks adding a test for this.

If this is a security bug should it also be fixed in 1.9.2, or was it introduced after that branch? Does it need to be fixed in 1.9.1 (Firefox 3.5) also? I feel more confident in assuming it doesn't affect 1.9.0

Yeah, we should fix this on all relevant branches (which, afaict, is 1.9.1 and 1.9.2).