629822 - GatherAndConvertResults passes wrong scope to NativeData2JS

Status: RESOLVED DUPLICATE of bug 627954

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Currently GatherAndConvertResults passes mCurrentJSObject as the scope to NativeData2JS which may be in a different compartment than the caller, in whose compartment NativeData2JS is creating the return value.

It seems like the fix is to hoist/reuse (and rename) ComputeWrapperInfo.

Steve Fink is able to reproduce a crash with Firebug.

Comment 1

[Steve Fink [:sfink] [:s:]]

STR:

1. Install Firebug1.7 (version probably doesn't matter)
2. Open up any page
3. Start Firebug by clicking on the bug icon in the bottom right corner
4. Click on the 'Console' tab
5. At the command line (bottom; prefixed with '>>>'), enter 'Firebug' and press enter
6. Repeat step 5.

It will first say 'Firebug is undefined' (step 5), then it will crash with a compartment mismatch (step 6).

Comment 2

[Luke Wagner [:luke]]

I'm going to base this on top of the patches in bug 627954.

Comment 3

[Luke Wagner [:luke]]

Hah, wow, the sanity asserts I added in the patches in bug 627954 hit just starting a browser with Firebug enabled!

#2 PR_Assert("!scope || scope->compartment() == GetJSContext()->compartment")
#3 XPCConvert::NativeData2JS
#4 XPCConvert::NativeData2JS
#5 CallMethodHelper::GatherAndConvertResults
#6 CallMethodHelper::Call
#7 XPCWrappedNative::CallMethod
#8 XPCWrappedNative::GetAttribute
#9 XPC_WN_GetterSetter
...

Comment 4

[Luke Wagner [:luke]]

Oh, nevermind, this is not related to Firebug, since the patches used to be green, the assert must have come in with a recent pull --rebase.  However, if these asserts are fixed, then this bug should be resolved (its the same issue: mCurrentJSObject is in a different compartment than cx and mCurrentJSObject is being passed as the 'scope' to NativeData*.

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Hack to work around return value compartment (NOT A FIX)

If anyone else is hitting this, here's a temporary workaround to allow things to proceed.

Just in case, NOTE: THIS IS NOT A FIX. Asa or whoever, do not add [has patch] to the whiteboard solely for this patch.

Comment 6

[Luke Wagner [:luke]]

The crash with STR in comment 1 are fixed by the WIP patch in bug 627954.