1180038 - nsIXPConnect::wrapNativeHolder should return an nsIXPConnectWrappedNative

Status: RESOLVED DUPLICATE of bug 958643

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

The only code that uses this function that I added recently in bug 958641 is some storage stuff. Looking at the code more closely, the interface this code really expects to get back is an nsIXPConnectWrappedNative, not an nsIXPConnectJSObjectHolder. In fact, if we return something that isn't a WrappedNative, we'll hit a null-deref crash later. So, I think we can just fail in NativeInterface2JSObject in the cases that return an XPCJSObjectHolder.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Inline CreateHolderIfNeeded.

It may seem silly to split this into a separate part, but my initial version of this patch had a silly bug that made Firefox crash on startup.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make wrapNativeHolder return an nsIXPConnectWrappedNative.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8629153 [details] [diff] [review]
part 1 - Inline CreateHolderIfNeeded.

Review of attachment 8629153 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCConvert.cpp
@@ +770,5 @@
>          if (!flat)
>              return false;
>      }
>      if (flat) {
>          if (allowNativeWrapper && !JS_WrapObject(cx, &flat))

Can flat be null after this?

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to :Ms2ger from comment #3)
> Can flat be null after this?

I'm not sure. Maybe wrap could fail and set flat to null?

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Inline CreateHolderIfNeeded.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43d3af32a091

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make wrapNativeHolder return an nsIXPConnectWrappedNative.

r? gabor for the XPConnect changes, mak for the storage changes. The idea is that the storage code can just hold onto nsIXPConnectWrappedNative directly instead of QIing at the end. Most of this is type changes.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8630154 [details] [diff] [review]
part 2 - Make wrapNativeHolder return an nsIXPConnectWrappedNative.

Review of attachment 8630154 [details] [diff] [review]:
-----------------------------------------------------------------

provided the tests don't complain, I think this is going to work.

Comment 8

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8630154 [details] [diff] [review]
part 2 - Make wrapNativeHolder return an nsIXPConnectWrappedNative.

Review of attachment 8630154 [details] [diff] [review]:
-----------------------------------------------------------------

Passing it to Bobby since I don't feel comfortable with this patch, but might be actually correct...

::: js/xpconnect/src/XPCConvert.cpp
@@ +775,5 @@
>              return false;
>  
>          if (dest) {
> +            // No nsIXPConnectWrappedNative to return.
> +            return false;

Same as the other one bellow, we will fail in case a security wrapper would have been used here pre-patch. Not sure if that's OK, the consumers of this functions are quite tricky...

@@ +847,5 @@
>          if (flat == original) {
>              wrapper.forget(dest);
>          } else {
> +            // No nsIXPConnectWrappedNative to return.
> +            return false;

I'm not convinced about this part. If I read this correctly if flat !=original and flat is null that means flat was an object but when we wrapped it we got null AND the wrapping did not fail (returned true), I don't even know what that means, but returning false makes sense in this weird case I guess. But if the wrapping just succeeded which is the important case, and returned some kind of wrapper, than the original code returned that in a holder and returned true, while your code returns false... Maybe nulling the dest and returning true is more inline with the original... Anyway, I don't know if it's OK to just get rid of this case, I would like Bobby to take a look at this part.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8630154 [details] [diff] [review]
part 2 - Make wrapNativeHolder return an nsIXPConnectWrappedNative.

Hmm maybe I can do something else. Cancelling review for now.

Comment 10

[Andrew McCreight [:mccr8]]

I think instead the storage code can keep alive the wrapper, then call GetWrappedNativeOfJSObject() on it in the dtor to do the stuff it needs to do on the wrap native. Then we can eliminate all of this holder stuff from NativeInterface2JSObject.

Comment 11

[Andrew McCreight [:mccr8]]

This method is unused now. I'm removing it in bug 958643.