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)
Core
XPConnect
Tracking
()
VERIFIED
FIXED
mozilla0.9.9
People
(Reporter: dbradley, Assigned: dbradley)
Details
Attachments
(1 file, 2 obsolete files)
1.64 KB,
patch
|
jst
:
review+
jband_mozilla
:
superreview+
|
Details | Diff | Splinter Review |
Need a way to retrieve the opaque security info pointer from a wrapped native via nsIXPConnectWrappedNative
Assignee | ||
Comment 1•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
Comment 2•23 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•23 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•23 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•22 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•22 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•22 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•22 years ago
|
||
Oops! Foolish me :)
Comment 9•22 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•22 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.
Comment 11•22 years ago
|
||
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 12•22 years ago
|
||
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•22 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•22 years ago
|
||
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•22 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•22 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•22 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•22 years ago
|
||
Attachment #68893 -
Attachment is obsolete: true
Comment 19•22 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 20•22 years ago
|
||
Comment on attachment 69598 [details] [diff] [review] Added GetSecurityInfoAddress to nsIXPConnectWrappedNative r=jst
Attachment #69598 -
Flags: review+
Assignee | ||
Comment 21•22 years ago
|
||
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•