The case when we have a [Cached] or [StoreInSlot] attribute that has a sequence or dictionary type is interesting, because when the cache is filled we create a new JS object in the reflector compartment (see bug 946898). Then Xrays just see that content-side object, via a SJOW or whatever we call it nowadays. But since these are vanilla objects representing some underlying state that's expressed as a C++ nsTArray or dictionary struct, it would be better/safer to give the Xray its own JS reflection of that state. Bobby and I just talked about this for a bit and we have something like a plan for it, as follows: When getting a [Cached] or [StoreInSlot] attribute of sequence or dictionary type via Xrays, store the cached value in a slot on the Xray's holder, not on the reflector. This requires that we use a holder class with sufficiently many reserved slots. Most simply, we codegen holder classes for bindings with such attributes, and expose the JSClass* somewhere where Xrays can read it (e.g. the same place all the other Xray data hangs out). For bindings without such attributes, we'd just put nullptr there, since that's the JSClass* we use for the holder right now for WebIDL xrays. This would solve the issue completely, except for what happens if ClearCachedFooValue is called. We discussed several options: 1) Just enumerate all compartments, look for Xrays for our object, and clear the value on them. I fear this will be too slow. For example, for Contacts this would have to happen on every set of one of the array properties. 2) Have a flag on the object that says whether it has any Xrays, and if so do #1. Weird performance pitfall, plus I expect for Contacts we usually have an Xray. 3) Attach a sequence number to every prop in question and when getting on an Xray compare the sequence number to the underlying object before using the cached value. This would work, but needs twice the number of slots. 4) Attach a sequence number to the object in question and otherwise do as #3. This breaks the [Cached] semantics: clearing the cached value for one prop effectively clears it for all on Xrays. 5) Attach a weakmap of some sort to the content object (probably in a slot) that points to the globals of its extant Xrays. Xrays would need finalizers to remove themselves. Clearing a cached prop would enumerate the weakmap. #5 sounds like the most likely approach to be sane-ish. Just need to do it.... Comments welcome.

So the problem with removing Xrays in a finalizer is this: there is no guarantee the content object is still alive at that point. It might have been finalized already. And then the weakmap is gone too... But why would we ever need to remove ourselves anyway? Seems like as long as the global is live so will be the xray with the cached thing on it.... so just having a WeakSet of relevant globals should be good enough.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #1) > But why would we ever need to remove ourselves anyway? Seems like as long > as the global is live so will be the xray with the cached thing on it Not necessarily - the wrapper might have gone out of scope and been collected, no? But see below. > .... so > just having a WeakSet of relevant globals should be good enough. Even if it's slightly too conservative, I think this approach is totally sensible and fine.

> Not necessarily - the wrapper might have gone out of scope and been collected, no? No, because we're assuming it has an expando object (that the cached thing lives on), and hence is prevented from going away until the underlying object goes away. Put another way, a filled cache acts _exactly_ like an expando property in terms of effect on object lifetimes.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3) > > Not necessarily - the wrapper might have gone out of scope and been collected, no? > > No, because we're assuming it has an expando object (that the cached thing > lives on), and hence is prevented from going away until the underlying > object goes away. Put another way, a filled cache acts _exactly_ like an > expando property in terms of effect on object lifetimes. Expando objects hang off the target object, not the wrapper, right? If there are object-valued expandos, those objects will be held alive, and entrain the scope with it. But in the general case, there's nothing holding the wrapper itself, right? Or am I forgetting something?

> Expando objects hang off the target object, not the wrapper, right? Oh, so we do. I hadn't realized that. I just realized you were talking about putting the props on the holder, not on the expando. But if we do that, we need to come up with some sort of way to keep the holder actually alive, right? What if, instead, we just put the cached stuff on the Xray expando in the Xray case? That would give us the desired caching behavior, and we could clear the slots easily when the cache needs to be cleared: just walk our expando chain. The drawback is that we'd need to coordinate slots a bit between Xray code and DOM code, but that seems ok...

We talked about this on IRC, I think this sounds like a good approach. I haven't paged all of this into my head enough to be sure that there are no gotchas, but I can't think of any offhand.

So I've hit an interesting snag. We have devtools tests that poke at documents of windows that are removed from docshells. In the old world, this would hit the document cache (because we do NOT clear it when the window is removed from the docshell) and gotten a non-null document. Now that I am using a separate cache for the Xray, it falls through to the underlying window (because I don't force that cache to always be filled, unlike the normal object case) and gets null, which makes the test throw and fail. Some plausible options: 1) Ensure that the cache is always filled on the Xray expando in the document case. This is actually a bit annoying, because it requires always creating the Xray expando for this case, and may _still_ not solve the problem (e.g. if the Xray is first created after the window is already removed from docshell). 2) Change the window's document getter to poke at the "normal" cache if it doesn't find a document in some other way. This is safe, because in the end we just end up with the same document object. 3) Only use the Xray expando slots for dictionary/sequence types; use the slots on the underlying object otherwise. 4) Use the slots on the underlying object for all interface types, use the Xray expando slots otherwise. Options 3 and 4 are basically the same except in terms of whether the list of things that use the Xray expando slots is a whitelist or a blacklist... My temptation is to do option #4, and possibly even restrict to wrappercached interface types. That seems safe enough and should solve my devtools test problem, along with some navigatorproperty issues that I already solved by basically doing #4 for navigator properties only. Any objections?

Oh, I guess there is also: 5) Fix the devtools code to not poke at .document on this torn-down window, or to catch the exception if it does. But it seems like basically leaves the footgun in place for other chrome code, so I'd rather solve the underlying "make sure the document is never null" issue...

On IRC we decided on #4.

The other option, of course, is to just define an "expando slot count" constant in the header and then static_assert it has the right value once the ExpandoSlots enum is declared.

This guarantees that an interface type returned from a [Cached] or [StoreInSlot] getter must be wrappercached, because non-wrappercached things can only be returned from [NewObject] getters or methods.

Let me know if you want to see the codegen for the other bits (e.g. the ClearCached*Value function), but I was pretty sure you'd want to see the main output. ;)

Comment on attachment 8798511 [details] [diff] [review] part 3. Codegen Xray expando JSClasses for DOM objects with [Cached] or [StoreInSlot] members Review of attachment 8798511 [details] [diff] [review]: ----------------------------------------------------------------- r=me as best as I can review Codegen.py. ::: dom/bindings/Codegen.py @@ +553,5 @@ > + def define(self): > + return fill( > + """ > + static const JSClass sXrayExpandoObjectClass = { > + "XrayExpandoObject", I'm a bit worried that this is going to go out of sync with the others. Could you hoist this string into a helper somewhere, and somehow statically generated a default class by passing memberSlots=0? That would avoid the footgun where somebody tries to update the JSClass in XrayWrapper.cpp but misses the custom ones (which has a good chance of going undetected). @@ +554,5 @@ > + return fill( > + """ > + static const JSClass sXrayExpandoObjectClass = { > + "XrayExpandoObject", > + JSCLASS_HAS_RESERVED_SLOTS(xpc::JSSLOT_EXPANDO_COUNT + ${memberSlots}) | Can you add a comment here that this may allocate more slots than we actually need? Ideally one that appears both here and in the generated code.

Comment on attachment 8798512 [details] [diff] [review] part 4. Use the codegenned JSClass, if available, when creating Xray expando objects Review of attachment 8798512 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.cpp @@ +1892,5 @@ > obj, flags, props); > } > > +const JSClass* > +XrayGetExpandoClass(JSContext* cx, JS::Handle<JSObject*> obj) This function also becomes a one-liner if we just generate nativePropertyHooks to always point to the right thing. ::: dom/bindings/BindingUtils.h @@ +2401,5 @@ > } > > +/** > + * Get the Xray expando class to use for the given DOM object. Null will be > + * returned if the default Xray expando class should be used. the "Null" part of this comment will need updating with my other suggestion. ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +1164,5 @@ > + const JSClass* expandoClass = getExpandoClass(cx, target); > + if (!expandoClass) { > + expandoClass = &ExpandoObjectClass; > + } else { > + MOZ_ASSERT(!strcmp(expandoClass->name, "XrayExpandoObject")); Can we just teach getExpandoClass about the default, and get rid of this conditionality here? I think that also dovetails with my suggestion to hoist the default expando class to codegen.

> Could you hoist this string into a helper somewhere, and somehow statically generated a default class by passing memberSlots=0? Per IRC discussion, will make this a macro and then apply the other comments.

Comment on attachment 8798513 [details] [diff] [review] part 5. Clear the relevant slots on Xray expandos when clearing cached slots on a DOM object Review of attachment 8798513 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +38,5 @@ > > +def memberXrayExpandoReservedSlot(member, descriptor): > + return ("(xpc::JSSLOT_EXPANDO_COUNT + %d)" % > + member.slotIndices[descriptor.interface.identifier.name]) > + Nit: whitespace ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +10,5 @@ > > #include "nsDependentString.h" > #include "nsIScriptError.h" > #include "mozilla/dom/Element.h" > +#include "mozilla/dom/ScriptSettings.h" I can't quite figure out what uses ScriptSettings here. @@ +2426,5 @@ > return Traits::singleton.enumerateNames(cx, wrapper, flags, props); > } > > +void > +ClearXrayExpandoSlots(JSObject* target, size_t slotIndex) Can you move this right underneath cloneExpandoChain, which has more relevant code nearby? ::: js/xpconnect/wrappers/XrayWrapper.h @@ +600,5 @@ > +/* > + * Clear the given slot on all Xray expandos for the given object. > + * > + * Note that this function may be called on a thread on which Xrays don't > + * exist. In that case it should just do nothing. Nit: extra space. But I'd just say "No-op when called on non-main threads (where Xrays don't exist)."

> Nit: whitespace Fixed. > I can't quite figure out what uses ScriptSettings here. RootingCx() does. > Can you move this right underneath cloneExpandoChain, which has more relevant code nearby? Done. > But I'd just say "No-op when called on non-main threads (where Xrays don't exist)." Done.

> Can you add a comment here that this may allocate more slots than we actually need? Er, done locally. Sorry I missed that before posting the above interdiffs. :(

Comment on attachment 8798525 [details] [diff] [review] part 7. When getting a cacheable property off a DOM Xray, cache it on the Xray's expando object Review of attachment 8798525 [details] [diff] [review]: ----------------------------------------------------------------- I don't follow all the Codegen.py changes, but the generated code looks good. ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +2446,5 @@ > } > } > > +JSObject* > +EnsureXrayExpandoObject(JSContext* cx, JS::HandleObject wrapper) Please move this as well.

Comment on attachment 8798516 [details] [diff] [review] part 8. Add tests for the new [Cached] setup on Xrays Review of attachment 8798516 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the tests, as always. ::: dom/bindings/test/test_dom_xrays.html @@ +152,5 @@ > + var contacts = win.navigator.mozContacts; > + isnot(contacts, undefined, > + "Must have a mozContacts for this to work. Find another " + > + "NavigatorProperty, or remove all this NavigatorProperty code") > + ok(Cu.isXrayWrapper(contacts), You can also check Cu.getGlobalForObject.

Comment on attachment 8799533 [details] [diff] [review] Interdiff for the old part 3 Review of attachment 8799533 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Configuration.py @@ +278,5 @@ > DescriptorProvider.__init__(self) > self.config = config > self.interface = interface > > + self.wantsXrays = (not interface.isExternal() and Not totally sure where this interface.isExternal thing came from?

> Please move this as well. Yep, done when I was resolving merge conflicts on top of the previous changes. > You can also check Cu.getGlobalForObject. Nice, will do. > Not totally sure where this interface.isExternal thing came from? It's to avoid poking at interface.totalMembersInSlots in the case when the interface is external (and hence we're not going to codegen anything for it anyway), because IDLExternalInterface has no such member.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c4cad1447749 part 1. Move the ExpandoSlots enum to XrayWrapper.h. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/0a0046d3e726 part 2. Declare XrayExpandoObjectClassOps in XrayWrapper.h so we can use it from bindings code. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/802ef473a083 part 3. Create a macro for declaring Xray expando classes, and move the default Xray expand class definition to bindings code. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/01ab3f3823d5 part 4. Codegen Xray expando JSClasses for DOM objects with [Cached] or [StoreInSlot] members. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/dd2884c72021 part 5. Use the codegenned JSClass, if available, when creating Xray expando objects. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/efda96f7dd1c part 6. Clear the relevant slots on Xray expandos when clearing cached slots on a DOM object. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/1617e1d2c04f part 7. Forbid using [Cached] or [StoreInSlot] with [NewObject]. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/6576edddb9a6 part 8. When getting a cacheable property off a DOM Xray, cache it on the Xray's expando object. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/38892fd53242 part 9. Add tests for the new [Cached] setup on Xrays. r=bholley

https://hg.mozilla.org/mozilla-central/rev/c4cad1447749 https://hg.mozilla.org/mozilla-central/rev/0a0046d3e726 https://hg.mozilla.org/mozilla-central/rev/802ef473a083 https://hg.mozilla.org/mozilla-central/rev/01ab3f3823d5 https://hg.mozilla.org/mozilla-central/rev/dd2884c72021 https://hg.mozilla.org/mozilla-central/rev/efda96f7dd1c https://hg.mozilla.org/mozilla-central/rev/1617e1d2c04f https://hg.mozilla.org/mozilla-central/rev/6576edddb9a6 https://hg.mozilla.org/mozilla-central/rev/38892fd53242