1007207 - Classinfo check in XPCWrappedNative::GetNewOrUsed should not be based on the iid at all

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

We have code in XPCWrappedNative::GetNewOrUsed that checks for nsIClassInfo, and avoids using the Scriptable Helper when it doesn't apply.

This used to be based on the IID passed to GetNewOrUsed, which isn't exactly the most reliable field. In bug 763343 (not the highest moment in our relationship), Peter and I modified this to check to _also_ check for ClassInfo singletons, regardless of IID.

Recently, while poking around at some things, I determined that we didn't go far enough. We shouldn't base it on the IID at all, because, for self-implemented classinfo we still need to use the SH (to get the right PreCreate hook) even if the first interface we QI to happens to be nsIClassInfo.

Attaching a patch, which has a green try run here:
https://tbpl.mozilla.org/?tree=Try&rev=1605151b09b6

Comment 1

[Bobby Holley (:bholley)]

Attachment: Don't treat self-implemented classinfo instances differently during XPCWN creation. v1

Comment 2

[Nicholas Nethercote [inactive]]

peterv: almost-two-week review ping. This is blocking bug 997906, which should give us a nice memory usage win.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> peterv: almost-two-week review ping. This is blocking bug 997906, which
> should give us a nice memory usage win.

This isn't really urgent. Bug 997906 is stalled for other reasons. But it'd be nice to get in the tree when peter has time.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 8418809 [details] [diff] [review]
Don't treat self-implemented classinfo instances differently during XPCWN creation. v1

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

This doesn't seem completely right either. It seems to me that we really want to check if CI == the object it's the CI for, and use the SH only in that case? But there's no way to go back to the original object from the CI instance, so I don't have a better idea :-(. Are there actual cases that this affects in the tree?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #4)
> This doesn't seem completely right either. It seems to me that we really
> want to check if CI == the object it's the CI for, and use the SH only in
> that case? But there's no way to go back to the original object from the CI
> instance, so I don't have a better idea :-(.

Yeah. :-(

> Are there actual cases that this affects in the tree?

It bit me in a patch that I haven't landed yet (and may not land). But this seemed marginally more correct, so I wanted to get it in the tree.

Comment 6

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f627f02f933

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5f627f02f933