1138874 - Assertion failure occurs if compacting GC happens inside ReparentWrapper()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jon Coppeard (:jonco)]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Jon Coppeard (:jonco)]

(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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Er, I _meant_ to cc Peter and Bobby in case I'm missing something.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1138874-reparent-wrapper

Great!  Thanks for the detailed analysis.  Here's a patch that makes does this.

Comment 6

[Bobby Holley (:bholley)]

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

[Boris Zbarsky [:bzbarsky]]

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

[Bobby Holley (:bholley)]

(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

[Boris Zbarsky [:bzbarsky]]

Yes, that we can do.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note that I just landed bug 928336 so you don't need to do the initial set-to-null bit.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: 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.

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e7a1c386a75

Comment 14

[Jon Coppeard (:jonco)]

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

[Bobby Holley (:bholley)]

    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.

Comment 16

[Jon Coppeard (:jonco)]

(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

[Bobby Holley (:bholley)]

(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

[Boris Zbarsky [:bzbarsky]]

  // if expandos are present then the wrapper will already already have been
  // preserved called for this native.

Stray "called" there?

Comment 19

[Bobby Holley (:bholley)]

And stray 'already' ;-)

Comment 20

[Jon Coppeard (:jonco)]

Attachment: bug1138874-fix-assert

Well it does makes sense to generalise thise, and it's shorter.

Comment 21

[Bobby Holley (:bholley)]

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.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e7a1c386a75

Comment 23

[Jon Coppeard (:jonco)]

Ugh, fixed stupid misspelling and pushed:

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

Note comment 18 and 19?

Comment 25

[Jon Coppeard (:jonco)]

Sorry, fixed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/aebb9cb5dfbb

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb0f7a65ebd3
https://hg.mozilla.org/mozilla-central/rev/aebb9cb5dfbb