Closed
Bug 946906
Opened 11 years ago
Closed 8 years ago
Create a safer setup for Xrays for bindings with [Cached] or [StoreInSlot] attributes
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 2 open bugs)
Details
Attachments
(15 files, 3 obsolete files)
2.14 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
5.01 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
4.25 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
2.72 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
2.35 KB,
text/plain
|
Details | |
1.53 KB,
text/plain
|
Details | |
2.83 KB,
text/plain
|
Details | |
1.78 KB,
text/plain
|
Details | |
18.93 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
5.77 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
6.46 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
8.09 KB,
patch
|
Details | Diff | Splinter Review | |
4.57 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
6.39 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
No longer blocks: SH-bindings
Assignee | ||
Comment 1•8 years ago
|
||
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.
Comment 2•8 years ago
|
||
(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.
Assignee | ||
Comment 3•8 years ago
|
||
> 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.
Comment 4•8 years ago
|
||
(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?
Assignee | ||
Comment 5•8 years ago
|
||
> 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...
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
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?
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 8•8 years ago
|
||
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...
Assignee | ||
Comment 10•8 years ago
|
||
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.
Attachment #8798509 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8798510 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8798511 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8798512 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8798513 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 15•8 years ago
|
||
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.
Attachment #8798514 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8798515 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8798516 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 18•8 years ago
|
||
Assignee | ||
Comment 19•8 years ago
|
||
Assignee | ||
Comment 20•8 years ago
|
||
Assignee | ||
Comment 21•8 years ago
|
||
Assignee | ||
Comment 22•8 years ago
|
||
Attachment #8798525 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•8 years ago
|
Attachment #8798515 -
Attachment is obsolete: true
Attachment #8798515 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 23•8 years ago
|
||
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. ;)
Updated•8 years ago
|
Attachment #8798509 -
Flags: review?(bobbyholley) → review+
Updated•8 years ago
|
Attachment #8798510 -
Flags: review?(bobbyholley) → review+
Comment 24•8 years ago
|
||
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.
Attachment #8798511 -
Flags: review?(bobbyholley) → review+
Comment 25•8 years ago
|
||
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.
Attachment #8798512 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 26•8 years ago
|
||
> 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 27•8 years ago
|
||
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)."
Attachment #8798513 -
Flags: review?(bobbyholley) → review+
Updated•8 years ago
|
Attachment #8798514 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 28•8 years ago
|
||
Attachment #8799532 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 29•8 years ago
|
||
Attachment #8799533 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 30•8 years ago
|
||
Attachment #8799534 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•8 years ago
|
Attachment #8798511 -
Attachment is obsolete: true
Assignee | ||
Comment 31•8 years ago
|
||
Attachment #8799535 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•8 years ago
|
Attachment #8799534 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 32•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8798512 -
Attachment is obsolete: true
Assignee | ||
Comment 33•8 years ago
|
||
> 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.
Assignee | ||
Comment 34•8 years ago
|
||
> 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 35•8 years ago
|
||
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.
Attachment #8798525 -
Flags: review?(bobbyholley) → review+
Comment 36•8 years ago
|
||
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.
Attachment #8798516 -
Flags: review?(bobbyholley) → review+
Updated•8 years ago
|
Attachment #8799532 -
Flags: review?(bobbyholley) → review+
Comment 37•8 years ago
|
||
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?
Attachment #8799533 -
Flags: review?(bobbyholley) → review+
Updated•8 years ago
|
Attachment #8799535 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 38•8 years ago
|
||
> 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.
Comment 39•8 years ago
|
||
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
Comment 40•8 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•