Currently DOM Nodes' and XHRs' wrappers are stored in their owner document. I'm not quite that is the right way. Perhaps scriptContext or global object would be a better place?

I've actually been working on a patch for the last week or two that stores them in the objects themselves. I need to update my tree for the recent XHR changes.

(In reply to comment #0) > I'm not quite that is the right way. +sure Is it possible that we have several wrappers we should be able store? Currently that isn't possible because node is the key and wrapper the value in the hashtable.

We should only have one wrapper for these objects. Patch gives a 20-30% speedup on some of the dromaeo DOM tests.

Comment on attachment 341136 [details] [diff] [review] v1 >@@ -744,9 +842,19 @@ protected: > * NODE_* macros for the layout of the bits in this > * member. > */ > PtrBits mFlagsOrSlots; >+ >+ nsPreservedWrapper mWrapper; > }; Could we store mWrapper in slots?

Don't think that's necessary (see also https://bugzilla.mozilla.org/show_bug.cgi?id=453738#c27).

I wonder if it'd be worth adding a new class with no virtual methods and one member to point to the wrapper/JSObject? That way it'd be possible to QI to that class w/o doing any reference counting, and down the road we could maybe bake this in with the class we're considering adding for this pointer offsets etc as well.

Comment on attachment 341195 [details] [diff] [review] v1.1 Ugh, should have cleared those. I'll attach a newer version of the patch tomorrow.

Comment on attachment 344619 [details] [diff] [review] v2 XPCWrappedNative::GetUsedOnly(): + nsWrapperCache* cache; + Object->QueryInterface(NS_GET_IID(nsWrapperCache), (void**)&cache); + if(cache && + (wrapper = static_cast<XPCWrappedNative*>(cache->GetWrapper()))) + { + NS_ADDREF(wrapper); + } + else + { [do expensive stuff] If the above QI succeeds, there's no point in going into the else case even if there is no wrapper in the cache since we won't find the wrapper in the map etc either. r+sr=jst with that.

Patch that was landed.

Backed out for unit test failures: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1225722086.1225726095.12145.gz&fulltext=1

The problem is xpcshell tests that create an XMLHttpRequest object, we'll have a null mOwner and so the PreCreate hook returns an error.

Can't we just avoid changing parentObj in that case?

And can we somehow assert (ideally statically) that the only way to get into that precreate method is with XHR?

Yeah, I think I agree with bz here, given that this case does indeed happen. It'd been nice to not have to go there, but I don't see a better way to deal with that.

Had to back out again, mailnews tinderboxes went orange. Current theory is that because their (hand-coded) QI implementations don't set result pointer to null on failure, we'll get a bogus pointer when doing nsWrapperCache* cache; CallQueryInterface(src, &cache); I'm going to try to check in this patch (with |nsWrapperCache* cache = nsnull;|) early tomorrow morning.

Had to back out again, orange due to leaks. I found one of the causes, need to release wrapper when FindTearoff fails in XPCConvert::NativeInterface2JSObject, but there's still some leaks remaining.

Patch in bug 464114 fixes some of the leaks that show up. Still hunting more.

Actually, on trunk with the patch from bug 464114 this doesn't cause any additional leaks (borwser-chrome tests already leak on tinderbox). So this is now blocked on review and landing of the patch in bug 464114.

We really should get this in for Beta2 as has always been the plan. The bug for some reason never got the flag.

This is the patch that I'm going to land. It fixes any remaining leaks when running mochitests (apart from the one fixed in bug 464114).

Looks like it finally sticks. Filed bug 464735 for figuring out a better fix for the leaks.

This seems to have caused at least one change in whether chrome sees XPCNativeWrappers. See bug 471395.

Also apparently caused bug 498897