Closed Bug 384632 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: XPConnect, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file testcase (obsolete) —
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.
Attached file testcase
The pass condition on the other testcase was reversed (oops!).
Attachment #268537 - Attachment is obsolete: true
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)?
I guess it could become nsISupports..
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?
So in this case "we" is not chrome code?  Otherwise, I'd think nsScriptSecurityManager::CheckXPCPermissions would allow wrapper creation...
Right, "we" is the content page (we're returning fromSetUserData and dealing with out params).
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?
Attached patch Possible fixSplinter Review
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.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #270212 - Flags: review?(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);
Attachment #270212 - Flags: review?(peterv) → review+
(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 :-).
Attachment #270212 - Flags: superreview?(jst)
Attachment #270212 - Flags: review?(jband_mozilla)
Attachment #270212 - Flags: superreview?(jst) → superreview+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 415037
Attachment #270212 - Flags: review?(jband_mozilla)
You need to log in before you can comment on or make changes to this bug.