1404107 - Xray expandos can get lost when reparenting wrappers

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Consider the attached testcase.  It fails this subtest:

    browser.test.assertEq(node.expando, 6,
                         "Adoption should not change expandos (2)");

The reason that fails is that the reparenting process on adoption looks like this:

1)  Create a new reflector.
2)  Clone the Xray expandos over.
3)  JS_TransplantObject(old, new).

Step 3, in this case, finds the existing CCW from the new compartment to the old reflector.  It then swap()s the new reflector with the CCW, and uses the now-no-longer-ccw as the new reflector.  So we end up with an Xray to what used to be the CCW.

But when we clone the Xray expandos we use the "new reflector" value as the hashtable key.  Since we then forget about that JSObject* (and use the existing CCW instead), we lose the expandos.

Plausible fixes here include doing the "we have an existing CCW" detection earlier (before we get into JS_TransplantObject) and keying off the right thing when cloning the Xray expandos, or changing the ordering of things so we JS_TransplantObject before we clone the Xray expandos.  This last should work, I think, because Xray expando cloning only depends on the compartment and JSObject* value of "src", and JS_TransplantObject won't affect either of those.

Comment 1

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e6a7747c38befe8df5f62d3a734fc5397825290 is an attempt at a quick hack for the "change the ordering" option.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Text that Xray expandos are not lost when reparenting wrappers

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Fix cloning expando chains when reparenting DOM objects

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Refactor: Move some reparenting complexity into XPConnect

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8915658 [details] [diff] [review]
Fix cloning expando chains when reparenting DOM objects

This looks reasonable.

It would be good to have an explicit test for the case described in the "NB: It's important to do this _after_ copying the properties" bit right after the GetExpandoChain call, but involving Xrays.  That is, set "foo.x = foo" from the Xray side (so one of the expando objects points to foo itself) and then make sure weird things don't happen.

This changeset presumably needs to land before the test changeset.

r=me

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8915659 [details] [diff] [review]
Refactor: Move some reparenting complexity into XPConnect

r=me

Comment 7

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39cf0e5a3aeb54e17c9bdd1e24aa88428de4981e

Comment 8

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c5ab491f2fd0e7f59124ad26a2e74f31f45e94
Bug 1404107 - Fix cloning expando chains when reparenting DOM objects. r=bz,r=mrbkap

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d427a154d33e0b16bdd0ad01e802047f8930c7f
Bug 1404107 - Test that Xray expandos are not lost when reparenting wrappers. r=mrbkap

https://hg.mozilla.org/integration/mozilla-inbound/rev/886a50c39daad5843af3232ef54738e1462b08e2
Bug 1404107 - Test that an Xray expando whose value is a DOM node works when the node is reparented. no_r=me.

https://hg.mozilla.org/integration/mozilla-inbound/rev/208cf9b36e87238ae9694a74d7ea8b3baec57796
Bug 1404107 - Refactor: Move some reparenting complexity into XPConnect. r=mrbkap,r=bz

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a9c5ab491f2f
https://hg.mozilla.org/mozilla-central/rev/6d427a154d33
https://hg.mozilla.org/mozilla-central/rev/886a50c39daa
https://hg.mozilla.org/mozilla-central/rev/208cf9b36e87

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8915658 [details] [diff] [review]
Fix cloning expando chains when reparenting DOM objects

Approval Request Comment
[Feature/Bug causing the regression]: Probably been broken for a while...
[User impact if declined]: Can't land the fix for bug 1355109
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Yes.
[Why is the change risky/not risky?]: This is changing pretty delicate code,
   and in particular, it's changing the ordering of steps in this code.  We
   believe the new ordering is correct and that we have tested all the edge
   cases that can come up, but this is still pretty risky.  If it were not for
   the benefits we expect from bug 1355109, I would not be requesting this
   uplift.
[String changes made/needed]: None.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915658 [details] [diff] [review]
Fix cloning expando chains when reparenting DOM objects

Needed for bug 1355109, Beta57+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/41e3144ad0da
https://hg.mozilla.org/releases/mozilla-beta/rev/de5bfee27df0
https://hg.mozilla.org/releases/mozilla-beta/rev/e3697ba2690a
https://hg.mozilla.org/releases/mozilla-beta/rev/68132ce4a0ed

Comment 13

[Andrei Vaida [:avaida]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Boris's assessment on manual testing needs and the fact that this fix has automated coverage.