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

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla23
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Created attachment 737690 [details] [diff] [review]
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.
Attachment #737690 - Flags: review?(peterv)
Whiteboard: [need review]
(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?
> 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.
(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.
> 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...
(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?
Attachment #737690 - Flags: review?(peterv) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb589744522d
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/eb589744522d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.