Need to expose the security info pointer from the nsIXPConnectWrappedNative

VERIFIED FIXED in mozilla0.9.9

Status

()

--
enhancement
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: dbradley, Assigned: dbradley)

Tracking

Trunk
mozilla0.9.9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

17 years ago
Need a way to retrieve the opaque security info pointer from a wrapped native
via nsIXPConnectWrappedNative
(Assignee)

Comment 1

17 years ago
Created attachment 66213 [details] [diff] [review]
Adds GetSecurityInfo method to nsIXPConnectWrappedNative
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9

Comment 2

17 years ago
Does Mitch just want to get the current value of the pointer? Or does he want to
get its address so that he can store in a new value for subsequent calls? In
other places where we expose this we expose the address in order to give the
caller flexibility. Perhaps we should do that here whether he needs that added
flexibility (today) or not.
(Assignee)

Comment 3

17 years ago
When I asked initially, I got the impression he just needed the value. But as
you suggest it might be wise to open it up for the future.

jband, what would be the best XPCOM signature used for returning such a beast?
Do I just do something like native voidPtrPtr(void**) and return that?

Comment 4

17 years ago
I think that would work, but I believe it fits our pattern better to say:

[ptr] native voidPtrPtr(void*);

And certainly do it locally to that .idl file (we wouldn't want to give people
ideas by putting it in nsrootidl.idl :)
(Assignee)

Comment 5

17 years ago
Thought about this some more. I'm going to leave as is. If I expose this by
address it creates the posibility that someone could hold onto a C++ pointer
into a wrapped native without a reference. If we need to be able to change this
at some point in the future we can add a corresponding Set.

Comment 6

17 years ago
The point of this bug was to expose what Mitch needs.
Mitch: what do you need?

also, why bother with the null check?:
+    void** result = GetSecurityInfoAddr();
+    *securityInfo = result ? *result : nsnull;

GetSecurityInfoAddr can't return null. Why not:

    *securityInfo = *GetSecurityInfoAddr();

(Assignee)

Comment 7

17 years ago
I tought it could return null:

class XPCWrappedNative
{
...
    void**
    GetSecurityInfoAddr() {return HasProto() ?
                                   mMaybeProto->GetSecurityInfoAddr() : nsnull;}
The interface I added is in the wrapped native, not the wrapped native prototype.

In any case, the question of what Mitch wants is still open. I'll send him
direct mail.

Comment 8

17 years ago
Oops! Foolish me :)

Comment 9

17 years ago
Comment on attachment 66213 [details] [diff] [review]
Adds GetSecurityInfo method to nsIXPConnectWrappedNative

r/sr=jband. If Mitch says this exposes what he needs, then this is fine AFAIC.
Attachment #66213 - Flags: superreview+

Comment 10

17 years ago
Hmm, I'm still OK with this. But on second thought, you *might* consider an
error nsresult in the null case (the case where the wrapper has no prototype).
This would distinguish "security info not set" from "can not have security
info". That distinction might be of use. But, this may just not matter. Again,
it depends on what Mitch wants out of this.
I do need to be able to change the value of the pointer, so I need void**, not
void*. Look at the last argument to nsIXPCSecurityManager::CanAccess - that's
what  need.

As for the null check, if the pointer exists but isn't set, I'd expect
*securityInfo == null. If there is no security info pointer, I'd expect
securityInfo == null. Again, just like what I get through nsIXPCSecurityManager.
Does this seem reasonable?

BTW, I promise not to keep the address of that pointer around beyond the
lifetime of this particular function call - I have no reason to do that.
Comment on attachment 66213 [details] [diff] [review]
Adds GetSecurityInfo method to nsIXPConnectWrappedNative

With the change I mentioned above, r=mstoltz.
Attachment #66213 - Flags: review+

Comment 13

17 years ago
Comment on attachment 66213 [details] [diff] [review]
Adds GetSecurityInfo method to nsIXPConnectWrappedNative

This will change the diff enough to require new review.
Attachment #66213 - Attachment is obsolete: true
(Assignee)

Comment 14

17 years ago
Created attachment 68893 [details] [diff] [review]
Exposes GetSecurityInfoAddr to interface

I went the [notxpcom] route. Is this suitable for this situation? It allows me
to keep one implementation/function for this rather than creating one that
forwards to the internal one. And I think it will be easier for Mitch to use
given.

Comment 15

17 years ago
Comment on attachment 68893 [details] [diff] [review]
Exposes GetSecurityInfoAddr to interface

I don't like this. I don't see a need to add interface methods that 
don't return nsresult.

I think you should leave the existing inline method as is and add 
an interface method that calls it - much like your original patch. 
Leave off the [notxpcom] part.

Mitch uses:
  void** ppInfo = nsnull;
  wrapper->GetSecurityInfoAddr(&ppInfo);
  if(ppInfo)
    ...
Attachment #68893 - Flags: needs-work+
(Assignee)

Comment 16

17 years ago
Before I put up the new patch, wanted to get your take on this issue. I'm a
little leary of overloading the GetSecurityInfoAddr name, because of the vtable
issue we had with statics. I wasn't sure if we'd see the same vtable issue with
non-statics as well. So I was going to to go with GetSecurityInfoAddress to be safe.

Comment 17

17 years ago
Using a different name is, of course, fine and safe. I doubt there would be a
problem here anyway. In the other case the problem had to do with static and
virtual methods with the same name declared in the same interface. Here you are
coming along later and declaring an inline method in a concrete class. That
*can't effect the interface or how the vtbl layout works. Nevertheless, I don't
think it is a big deal either way.
(Assignee)

Comment 18

17 years ago
Created attachment 69598 [details] [diff] [review]
Added GetSecurityInfoAddress to nsIXPConnectWrappedNative
Attachment #68893 - Attachment is obsolete: true

Comment 19

17 years ago
Comment on attachment 69598 [details] [diff] [review]
Added GetSecurityInfoAddress to nsIXPConnectWrappedNative

>+    /* 
>+     * This returns a pointer into the instance and care should be taken
>+     * to make sure the pointer is kept past the life time of the
>+     * object it points into.
>+     */

Add the word 'not' to the above and sr=jband
Attachment #69598 - Flags: superreview+
Comment on attachment 69598 [details] [diff] [review]
Added GetSecurityInfoAddress to nsIXPConnectWrappedNative

r=jst
Attachment #69598 - Flags: review+
(Assignee)

Comment 21

17 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 22

17 years ago
Marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.