862077 - XPCWrappedNative::GetNewOrUsed shouldn't trust wrapper caches so much

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

If an inner window gets into GetNewOrUsed and the cache in the helper is non-null (e.g. it came through new-binding parent-wrapping as an EventTarget), we will do bad things, since cache and !cache->GetWrapperPreserveColor() will be true, but we will in fact have an XPCWrappedNative already.

Since most things whose wrapping performance we care about aren't coming through here nowadays, and since fewer and fewer will in the future, we should just treat this case the same as the !cache case and look for an already-existing wrapper.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Even if we have a wrapper cache, don't assume that no cached wrapper means no wrapped native.: it's not true for inner windows.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Diff -w for review ease

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #0)
> If an inner window gets into GetNewOrUsed and the cache in the helper is
> non-null (e.g. it came through new-binding parent-wrapping as an
> EventTarget)

Can you explain this part a little bit more? IIRC only outer windows use the wrapper cache, right? So what currently causes us to do the right thing when we pass an inner to GetNewOrUsed? Or is the point that we don't, but somehow we've never hit that before?

> // Some things are nsWrapperCache subclasses but never use the cach

Per the above - does this apply to anything but inner windows? If not, maybe be more specific in the comment?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> So what currently causes us to do the right thing when we pass an inner to GetNewOrUsed?

The fact that xpcObjectHelper has a null wrapper cache pointer, because inner windows don't QI to nsWrapperCache.

But the webidl binding code basically uses cast-by-inheritance, rather than QI, to get the nsWrapperCache, and EventTarget inherits from nsWrapperCache.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> But the webidl binding code basically uses cast-by-inheritance, rather than
> QI, to get the nsWrapperCache, and EventTarget inherits from nsWrapperCache.

Yikes. Maybe we should make inner windows wrapper cache as well? IIRC mrbkap said there was never an actual reason not to, he was just trying to avoid breaking things when he was implementing some of this stuff.

It seems like that would be much better than this fix, which seems kinda whack-a-mole.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Maybe we should make inner windows wrapper cache as well?

Yes, but that might be a more involved project...  I'd like to get us to a correct behavior in the safest and quickest way possible, before people add code that triggers this as part of WebIDL conversions...

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> > Maybe we should make inner windows wrapper cache as well?
> 
> Yes, but that might be a more involved project...  I'd like to get us to a
> correct behavior in the safest and quickest way possible, before people add
> code that triggers this as part of WebIDL conversions...

Yeah that sounds fine. File a bug though?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Filed bug 862145.

Comment 9

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb589744522d

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eb589744522d