Hmm. Is this bug about de-suckifying the xpconnect thisObject hook, basically? Note that it also wants JSStackFrames and so forth, so just JS_GetScopeChain elimination won't be enough.

The patch in bug 556277 would, I think, fix this by the simple expedient of not calling the thisObject hook anymore except for the global object.

mrbkap just realized we can fix this by making the engine just pass the callee to the thisObject hook. Taking. Now this should block bug 556277 instead of the other way round.

I got all the way through this and realized that in every case the object I was passing as the new 'scope' parameter was obj->getGlobal(). So I reverted it all and made this change instead. I assume that the 'scope' variable here only really matters up to compartment. That is, nsXPConnect::GetWrapperForObject wants a global object in the right compartment, but it really doesn't matter which global object. Is that right? There are 6 places in the JS engine where we call the thisObject hook. In two-and-a-half of them, we are calling it explicitly to wrap the global object for use as the global 'this'. In those cases I was doing, basically, obj->map->ops->thisObject(cx, obj, obj). Silly. The other cases are a little more interesting, but it always amounted to the same thing: the engine wants a this-object in the same compartment as the object whose hook it is calling.

> That is, nsXPConnect::GetWrapperForObject wants a global object in the right > compartment, but it really doesn't matter which global object. Is that right? I don't think it is, if a compartment can contain more than one inner window, but peterv would know for sure.

"Compartment" is new lingo I'm introducing, and a compartment can indeed contain multiple inner windows (with the same principals).

mrbkap dropped by #jsapi briefly yesterday and pointed out a major problem with this patch: we do have calls across compartments, such that the "this" argument provided by the caller is from a different compartment than the callee function object. XPConnect wants that function object's scope, not the "this" argument's scope. An example of when we do this is setTimeout. I still have two questions: - The question in comment 4; and - Why do we have cross-compartment calls? Isn't the argument to setTimeout (when it's a function object and not a string) always from the same compartment as the particular setTimeout function object being called? And isn't that the same compartment as the global that will be passed to the function when the event fires?

obj_getProto has a good-sized chunk of code that obj_getPrototypeOf doesn't have. I think the code is unnecessary.

I just realized I could fix the deep bail just by add a new API function, JS_GetGlobalForScopeChain. (That can be implemented in a way that doesn't leave trace.) But I think this patch fixes some bugs with the way we were calculating scope before, particularly when a fast native calls JS_ComputeThis.

Comment on attachment 444747 [details] [diff] [review] part 2, avoid thisObject (and other paranoia) in obj_getProto I believe this paranoia was leftover from the removal of __parent__ (which used to share the same obj_getSlot getter function as __proto__).

Comment on attachment 444753 [details] [diff] [review] part 3, compute scope before calling thisObject hook - v1 Looks good to me.

(In reply to comment #12) > (From update of attachment 444747 [details] [diff] [review]) > I believe this paranoia was leftover from the removal of __parent__ (which used > to share the same obj_getSlot getter function as __proto__). Right, that could have been removed in the patch for bug 552560. /be

Part 1: http://hg.mozilla.org/tracemonkey/rev/7142b29c7b1b Part 2: http://hg.mozilla.org/tracemonkey/rev/99b5258455f4 Part 3: http://hg.mozilla.org/tracemonkey/rev/8cb8888c68d3

Part 3 had to be backed out. Still investigating.

I inserted an assertion that the scope calculated the old way matched the scope calculated the new way. That didn't get very far, and it seemed to be tripping over cases where (a) it didn't really matter, and (b) the new way was arguably better. So I weakened that to an assertion that the two ways of calculating scope at least got the same principals. Below is a stack. Assertion failure: p1 == p2, at /home/jorendorff/dev/tracemonkey/js/src/jsapi.cpp:5688 Program received signal SIGABRT, Aborted. 0x0012d422 in ?? () (gdb) bt #0 0x0012d422 in ?? () #1 0x0013c230 in raise () from /lib/tls/i686/cmov/libpthread.so.0 #2 0x020fa975 in JS_Assert (s=0x21ab0e2 "p1 == p2", file=0x21a5d20 "/home/jorendorff/dev/tracemonkey/js/src/jsapi.cpp", ln=5688) at /home/jorendorff/dev/tracemonkey/js/src/jsutil.cpp:79 #3 0x01fe1fd7 in JS_AssertSamePrincipals (cx=0xb45f6800, obj1=((JSObject *) 0xb479aac0) [object Window], obj2= ((JSObject *) 0xb47229c0) [object ChromeWindow]) at /home/jorendorff/dev/tracemonkey/js/src/jsapi.cpp:5688 #4 0x0208235e in JSObject::thisObject (this=0xb47a1160, cx=0xb45f6800, scope=((JSObject *) 0xb479aac0) [object Window]) at /home/jorendorff/dev/tracemonkey/js/src/jsobj.cpp:6182 #5 0x020618a0 in CallThisObjectHook (cx=0xb45f6800, obj=((JSObject *) 0xb47a1160) [object HTMLDocument], scope= ((JSObject *) 0xb479aac0) [object Window], argv=0xb703505c) at /home/jorendorff/dev/tracemonkey/js/src/jsinterp.cpp:380 #6 0x02061a56 in js_ComputeThis (cx=0xb45f6800, argv=0xb703505c) at /home/jorendorff/dev/tracemonkey/js/src/jsinterp.cpp:429 #7 0x020622ee in js_Invoke (cx=0xb45f6800, argc=0, vp=0xb7035054, flags=0) at /home/jorendorff/dev/tracemonkey/js/src/jsinterp.cpp:669 #8 0x02062aac in js_InternalInvoke (cx=0xb45f6800, obj=((JSObject *) 0xb47a1160) [object HTMLDocument], fval=warning: can't find linker symbol for virtual table for `JSClass' value $jsobject(0xb47a11c0) [object Function], flags=0, argc=0, argv=0x0, rval=0xbfffd620) at /home/jorendorff/dev/tracemonkey/js/src/jsinterp.cpp:872 #9 0x01fe02e0 in JS_CallFunctionValue (cx=0xb45f6800, obj=((JSObject *) 0xb47a1160) [object HTMLDocument], fval=warning: can't find linker symbol for virtual table for `JSClass' value $jsobject(0xb47a11c0) [object Function], argc=0, argv=0x0, rval=0xbfffd620) at /home/jorendorff/dev/tracemonkey/js/src/jsapi.cpp:4881 #10 0x004c254b in XPCWrapper::GetOrSetNativeProperty (cx=0xb45f6800, obj=((JSObject *) 0xb47a11a0) [object XPCNativeWrapper], wrappedNative= 0xb1ad6540, id=$jsstring(0xb5e29bf0) "defaultView", vp=0xbfffdc24, aIsSet=0, isNativeWrapper=1) at /home/jorendorff/dev/tracemonkey/js/src/xpconnect/src/XPCWrapper.cpp:880 #11 0x004b1d93 in XPC_NW_GetOrSetProperty (cx=0xb45f6800, obj=((JSObject *) 0xb47a11a0) [object XPCNativeWrapper], id= $jsstring(0xb5e29bf0) "defaultView", vp=0xbfffdc24, aIsSet=0) at /home/jorendorff/dev/tracemonkey/js/src/xpconnect/src/XPCNativeWrapper.cpp:657 #12 0x004b1dc7 in XPC_NW_GetProperty (cx=0xb45f6800, obj=((JSObject *) 0xb47a11a0) [object XPCNativeWrapper], id= $jsstring(0xb5e29bf0) "defaultView", vp=0xbfffdc24) at /home/jorendorff/dev/tracemonkey/js/src/xpconnect/src/XPCNativeWrapper.cpp:663 #13 0x020843d7 in JSScopeProperty::get (this=0xb0a422b0, cx=0xb45f6800, obj=((JSObject *) 0xb47a11a0) [object XPCNativeWrapper], pobj= ((JSObject *) 0xb47a11a0) [object XPCNativeWrapper], vp=0xbfffdc24) at /home/jorendorff/dev/tracemonkey/js/src/jsscope.h:994 #14 0x0207dfbe in js_NativeGet (cx=0xb45f6800, obj=((JSObject *) 0xb47a11a0) [object XPCNativeWrapper], pobj= ((JSObject *) 0xb47a11a0) [object XPCNativeWrapper], sprop=((JSScopeProperty *) 0xb0a422b0) "defaultView", getHow=1, vp=0xbfffdc24) at /home/jorendorff/dev/tracemonkey/js/src/jsobj.cpp:4594 #15 0x0207e8d7 in js_GetPropertyHelper (cx=0xb45f6800, obj=((JSObject *) 0xb47a11a0) [object XPCNativeWrapper], id=$jsid("defaultView"), getHow= 1, vp=0xbfffdc24) at /home/jorendorff/dev/tracemonkey/js/src/jsobj.cpp:4766 #16 0x0204c21c in js_Interpret (cx=0xb45f6800) at /home/jorendorff/dev/tracemonkey/js/src/jsops.cpp:1489 #17 0x020627fb in js_Invoke (cx=0xb45f6800, argc=1, vp=0xb7035020, flags=0) at /home/jorendorff/dev/tracemonkey/js/src/jsinterp.cpp:821 #18 0x02062aac in js_InternalInvoke (cx=0xb45f6800, obj=((JSObject *) 0xb4763400) [object XULElement], fval=warning: can't find linker symbol for virtual table for `JSClass' value $jsobject(0xb47a1040) [object Function], flags=0, argc=1, argv=0xb0b5c010, rval=0xbfffdf00) at /home/jorendorff/dev/tracemonkey/js/src/jsinterp.cpp:872 #19 0x01fe02e0 in JS_CallFunctionValue (cx=0xb45f6800, obj=((JSObject *) 0xb4763400) [object XULElement], fval=warning: can't find linker symbol for virtual table for `JSClass' value $jsobject(0xb47a1040) [object Function], argc=1, argv=0xb0b5c010, rval=0xbfffdf00) at /home/jorendorff/dev/tracemonkey/js/src/jsapi.cpp:4881 #20 0x00d85c38 in nsJSContext::CallEventHandler (this=0xb4863c40, aTarget=0xb2ee5d00, aScope=0xb47229c0, aHandler=0xb47a1040, aargv=0xb0bfeac0, arv=0xbfffe09c) at /home/jorendorff/dev/tracemonkey/dom/base/nsJSEnvironment.cpp:2172 #21 0x00e0d780 in nsJSEventListener::HandleEvent (this=0xb0bfe700, aEvent=0xb0a28cc0) at /home/jorendorff/dev/tracemonkey/dom/src/events/nsJSEventListener.cpp:228 #22 0x00d35a5a in nsXBLPrototypeHandler::ExecuteHandler (this=0xb1b32260, aTarget=0xb2ee5d00, aEvent=0xb0a28cc0) at /home/jorendorff/dev/tracemonkey/content/xbl/src/nsXBLPrototypeHandler.cpp:332 #23 0x00d3124c in nsXBLEventHandler::HandleEvent (this=0xb1bd7120, aEvent=0xb0a28cc0) at /home/jorendorff/dev/tracemonkey/content/xbl/src/nsXBLEventHandler.cpp:88 #24 0x00b99303 in nsEventListenerManager::HandleEventSubType (this=0xb2fbf820, aListenerStruct=0xb0b09420, aListener=0xb1bd7120, aDOMEvent= 0xb0a28cc0, aCurrentTarget=0xb2ee5d00, aPhaseFlags=2, aPusher=0xbfffe610) at /home/jorendorff/dev/tracemonkey/content/events/src/nsEventListenerManager.cpp:1085 #25 0x00b996e7 in nsEventListenerManager::HandleEventInternal (this=0xb2fbf820, aPresContext=0x0, aEvent=0xb0b57b80, aDOMEvent=0xbfffe600, aCurrentTarget=0xb2ee5d00, aFlags=2, aEventStatus=0xbfffe604, aPusher=0xbfffe610) at /home/jorendorff/dev/tracemonkey/content/events/src/nsEventListenerManager.cpp:1181 #26 0x00bc56fa in nsEventListenerManager::HandleEvent (this=0xb2fbf820, aPresContext=0x0, aEvent=0xb0b57b80, aDOMEvent=0xbfffe600, aCurrentTarget= 0xb2ee5d00, aFlags=2, aEventStatus=0xbfffe604, aPusher=0xbfffe610) at /home/jorendorff/dev/tracemonkey/content/events/src/nsEventListenerManager.h:144 #27 0x00bc5bbc in nsEventTargetChainItem::HandleEvent (this=0xb2f36020, aVisitor=..., aFlags=2, aMayHaveNewListenerManagers=0, aPusher=0xbfffe610) at /home/jorendorff/dev/tracemonkey/content/events/src/nsEventDispatcher.cpp:212 #28 0x00bc3b32 in nsEventTargetChainItem::HandleEventTargetChain (this=0xb2f36040, aVisitor=..., aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=0, aPusher=0xbfffe610) at /home/jorendorff/dev/tracemonkey/content/events/src/nsEventDispatcher.cpp:364 #29 0x00bc46b8 in nsEventDispatcher::Dispatch (aTarget=0xb1b3a8e0, aPresContext=0x0, aEvent=0xb0b57b80, aDOMEvent=0xb0a28cc0, aEventStatus= 0xbfffe760, aCallback=0x0, aTargets=0x0) at /home/jorendorff/dev/tracemonkey/content/events/src/nsEventDispatcher.cpp:628 #30 0x00bc4a70 in nsEventDispatcher::DispatchDOMEvent (aTarget=0xb1b3a8e0, aEvent=0x0, aDOMEvent=0xb0a28cc0, aPresContext=0x0, aEventStatus= 0xbfffe760) at /home/jorendorff/dev/tracemonkey/content/events/src/nsEventDispatcher.cpp:691 #31 0x00b0e3db in nsGenericElement::DispatchDOMEvent (this=0xb1b3a8e0, aEvent=0x0, aDOMEvent=0xb0a28cc0, aPresContext=0x0, aEventStatus= 0xbfffe760) at /home/jorendorff/dev/tracemonkey/content/base/src/nsGenericElement.cpp:2968 #32 0x00a94e5f in nsContentUtils::DispatchChromeEvent (aDoc=0xb0be8400, aTarget=0xb0be8400, aEventName=..., aCanBubble=1, aCancelable=1, aDefaultAction=0x0) at /home/jorendorff/dev/tracemonkey/content/base/src/nsContentUtils.cpp:3340 #33 0x00ad83ed in nsDocument::DoNotifyPossibleTitleChange (this=0xb0be8400) at /home/jorendorff/dev/tracemonkey/content/base/src/nsDocument.cpp:5195 #34 0x00aed186 in nsRunnableMethodImpl<void (nsDocument::*)(), false>::Run (this=0xb0a129e0) at ../../../dist/include/nsThreadUtils.h:346 #35 0x017c73aa in nsThread::ProcessNextEvent (this=0xb7de9650, mayWait=0, result=0xbfffe95c) at /home/jorendorff/dev/tracemonkey/xpcom/threads/nsThread.cpp:527 #36 0x0175bcd1 in NS_ProcessNextEvent_P (thread=0xb7de9650, mayWait=0) at nsThreadUtils.cpp:250 #37 0x01698748 in mozilla::ipc::MessagePump::Run (this=0xb7de0a60, aDelegate=0xb7d237c0) at /home/jorendorff/dev/tracemonkey/ipc/glue/MessagePump.cpp:118 #38 0x0183a07d in MessageLoop::RunInternal (this=0xb7d237c0) at /home/jorendorff/dev/tracemonkey/ipc/chromium/src/base/message_loop.cc:216 #39 0x01839ff9 in MessageLoop::RunHandler (this=0xb7d237c0) at /home/jorendorff/dev/tracemonkey/ipc/chromium/src/base/message_loop.cc:199 #40 0x01839f7d in MessageLoop::Run (this=0xb7d237c0) at /home/jorendorff/dev/tracemonkey/ipc/chromium/src/base/message_loop.cc:173 #41 0x01546752 in nsBaseAppShell::Run (this=0xb46198d0) at /home/jorendorff/dev/tracemonkey/widget/src/xpwidgets/nsBaseAppShell.cpp:175 #42 0x0129e14d in nsAppStartup::Run (this=0xb46dee50) at /home/jorendorff/dev/tracemonkey/toolkit/components/startup/src/nsAppStartup.cpp:184 #43 0x004142a9 in XRE_main (argc=4, argv=0xbffff104, aAppData=0xb7d10380) at /home/jorendorff/dev/tracemonkey/toolkit/xre/nsAppRunner.cpp:3564 #44 0x08049ad7 in main (argc=4, argv=0xbffff104) at /home/jorendorff/dev/tracemonkey/browser/app/nsBrowserApp.cpp:158 (gdb) f 3 #3 0x01fe1fd7 in JS_AssertSamePrincipals (cx=0xb45f6800, obj1=((JSObject *) 0xb479aac0) [object Window], obj2= ((JSObject *) 0xb47229c0) [object ChromeWindow]) at /home/jorendorff/dev/tracemonkey/js/src/jsapi.cpp:5688 5688 JS_ASSERT(p1 == p2); (gdb) p p1 $1 = (JSPrincipals *) 0xb0ba6f14 (gdb) p p2 $2 = (JSPrincipals *) 0xb7aff3d4 (gdb) p *p1 $3 = { codebase = 0xb0bfebe8 "about:blank", getPrincipalArray = 0x6d7e44 <nsGetPrincipalArray>, globalPrivilegesEnabled = 0x6d7e4e <nsGlobalPrivilegesEnabled>, refcount = 1, destroy = 0x6d7ebf <nsDestroyJSPrincipals>, subsume = 0x6d7e58 <nsJSPrincipalsSubsume> } (gdb) p *p2 $4 = { codebase = 0xb7afa6a8 "[System Principal]", getPrincipalArray = 0x6d7e44 <nsGetPrincipalArray>, globalPrivilegesEnabled = 0x6d7e4e <nsGlobalPrivilegesEnabled>, refcount = 5320, destroy = 0x6d7ebf <nsDestroyJSPrincipals>, subsume = 0x6d7e58 <nsJSPrincipalsSubsume> }

The executing line of code is in tabbrowser.xml (so, chrome): var contentWin = event.target.defaultView; The value of event.target has already been calculated, and it is an XPCNativeWrapper object ultimately parented to a Window, whereas the current function is parented to a ChromeWindow. This breaks my understanding of XPConnect. I need a refill on enlightenment from mrbkap.

So is the question why there's no SJOW around the whole thing, effectively?

"ultimately parented to a Window" - N.B. not a wrapper of a Window or a Window object that's chrome. The principals of this Window say codebase = "about:blank". My (apparently wrong) expectation is that event, event.target, and event.target.defaultView would all be XPCNativeWrappers parented to chrome. Ideally I think when we evaluate the .defaultView part, we should unwrap event.target, call the getter in content-land, and then rewrap the result for chrome. Since the actual property get happens in content-land, I would expect the thisObject hook to want a content scope.

The thing is that there is only one (automatic) XPCNativeWrapper for a given XPCWrappedNative. So it can't be parented to chrome, since there is no canonical chrome to point it to. > Since the actual property get happens in content-land With XPCNativeWrapper, it doesn't. In fact, the whole point of XPCNativeWrapper is that property access on it is guaranteed to not call any content-defined getters or setters (including of the "just stick something in a slot" variety). XPCNativeWrapper guarantees that any property access will be directly mapped to the relevant C++-implemented nsIFoo property access (or get/set an expando on the XPCNativeWrapper itself, not the underlying content object).

And to be clear, your "Ideally ..." description is how SJOW works, but SJOW are in fact parented to chrome in this situation as I recall.

(In reply to comment #21) > The thing is that there is only one (automatic) XPCNativeWrapper for a given > XPCWrappedNative. So it can't be parented to chrome, since there is no > canonical chrome to point it to. Compartmentalization (bug 563106) will require that XPCNWs be parented to chrome. The proposed new rule is: the only edges that may point across compartment boundaries are the pointer from a wrapper to its wrappee. I just filed bug 565730 for that. > > Since the actual property get happens in content-land > > With XPCNativeWrapper, it doesn't. In fact, the whole point of > XPCNativeWrapper is that property access on it is guaranteed to not call any > content-defined getters or setters (including of the "just stick something in a > slot" variety). XPCNativeWrapper guarantees that any property access will be > directly mapped to the relevant C++-implemented nsIFoo property access (or > get/set an expando on the XPCNativeWrapper itself, not the underlying content > object). I understand. That seems like an orthogonal concern though. XPCNWs have this x-ray property, but that doesn't necessarily imply that they shouldn't also do what SJOWs do regarding wrapping and unwrapping, setting aside the stack, and generally ensuring the environment in which the wrappee executes is as "safe" (in the sense of "Safe"JSObjectWrapper) as we can make it. Back to the bug at hand -- I'm just going to (a) move the scope-calculation into the engine (2) pass the hook *exactly* the same scope as it was calculating before, but compute it without falling off trace. I'll file a followup bug to retry the more ambitious change in part 3. Patch coming soon...

Phew, that took a bit longer to get right than I thought it would.

Comment on attachment 445184 [details] [diff] [review] part 3, compute scope before calling thisObject hook - v2 We want this API for getting the current global object without falling off trace in other places, too. File a followup to add such an API?

needs before/after numbers

Sorry for the shuffling around, Blake, but this is a lot simpler and doesn't have a weirdly persistent 0.2% perf cost. Still hoping to brush all this aside later as compartments take shape.

I don't know what's going on, but this isn't quite fixed on m-c (though the patches do look like they landed). In particular, this simple testcase: for (var i = 0; i < 10; ++i) { document.createTextNode(i); } seems to deep bail quite nicely over here in today's m-c nightly (I see the deep bail calls in the shark nightly, and changing |i| to |i+""| in the createTextNode call speeds the testcase up a good bit). That said, this deep bail is from the JS_FrameIterator call that the security manager makes when XPC_WN_JSOp_ThisObject calls GetCxSubjectPrincipalAndFrame. Was that not expected to be fixed by this bug?

Thanks for checking. Let's open a new bug for this. mrbkap will file.

bug 568968.