Closed
Bug 1138874
Opened 10 years ago
Closed 10 years ago
Assertion failure occurs if compacting GC happens inside ReparentWrapper()
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
Details
Attachments
(2 files, 1 obsolete file)
7.09 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
1.96 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
While ReparentWrapper() is copying properties and cloning expandos from the original object, both this object and the new object have their DOM_OBJECT_SLOT set to point to the same native.
This is a problem with compacting GC because if the objects are moved then the object moved hook will be called to update the wrapper cache on native and will assert if it doesn't see the expected previous value. If only one object is moved this may work depending on the state of the wrapper cache (I'm not sure exactly) but if both objects are moved this is guaranteed to fail.
Comment 1•10 years ago
|
||
So it seems like we either replace the assert with a check or we need to change around ReparentWrapper's management of the pointer, yes?
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Not doing reviews right now from comment #1)
Yes. Is it possible to change ReparentWrapper() to swap the pointer from the original object to the new one after we've done the cloning, or does that depend on the new object pointing to the native already?
Comment 3•10 years ago
|
||
Alright, so...
We can definitely do the JS_CopyPropertiesFrom bits before we set DOM_OBJECT_SLOT on newobj, since that bit doesn't involve newobj in any way at all.
As far as CloneExpandoChain, the only place it uses dst is in the calls to getExpandoChain and attachExpandoObject.
getExpandoChain does ObjectScope(obj)->GetExpandoChain(obj). ObjectScope just uses the compartment of the object. GetExpandoChain just has a WeakMapPtr<JSObject*, JSObject*>. So getExpandoChain doesn't use DOM_OBJECT_SLOT.
attachExpandoObject does more getExpandoChain, a preserveWrapper, and setExpandoChain. The setExpandoChain bit is fine.
The problem, in theory, is preserveWrapper. This requires the DOM_OBJECT_SLOT to be set, since it needs to tell that DOM object to start tracing its JS reflector. However, in the specific case we're dealing with here, it would be fine if preserveWrapper just did nothing, because since we already had an expando chain and hence the wrapper was already preserved.
preserveWrapper already no-ops if UnwrapDOMObjectToISupports returns null. Unfortunately, UnwrapDOMObjectToISupports asserts that the DOM_OBJECT_SLOT value is a PrivateValue. That said, I'm changing that in bug 928336 to just return null if DOM_OBJECT_SLOT is an UndefinedValue(). So we could either grab that part of that patch into here, wait for that bug to land, or change ReparentWrapper to explicitly set PrivateValue(nullptr) on newobj across the CloneExpandoChain bit for now.
With that done, I'm pretty sure we can leave newobj not pointing to the actual DOM object until after we've done the CloneExpandoChain, at which point we can atomically (from a GC point of view) move over the DOM object pointer.
Comment 4•10 years ago
|
||
Er, I _meant_ to cc Peter and Bobby in case I'm missing something.
Assignee | ||
Comment 5•10 years ago
|
||
Great! Thanks for the detailed analysis. Here's a patch that makes does this.
Comment 6•10 years ago
|
||
That sounds good, yes, but please add the interesting bits of comment 3 into the patch as comments, and assert that the wrapper is already preserved.
Comment 7•10 years ago
|
||
We can't assert the wrapper is already preserved, since it might not be .. in the case when we have no Xray expandos.
But yes, we should have comments in preserveWrapper and here indicating the interdependency, as well as a comment for why we preset the slot to null.
Comment 8•10 years ago
|
||
(In reply to Not doing reviews right now from comment #7)
> We can't assert the wrapper is already preserved, since it might not be ..
> in the case when we have no Xray expandos.
Can't cloneExpandoChain assert it on src?
Comment 9•10 years ago
|
||
Yes, that we can do.
Comment 10•10 years ago
|
||
Note that I just landed bug 928336 so you don't need to do the initial set-to-null bit.
Assignee | ||
Comment 11•10 years ago
|
||
OK cool.
I've added some comments but I wasn't able to see how I could easily assert that the wrapper had been preserved in XrayTraits::cloneExpandoChain() without adding a new virtual to XrayTraits to either check this if necessary or return maybe return the wrapper cache if there is one. This applies to DOMXrayTraits only right? Let me know what you want me to do here.
Attachment #8572023 -
Attachment is obsolete: true
Attachment #8572713 -
Flags: review?(bobbyholley)
Comment 12•10 years ago
|
||
Comment on attachment 8572713 [details] [diff] [review]
bug1138874-reparent-wrapper v2
Review of attachment 8572713 [details] [diff] [review]:
-----------------------------------------------------------------
Yeah, just do:
#ifdef DEBUG
nsISupports *identity = mozilla::dom::UnwrapDOMObjectToISupports(target);
if (!identity) {
nsWrapperCache* cache = nullptr;
CallQueryInterface(identity, &cache);
MOZ_ASSERT_IF(cache, cache->PreservingWrapper());
}
#endif
Attachment #8572713 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
BTW I changed to assertion to check the wrapper cache for the source object, if there was no native set for the target.
Comment 15•10 years ago
|
||
2.13 +#ifdef DEBUG
2.14 + // When this is called from dom::ReparentWrapper() there will be no native
2.15 + // set for |dst|. Eventually it will be set to that of |src|. This will
2.16 + // prevent attachExpandoObject() from preserving the wrapper, but this is
2.17 + // not a problem because in this case the wrapper will already have been
2.18 + // preserved when expandos were originally added to |src|. In this case
2.19 + // assert the wrapper for |src| has been preserved.
2.20 + if (oldHead) {
2.21 + nsISupports *dstId = mozilla::dom::UnwrapDOMObjectToISupports(dst);
2.22 + if (!dstId) {
2.23 + nsISupports *srcId = mozilla::dom::UnwrapDOMObjectToISupports(src);
2.24 + if (srcId) {
2.25 + nsWrapperCache* cache = nullptr;
2.26 + CallQueryInterface(srcId, &cache);
2.27 + MOZ_ASSERT_IF(cache, cache->PreservingWrapper());
2.28 + }
2.29 + }
2.30 + }
2.31 +#endif
Wait, huh? Why do we skip checking that the wrapper is preserved if dst has a DOM object?
In general, the idea of the assertion was to check src. I'm not sure why the dst piece is there at all.
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #15)
Well because that is the situation we're trying to assert is going to be OK: when we can't preserve the wrapper on dst we ensure that the wrapper on src is already preserved.
But yes, it does make more sense to just assert that the wrapper is preserved on src if it has any expandos set.
Sorry I should have run this past you first but I thought that was the intent here.
Comment 17•10 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #16)
> (In reply to Bobby Holley (:bholley) from comment #15)
> Well because that is the situation we're trying to assert is going to be OK:
> when we can't preserve the wrapper on dst we ensure that the wrapper on src
> is already preserved.
Oh I see. Probably worth a comment explaining why we're doing it that way.
Comment 18•10 years ago
|
||
// if expandos are present then the wrapper will already already have been
// preserved called for this native.
Stray "called" there?
Comment 19•10 years ago
|
||
And stray 'already' ;-)
Assignee | ||
Comment 20•10 years ago
|
||
Well it does makes sense to generalise thise, and it's shorter.
Attachment #8573362 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Comment 21•10 years ago
|
||
Comment on attachment 8573362 [details] [diff] [review]
bug1138874-fix-assert
Review of attachment 8573362 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1013,3 @@
> if (oldHead) {
> + nsISupports *idendtity = mozilla::dom::UnwrapDOMObjectToISupports(src);
> + if (idendtity) {
This variable is misspelled.
Attachment #8573362 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 23•10 years ago
|
||
Ugh, fixed stupid misspelling and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0f7a65ebd3
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Assignee | ||
Comment 25•10 years ago
|
||
Flags: needinfo?(jcoppeard)
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fb0f7a65ebd3
https://hg.mozilla.org/mozilla-central/rev/aebb9cb5dfbb
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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
•