Closed Bug 469226 Opened 16 years ago Closed 14 years ago

every method in jsd-xpc would needs to push a frame before calling jsd_ to make quickstubs happy

Categories

(Core :: XPConnect, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: timeless)

References

Details

Attachments

(1 file)

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
Attachment #352634 - Flags: review?
Attachment #352634 - Flags: review? → review?(jorendorff)
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.
Attachment #352634 - Flags: review?(jorendorff) → review+
...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.
Assignee: timeless → jorendorff
Component: JavaScript Debugger → XPConnect
Product: Other Applications → Core
QA Contact: venkman → xpconnect
Summary: jsdStackFrame::Eval needs to push a frame to make quickstubs happy → every method in jsd-xpc would needs to push a frame before calling jsd_ to make quickstubs happy
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.
Assignee: jorendorff → nobody
How (if at all) does this impact firebug?
Flags: blocking1.9.1?
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.
Flags: blocking1.9.1? → blocking1.9.1-
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 :)
Assignee: nobody → mrbkap
http://hg.mozilla.org/mozilla-central/rev/6cd287a696e2
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 560139
Assignee: mrbkap → timeless
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: