Assertion failure occurs if compacting GC happens inside ReparentWrapper()

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
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

4 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?
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.
Er, I _meant_ to cc Peter and Bobby in case I'm missing something.
(Assignee)

Comment 5

4 years ago
Created attachment 8572023 [details] [diff] [review]
bug1138874-reparent-wrapper

Great!  Thanks for the detailed analysis.  Here's a patch that makes does this.
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.
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.
(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?
Yes, that we can do.
Note that I just landed bug 928336 so you don't need to do the initial set-to-null bit.
(Assignee)

Comment 11

4 years ago
Created attachment 8572713 [details] [diff] [review]
bug1138874-reparent-wrapper v2

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 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 14

4 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.
    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

4 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.
(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.
  // if expandos are present then the wrapper will already already have been
  // preserved called for this native.

Stray "called" there?
And stray 'already' ;-)
(Assignee)

Comment 20

4 years ago
Created attachment 8573362 [details] [diff] [review]
bug1138874-fix-assert

Well it does makes sense to generalise thise, and it's shorter.
Attachment #8573362 - Flags: review?(bobbyholley)
(Assignee)

Updated

4 years ago
Keywords: leave-open
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

4 years ago
Ugh, fixed stupid misspelling and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0f7a65ebd3
(Assignee)

Updated

4 years ago
Keywords: leave-open
Note comment 18 and 19?
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 25

4 years ago
Sorry, fixed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/aebb9cb5dfbb
Flags: needinfo?(jcoppeard)
https://hg.mozilla.org/mozilla-central/rev/fb0f7a65ebd3
https://hg.mozilla.org/mozilla-central/rev/aebb9cb5dfbb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.