Last Comment Bug 763343 - Cross-compartment wrapping of nsIClassInfo singleton instances fails
: Cross-compartment wrapping of nsIClassInfo singleton instances fails
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-10 13:20 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-07-19 05:20 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Handle classinfo singletons in cross-compartment wrapping. v1 (5.53 KB, patch)
2012-06-10 13:47 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Handle classinfo singletons in cross-compartment wrapping. v2 (7.76 KB, patch)
2012-06-12 10:10 PDT, Bobby Holley (:bholley) (busy with Stylo)
peterv: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-06-10 13:20:42 PDT
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.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-06-10 13:41:13 PDT
(More specifically, the breakage occurs because WrapperFactory::PrepareForUnwrapping makes a new WN in the new compartment, and unconditionally passes nsISupports as the IID).
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-06-10 13:47:27 PDT
Created attachment 631770 [details] [diff] [review]
Handle classinfo singletons in cross-compartment wrapping. v1
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-06-12 10:10:38 PDT
Created attachment 632305 [details] [diff] [review]
Handle classinfo singletons in cross-compartment wrapping. v2

Attaching the updated patch we talked about on IRC.
Comment 4 Peter Van der Beken [:peterv] 2012-07-12 07:27:47 PDT
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);
    }
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-07-13 05:35:33 PDT
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
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-07-13 20:23:57 PDT
https://hg.mozilla.org/mozilla-central/rev/43e70deac124
Comment 7 Peter Van der Beken [:peterv] 2012-07-18 08:31:45 PDT
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 :-/.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-07-18 13:10:02 PDT
(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.
Comment 9 Peter Van der Beken [:peterv] 2012-07-18 13:51:55 PDT
(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.
Comment 10 Peter Van der Beken [:peterv] 2012-07-18 14:11:18 PDT
s/in the case that matters //, I posted too soon :-).
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-07-19 05:20:58 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.