Closed Bug 1572895 Opened 2 months ago Closed 2 months ago

Update some XPConnect mochitests to work under Fission

Categories

(Core :: XPConnect, task)

task
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Fission Milestone M4
Tracking Status
firefox70 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.

Boris, test_bug505915.html checks if cross origin access causes a specific kind of security error. With Fission, this particular access just isn't allowed at all, so Kris's change to make the test work when Fission is testing something else entirely (it just confirms that you can't access the thing cross origin at all). Do you think that's okay? Maybe we can assume that non-Fission tests are always going to be run?

Flags: needinfo?(bzbarsky)

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

Boris, test_bug505915.html checks if cross origin access causes a specific kind of security error. With Fission, this particular access just isn't allowed at all, so Kris's change to make the test work when Fission is testing something else entirely (it just confirms that you can't access the thing cross origin at all). Do you think that's okay? Maybe we can assume that non-Fission tests are always going to be run?

To be clear, it tests the same thing with Fission is disabled. When Fission is enabled, it just tests that we don't have access to the cross-origin document at all, which has the same effect of not allowing us to create a tree walker for it.

The alternative is to just skip the test under Fission, but I think it's nice to continue testing that the same thing will never be possible under the same circumstances of the original bug.

(In reply to Kris Maglione [:kmag] from comment #3)

To be clear, it tests the same thing with Fission is disabled. When Fission is enabled, it just tests that we don't have access to the cross-origin document at all, which has the same effect of not allowing us to create a tree walker for it.
Well, it tests that the access fails, but the original bug was about making sure that it actually failed due to NS_ERROR_XPC_SECURITY_MANAGER_VETO and not something else, AFAICT.

The alternative is to just skip the test under Fission, but I think it's nice to continue testing that the same thing will never be possible under the same circumstances of the original bug.

That's true. The alternative I was thinking of was adding a chrome Mochitest for this behavior, but maybe that's overkill.

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

The alternative is to just skip the test under Fission, but I think it's nice to continue testing that the same thing will never be possible under the same circumstances of the original bug.

That's true. The alternative I was thinking of was adding a chrome Mochitest for this behavior, but maybe that's overkill.

Well, if we created a chrome mochitest, it would have to be a chrome mochitest which creates a content window, which creates another content window, and does all of the actual work.

That's kind of not the point of chrome mochitests :)

(That said, at this point it could pretty easily be converted to an xpcshell test, which would be much more efficient)

The simplest fix to the test would be if we could use a URL for the iframe which is same-site with the mochitest hostname (and hence same-process) but not same-origin. Maybe something on //test1.mochi.test:8888 or something?

Flags: needinfo?(bzbarsky)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #7)

The simplest fix to the test would be if we could use a URL for the iframe which is same-site with the mochitest hostname (and hence same-process) but not same-origin. Maybe something on //test1.mochi.test:8888 or something?

Even if that works now (and I don't think it does), it will stop working soon. We use the entire origin string to choose the process to put a frame in. The only exception right now is that we accidentally ignore origin attributes, since we construct a principal from just the URI to get the origin string from, but that's going to change.

If we want to continue testing the cross-origin case, I guess I'll just migrate it to an xpcshell test with a windowless browser.

Even if that works now (and I don't think it does), it will stop working soon. We use the entire origin string to choose the process to put a frame in

Uh... how can that possibly work? The test is running on http://mochi.test:8888, right? If the subframe is on http://test1.mochi.test:8888, then they can synchronously access each other if they both set document.domain to mochi.test, so you can't put them in separate processes. If we do put such things in separate processes with Fission, that is a major bug we need to fix.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #9)

Even if that works now (and I don't think it does), it will stop working soon. We use the entire origin string to choose the process to put a frame in

Uh... how can that possibly work? The test is running on http://mochi.test:8888, right? If the subframe is on http://test1.mochi.test:8888, then they can synchronously access each other if they both set document.domain to mochi.test, so you can't put them in separate processes. If we do put such things in separate processes with Fission, that is a major bug we need to fix.

Oh, sorry, I misunderstood what you were suggesting. Yeah, I suppose that might work. We construct the webisolated origin string using siteOrigin, so those would always wind up in the same process.

OK. I'd prefer it if we did the test1.mochi.test thing and didn't change the rest of the test.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #11)

OK. I'd prefer it if we did the test1.mochi.test thing and didn't change the rest of the test.

Already done :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/4477d849b72a0cf15159b1f99a8530a250575365
Bug 1572895 - Update some XPConnect mochitests to work under Fission. r=mccr8
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Fission Milestone: --- → M4
You need to log in before you can comment on or make changes to this bug.