Remote outer window proxies aren't CCWs, so they can't have Xrays, so they can't have Xray waivers. However, if we do a navigation from a local iframe to a remote iframe, we'll transplant a remote outer window proxy onto a local outer window proxy, which might have an Xray. This can cause some issues, particularly if we later navigate back to a different local window (as in bug 1559489).

To work around this, I think we should nuke Xray waivers on navigation to a remote outer window proxy. This makes Xray waiver behavior inconsistent with the non-Fission behavior, but I think Nika and Kris had some concerns that changing the non-Fission behavior might affect addons, so I'll leave changing that to a future bug that depends on this one.

Remote outer window proxies aren't CCWs, so they can't have Xrays, so
they can't have Xray waivers that do anything useful. However, if we
do a navigation from a local iframe to a remote iframe, we'll
transplant a remote outer window proxy onto a local outer window
proxy, which might have an Xray. This can cause some issues,
particularly if we later navigate back to a different local window.

To work around this, this patch nukes Xray waivers on navigation to a
remote outer window proxy. This makes Xray waiver behavior
inconsistent with the non-Fission behavior, but it is safer to leave
the non-Fission behavior alone for now, for fear of breaking addons.

(In reply to Andrew McCreight [:mccr8] from comment #1)

This makes Xray waiver behavior
inconsistent with the non-Fission behavior, but it is safer to leave
the non-Fission behavior alone for now, for fear of breaking addons.

Doesn't this just kick the can down the road such that these addons break when we ship fission? I'd think we'd want to ship this (nuking waivers when navigating cross-origin) now to identify any compat fallout.

(In reply to Bobby Holley (:bholley) from comment #2)

Doesn't this just kick the can down the road such that these addons break when we ship fission? I'd think we'd want to ship this (nuking waivers when navigating cross-origin) now to identify any compat fallout.

Yes, it does. I filed bug 1570487 for figuring out what happens if we make the behavior consistent. You are right that we should look into it sooner rather than later, but I didn't want to block fixing some Fission issues on it.

(In reply to Andrew McCreight [:mccr8] from comment #3)

(In reply to Bobby Holley (:bholley) from comment #2)

Doesn't this just kick the can down the road such that these addons break when we ship fission? I'd think we'd want to ship this (nuking waivers when navigating cross-origin) now to identify any compat fallout.

Yes, it does. I filed bug 1570487 for figuring out what happens if we make the behavior consistent. You are right that we should look into it sooner rather than later, but I didn't want to block fixing some Fission issues on it.

Is there any sort of investigative work we might do other than just shipping it and seeing if anything breaks? If so, I think we might as well just try shipping it everywhere now, since it's the same amount of work and we'll otherwise almost certainly forget about this until we're much closer to shipping Fission.

(In reply to Bobby Holley (:bholley) from comment #4)

Is there any sort of investigative work we might do other than just shipping it and seeing if anything breaks? If so, I think we might as well just try shipping it everywhere now, since it's the same amount of work and we'll otherwise almost certainly forget about this until we're much closer to shipping Fission.

Getting this stuff landed now unblocks a lot of other work, so I'd rather not get distracted by potential add-on fallout until we've gotten as far as we can get without dealing with it.

There are also multiple options for dealing with this in add-on scopes, including keeping the dangling waivers alive in some other, inactive form, and reenabling them when we navigate back to the window they originally pointed to. That wouldn't be my first choice (I really don't want people keeping long-term references to X-ray waivers at all), but it's an option.

Either way, I don't think this is the right time to worry about it.

Ok, I'll defer to how y'all want to run it.

I think enabling it for all navigations is just a one line patch (changing the transplant call in SetNewDocument) once this is landed, so I can experiment with that independently of the Fission transplanting work. For investigation, I just mean I haven't even tried to see if the tree is green with it, and I'd want to try uBlock Origin or something to see if anything falls over. By putting it separately, it will be easier to back out if something goes wrong.

(In reply to Andrew McCreight [:mccr8] from comment #7)

I think enabling it for all navigations is just a one line patch (changing the transplant call in SetNewDocument) once this is landed, so I can experiment with that independently of the Fission transplanting work. For investigation, I just mean I haven't even tried to see if the tree is green with it, and I'd want to try uBlock Origin or something to see if anything falls over. By putting it separately, it will be easier to back out if something goes wrong.

If we do decide to flip that switch in the near future, it should probably be nightly-only for at least a couple of cycles.

Backed out changeset 344a525cddbc (Bug 1570484) for spidermonkey bustage at BaseProxyHandler.cpp:389:25.

Backout: https://hg.mozilla.org/integration/autoland/rev/739df94ca7b8f809a122d7eb2c0e9d2da9e873c3

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=344a525cddbce9c754de16ca796ba642b4ae6b7b&selectedJob=259940275

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=259940275&repo=autoland&lineNumber=1594

It looks like the Firefox build is bootlegging WrapperObject.h in a way that the shell build doesn't.

Locally, I moved BaseProxyHandler.cpp to SOURCES and did a Firefox build. Hopefully that's sufficient. I also had to add an include for DeadObjectProxy.h. This base proxy handler doesn't include many files.

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