384632 - Should be possible to put a regular JS object in an nsIVariant

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: testcase

In order to test bug 352882, I had to figure out how to create a double-wrapped object. At the time, it seemed that the easiest way to do so was to create a regular JS object and stick it as the user data on a document. I was surprised to find that this didn't work, in order to make it work, I had to make it implement nsISecurityCheckedComponent. With the patch for bug 352882, this is no longer possible, but it seems like it should be possible to do this.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: testcase

The pass condition on the other testcase was reversed (oops!).

Comment 2

[Boris Zbarsky [:bzbarsky]]

So this just fails because XPCVariant doesn't know what to do with it?

What variant type would it be exposed as?  Esp. in languages other than JS (Object) and C++ (JSObject)?

Comment 3

[Boris Zbarsky [:bzbarsky]]

I guess it could become nsISupports..

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Well, the wrapping succeeds (you get an nsIVariant wrapping your JSObject). As far as I could tell, I ended up with an XPCWrappedJS in the variant. The problem occurs when we try to return the variant to JS (in this case, this happened during argument conversion after calling setUserData): we try to wrap the variant in an XPCWrappedNative, which calls InitTearOff for nsISupports, which ends up asking the security manager if it can create a wrapper, which fails since the object we're wrapping is not a DOM object.

Does that make sense?

Comment 5

[Boris Zbarsky [:bzbarsky]]

So in this case "we" is not chrome code?  Otherwise, I'd think nsScriptSecurityManager::CheckXPCPermissions would allow wrapper creation...

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Right, "we" is the content page (we're returning fromSetUserData and dealing with out params).

Comment 7

[Boris Zbarsky [:bzbarsky]]

So one solution would be to have nsXPCVariant implement nsISecurityCheckedComponent, right?

That said, it sounds like a bug in setUserData to me if it returns a double-wrapped JSObject instead of unwrapping it.  Or perhaps this this a bug in xpcconvert?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix

So, this seems to fix the bug by explicitly not double-wrapping the JS object when we get it out of the variant. I basically copied the code that currently resides in XPCConvert::NativeInterface2JSObject.

jband, do you know if the current behavior was intentional? While I see the argument for normally double-wrapping things (since they don't behave like normal XPCOM objects), this seems like a special case, since a variant holds anything.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 270212 [details] [diff] [review]
Possible fix

It'd be good to hear from jband :-/. This wouldn't cause us to bypass any security checks, right?

>+                // First QI the wrapper to the right interface.

Maybe something like: "canonicalize to get the root wrapper"? Unless I misunderstand what this is doing.

>+                nsCOMPtr<nsISupports> wrapper;
>+                nsresult rv = src->QueryInterface(iid, getter_AddRefs(wrapper));
>+                NS_ENSURE_SUCCESS(rv, JS_FALSE);

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to comment #9)
> Maybe something like: "canonicalize to get the root wrapper"? Unless I
> misunderstand what this is doing.

Nevermind this, I see I misunderstood :-).

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.