1516237 - Handle the same-compartment case in js::RemapWrapper

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

With same-compartment realms it's possible for the wrapper and the new target to be in the same compartment.

Comment 1

[Jan de Mooij [:jandem]]

If we start with this:

    Compartment 1  |  Compartment 2
    -------------------------------------------
    CCW1          --> XrayWaiver -> WindowProxy

Then create a new realm + WindowProxy in compartment 1 and transplant the WindowProxy in compartment 2, we get:

    Compartment 1  |  Compartment 2
    -------------------------------------------------
    CCW1          --> XrayWaiver -> CCW2(WindowProxy)
    XrayWaiver
    WindowProxy

In FixWaiverAfterTransplant we then recompute all CCWs for the old XrayWaiver in compartment 2 to point to the new XrayWaiver in compartment 1. However CCW1 and the new XrayWaiver are now same-compartment so we fail an assert in js::RemapWrapper.

Initially I thought we could transplant the CCW with a same-compartment wrapper to the same-compartment XrayWaiver, like this:

    Compartment 1  |  Compartment 2
    --------------------------------------------------------
    Wrapper         [unused XrayWaiver] -> CCW2(WindowProxy)
     -> XrayWaiver
      -> WindowProxy

It works but it breaks the invariant [0] that XrayWaivers are only referenced by CCWs. Another option is to do an UncheckedUnwrap, eliminating the XrayWaiver but this might be wrong if we then again load a content page and transplant the WindowProxy in compartment 1 with a CCW? Then we will end up with a vanilla non-waived Xray I think.

[0] https://searchfox.org/mozilla-central/rev/70a4778dd84ec94b4b3a2537fd28482de45b9a00/js/xpconnect/wrappers/WrapperFactory.cpp#624-626

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Another option is to do an UncheckedUnwrap, eliminating
> the XrayWaiver but this might be wrong if we then again load a content page
> and transplant the WindowProxy in compartment 1 with a CCW? Then we will end
> up with a vanilla non-waived Xray I think.

Hm looking at ShouldWaiveXray I think this is what will happen anyway because waivers are not preserved if not same-origin.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: WIP

Comment 4

[Jan de Mooij [:jandem]]

(At this point this is more of a feedback/review request on the attached patch..)

Comment 5

[Bobby Holley (:bholley)]

So this is reasonable (and very well-documented - thank you!), but I'm still a bit unhappy about introducing this kind of complexity so deep in the wrapper remapping code to handle this very particular case.

What about the following instead: in xpc::TransplantObject, we explicitly check whether the object has a waiver, and if so, whether the target compartment has a wrapper pointing to that waiver. If so, we nuke that CCW. This scopes the code closer to the case it handles, which is also nearby to the other waiver fixup code.

WDYT?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #5)
> What about the following instead: in xpc::TransplantObject, we explicitly
> check whether the object has a waiver, and if so, whether the target
> compartment has a wrapper pointing to that waiver. If so, we nuke that CCW.

OK I can try that. Apart from the test I added here I only hit this case on a print-preview browser-chrome test, so hopefully it will work.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1516237 - Fix FixWaiverAfterTransplant to nuke CCWs for oldWaiver in the new compartment. r?bholley

This case can come up with same-compartment realms. Keeping these CCWs
would confuse RemapWrapper because it'd be called with the CCW and target
in the same compartment.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5d46599eb99
Fix FixWaiverAfterTransplant to nuke CCWs for oldWaiver in the new compartment. r=bholley

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c5d46599eb99