nsIXPConnect::wrapNativeHolder should return an nsIXPConnectWrappedNative

RESOLVED DUPLICATE of bug 958643

Status

()

Core
XPConnect
RESOLVED DUPLICATE of bug 958643
3 years ago
9 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox42 affected)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1180040
(Assignee)

Comment 1

3 years ago
Created attachment 8629153 [details] [diff] [review]
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.
(Assignee)

Comment 2

3 years ago
Created attachment 8629154 [details] [diff] [review]
part 2 - Make wrapNativeHolder return an nsIXPConnectWrappedNative.
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?
(Assignee)

Comment 4

3 years ago
(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?
(Assignee)

Comment 5

3 years ago
Created attachment 8630150 [details] [diff] [review]
part 1 - Inline CreateHolderIfNeeded.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43d3af32a091
Attachment #8630150 - Flags: review?(gkrizsanits)
(Assignee)

Comment 6

3 years ago
Created attachment 8630154 [details] [diff] [review]
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.
Attachment #8629153 - Attachment is obsolete: true
Attachment #8629154 - Attachment is obsolete: true
Attachment #8630154 - Flags: review?(mak77)
Attachment #8630154 - Flags: review?(gkrizsanits)
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.
Attachment #8630154 - Flags: review?(mak77) → review+
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.
Attachment #8630154 - Flags: review?(gkrizsanits) → review?(bobbyholley)
Attachment #8630150 - Flags: review?(gkrizsanits) → review+
(Assignee)

Comment 9

3 years ago
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.
Attachment #8630154 - Flags: review?(bobbyholley)
(Assignee)

Comment 10

3 years ago
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.
(Assignee)

Comment 11

9 months ago
This method is unused now. I'm removing it in bug 958643.
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 958643
You need to log in before you can comment on or make changes to this bug.