04 xpcom_core!Break(char * aMsg = 0x00126000 "###!!! ASSERTION: wrong context on XPCJSContextStack!: 'cx == topJSContext', 05 xpcom_core!NS_DebugBreak_P 06 xpc3250!xpc_qsAssertContextOK 07 xpc3250!nsIDOMDocument_GetElementById 08 js3250!js_Interpret 09 js3250!js_Execute 0a js3250!JS_EvaluateUCInStackFrame 0b jsd3250!jsd_EvaluateUCScriptInStackFrame 0c jsd3250!JSD_AttemptUCScriptInStackFrame 0d jsd3250!jsdStackFrame::Eval

Comment on attachment 352634 [details] [diff] [review] support quick stubs This is an adequate spot fix, and I don't object to its going in.

...But let's talk about the underlying cause a bit. This quick stub code is not asserting that the JSContextStack is correct. The quick stub itself doesn't care if the JSContextStack is right or not; it doesn't use it. Rather, it's asserting the design assumption that quick stubs do not need to push/pop the JSContextStack in order to *keep* it correct. It seems this is wrong at least in the case of debuggers. It would also be wrong in other cases where we can call from C/C++ into JS without passing through XPConnect, right? Probably many such paths exist. This matters if we call back from the quick stub into JS via an XPCWrappedJS. I don't know about other cases where it might matter. jst, what do you think?

it's horribly wrong in terms of debuggers. ntdll!KiFastSystemCallRet ntdll!ZwWaitForSingleObject+0xc kernel32!WaitForSingleObjectEx+0xa8 kernel32!WaitForSingleObject+0x12 xpcom_core!Break+0x1c7 xpcom_core!NS_DebugBreak_P+0x2a4 xpc3250!xpc_qsAssertContextOK+0x8d xpc3250!nsIDOMDocument_GetElementById+0x1c js3250!js_Interpret+0xf0e0 js3250!js_Invoke+0x8f7 js3250!js_InternalInvoke+0x6d js3250!js_InternalGetOrSet+0x1df js3250!js_NativeGet+0x1f2 js3250!js_GetPropertyHelper+0x3e9 js3250!js_GetProperty+0x1a js3250!JS_GetPropertyDesc+0x7d js3250!JS_GetPropertyDescArray+0x1d0 jsd3250!_buildProps+0xbd jsd3250!jsd_GetCountOfProperties+0x25 jsd3250!JSD_GetCountOfProperties+0x28 jsd3250!jsdValue::GetPropertyCount+0x40 xpcom_core!NS_InvokeByIndex_P+0x27 xpc3250!XPCWrappedNative::CallMethod+0x1284 xpc3250!XPCWrappedNative::GetAttribute+0xe xpc3250!XPC_WN_GetterSetter+0x210 js3250!js_Invoke+0x87a js3250!js_InternalInvoke+0x6d js3250!js_InternalGetOrSet+0x1df js3250!js_NativeGet+0x1f2 js3250!js_GetPropertyHelper+0x3e9 js3250!js_Interpret+0xb567 js3250!js_Invoke+0x8f7 xpc3250!nsXPCWrappedJSClass::CallMethod+0xebd xpc3250!nsXPCWrappedJS::CallMethod+0x41 xpcom_core!PrepareAndDispatch+0x323 xpcom_core!SharedStub+0x16 jsd3250!jsdService::EnterNestedEventLoop+0xbf I think you guys have to fix your assumption. there are dozens of paths in jsd and the expense of the code I have to write here is really really bad since I have to do it for each and every method that would call jsd which might in turn do something.

Acknowledged. Unassigning for now. If jst thinks the assumption can't easily be repaired, then the solution is for quickstubs to push/pop the JSContextStack, and that's a simple change which I could take. May regress performance.

How (if at all) does this impact firebug?

Rob: please see this bug and answer bz's question in comment 5?

I have no idea. CC'ing debugging guru Barton to assess.

I don't know what quickstubs are. It looks like the patch saves and restores the context and updates this ContextStack side table. I can say Firebug does not use ContextStack but certainly uses frame.eval(), if that helps in anyway.

(In reply to comment #2) > It seems this is wrong at least in the case of debuggers. It would also be > wrong in other cases where we can call from C/C++ into JS without passing > through XPConnect, right? Probably many such paths exist. I'm not sure this is true. The only other ways that I know are XBL (which we should fix, probably before XBL2) and wrappers (which explicitly push). The jsd authors simply didn't know about this when they wrote this. For what it's worth, I don't think there's an exploit or anything else here... The top stack frame on the context on the top of the stack will be the same stack frame as the top of the cx that's passed in. Since it's a scripted frame, we don't have to worry about the principal represented by the context, and since it's the same frame, anybody looking at the old context will receive the right answers. So, if we want to take the assertion silencer, we should. But I don't think anything here warrants blocking.

Not blocking per last comment.

so, i spoke about this a bit w/ blake, and at this point it seems that the needs of quickstubs are such that it *doesn't* want the jsd context to be on the xpconnect stack and in fact wants to use the xpconnect stack and not the js stack when it does things. however, neither of us are absolutely certain :) wrt not blocking, it might actually be a problem depending on what quickstubs is doing and if they're vaguely security related because it could assuming it uses the js stack instead of the xpconnect stack result in quickstubs perhaps allowing a content caller to access something chromelike because it found the jsd debugger context in the js stack even though it isn't related. but it's really late, so i'm merely dumping thoughts instead of trying to figure out what the quickstubs code does :)