Closed Bug 121526 Opened 23 years ago Closed 22 years ago

Need to expose the security info pointer from the nsIXPConnectWrappedNative

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: dbradley, Assigned: dbradley)

Details

Attachments

(1 file, 2 obsolete files)

Need a way to retrieve the opaque security info pointer from a wrapped native
via nsIXPConnectWrappedNative
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
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.
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?
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 :)
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.
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();

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.
Oops! Foolish me :)
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+
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 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
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 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+
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.
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.
Attachment #68893 - Attachment is obsolete: true
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+
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: