The default bug view has changed. See this FAQ.

Cross-compartment wrapping of nsIClassInfo singleton instances fails

RESOLVED FIXED in mozilla16

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We have special handling for nsIClassInfo instances in XPCWrappedNative.cpp. But it only works if nsIClassInfo is specifically requested as the IID. This breaks when somebody tries to use a classinfo singleton cross-compartment.
(More specifically, the breakage occurs because WrapperFactory::PrepareForUnwrapping makes a new WN in the new compartment, and unconditionally passes nsISupports as the IID).
Created attachment 631770 [details] [diff] [review]
Handle classinfo singletons in cross-compartment wrapping. v1
Attachment #631770 - Flags: review?(peterv)
Created attachment 632305 [details] [diff] [review]
Handle classinfo singletons in cross-compartment wrapping. v2

Attaching the updated patch we talked about on IRC.
Attachment #631770 - Attachment is obsolete: true
Attachment #631770 - Flags: review?(peterv)
Attachment #632305 - Flags: review?(peterv)
Comment on attachment 632305 [details] [diff] [review]
Handle classinfo singletons in cross-compartment wrapping. v2

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

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +508,2 @@
>  
>      nsIClassInfo *info = helper.GetClassInfo();

I think you should reorder this so that we don't do the flags check if helper.GetClassInfo() != helper.Object(), that should be a cheap check (and we already call GetClassInfo anyway).

Something like:

    nsIClassInfo *info = helper.GetClassInfo();

    PRUint32 classInfoFlags;
    bool isClassInfo = helper.Object() == info &&;
                       NS_SUCCEEDED(info->GetFlags(&classInfoFlags)) &&
                       (classInfoFlags & nsIClassInfo::SINGLETON_CLASSINFO);
    if (!isClassInfo) {
        isClassInfo = Interface &&
                      Interface->GetIID()->Equals(NS_GET_IID(nsIClassInfo);
    }
Attachment #632305 - Flags: review?(peterv) → review+
This had a green try run back when I uploaded it. Updated and pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/43e70deac124
https://hg.mozilla.org/mozilla-central/rev/43e70deac124
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Hmm, I wish you'd actually done what I asked. We still call GetClassInfo twice for no reason and we compare IIDs sometimes for no reason :-/.
(In reply to Peter Van der Beken [:peterv] from comment #7)
> Hmm, I wish you'd actually done what I asked.

Actually, I did do what you asked. Your request in comment 4 was:

> I think you should reorder this so that we don't do the flags check
> if helper.GetClassInfo() != helper.Object()

The patch as-pushed does this. Saying "Something like: ..." does not imply that the proposed code must be used verbatim IMO.

> We still call GetClassInfo twice for no reason

GetClassInfo is an inline helper function with a cache, so I saw no reason to save it as a local per your sample code. 

> and we compare IIDs sometimes for no reason :-/.

You did not raise this concern, nor is it a change from the previous behavior.
(In reply to Bobby Holley (:bholley) from comment #8)
> The patch as-pushed does this. Saying "Something like: ..." does not imply
> that the proposed code must be used verbatim IMO.

Next time I'll enumerate the proposed changes literally. I thought it'd help to write it in code.

> GetClassInfo is an inline helper function with a cache, so I saw no reason
> to save it as a local per your sample code. 

It is already saved in a local a line below your changes, seems silly not to reuse that.

> You did not raise this concern, nor is it a change from the previous
> behavior.

Huh, I post code that's strictly faster in the case that matters and you'd rather not take it because I did not list all the improvements in text? Anyway, lesson learned, I'll be very literal in the future.
s/in the case that matters //, I posted too soon :-).
(In reply to Peter Van der Beken [:peterv] from comment #9)
> It is already saved in a local a line below your changes, seems silly not
> to reuse that.

Again, it's inline with a cache, so it quite likely compiles down to the same thing.

> Huh, I post code that's strictly faster in the case that matters and you'd
> rather not take it because I did not list all the improvements in text?

No, I chose not to take it because I'd already pushed the existing patch to try a month prior, and it wasn't fresh in my mind anymore. I wanted to land it and move on.

> Anyway, lesson learned, I'll be very literal in the future.

Peter, this patch sat in your review queue for over a month. I fixed a crash and did not make the existing code any slower. If you cared you could optimize the IID comparisons (and countless others) yourself, but you quite obviously have more important things to do. It seems highly counterproductive, both from an engineering and an interpersonal perspective, to spend time and energy criticizing the way I fixed this bug.
You need to log in before you can comment on or make changes to this bug.