Closed Bug 1516237 Opened 8 months ago Closed 8 months ago

Handle the same-compartment case in js::RemapWrapper

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

With same-compartment realms it's possible for the wrapper and the new target to be in the same compartment.
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
Flags: needinfo?(bobbyholley)
(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.
Attached patch WIP (obsolete) — Splinter Review
(At this point this is more of a feedback/review request on the attached patch..)
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?
Flags: needinfo?(bobbyholley)
(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.
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.
Attachment #9033170 - Attachment is obsolete: true
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c5d46599eb99
Fix FixWaiverAfterTransplant to nuke CCWs for oldWaiver in the new compartment. r=bholley
https://hg.mozilla.org/mozilla-central/rev/c5d46599eb99
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.