1567523 - Enabling Fission with e10s disabled silently fails

Status: NEW

(Core :: DOM: Navigation, defect, P5)

Description

[Andrew McCreight [:mccr8]]

Kris noticed that OSX tests looks very green with Fission enabled, which doesn't make any sense. When I run a Mochitest locally, it hangs before even creating a window. It sits in that state for a while, maybe a few minutes, until it eventually runs.

My theory is that something is going wrong with the creation of the hidden window. We create this eagerly on OSX, and lazily on other platforms. (See bug 827976.)

I see this in the console spew, which might be related:
0:02.72 GECKO(33763) [33763, Main Thread] WARNING: 'aUseRemoteSubframes && !mUseRemoteTabs', file /Users/amccreight/mc/docshell/base/nsDocShell.cpp, line 1572
0:02.79 GECKO(33763) [33763, Main Thread] WARNING: 'mIndex >= Count()', file /Users/amccreight/mc/xpcom/ds/nsStringEnumerator.cpp, line 202
0:02.95 GECKO(33763) [33763, Main Thread] WARNING: Cannot create non-remote fission window!: file /Users/amccreight/mc/toolkit/components/startup/nsAppStartup.cpp, line 605

Anyways, I can try investigating this, though if Kris happens to fix it first or whatever that's fine.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: log.txt

So, when you run a mochitest, it runs for a bit, then the little Nightly icon appears in the tray, then the Nightly icon goes away. Here's the log from right after the window goes away (I have a few minor edits in the logging.)

It does look like the warnings I was seeing before are related to the hidden window:
0:02.93 GECKO(35406) [35406, Main Thread] WARNING: 'aUseRemoteSubframes && !mUseRemoteTabs', file /Users/amccreight/mc/docshell/base/nsDocShell.cpp, line 1572
0:02.93 GECKO(35406) CREATING HIDDEN WINDOW POST:2
0:02.93 GECKO(35406) CREATING HIDDEN WINDOW YES
0:03.01 GECKO(35406) [35406, Main Thread] WARNING: 'mIndex >= Count()', file /Users/amccreight/mc/xpcom/ds/nsStringEnumerator.cpp, line 202
0:03.20 GECKO(35406) [35406, Main Thread] WARNING: Cannot create non-remote fission window!: file /Users/amccreight/mc/toolkit/components/startup/nsAppStartup.cpp, line 605

Comment 2

[Andrew McCreight [:mccr8]]

I figured this out. What is happening is that e10s is disabled, but Fission is enabled, which causes issues. This is happening because testing/mochitest/runtests.py sets browser.tabs.remote.autostart to false here:
https://searchfox.org/mozilla-central/rev/4436573fa3d5c4311822090e188504c10c916c6f/testing/mochitest/runtests.py#1922
Fission is still set to true.

I'm not sure why it is force disabling e10s to be off there. Maybe this is used for a profile that runs code for the test harness, rather than the Firefox we're actually testing? Anyways, if you set "fission.autostart" to be options.e10s there, then the test runs normally. That's not exactly a proper fix, but it at least explains the behavior.

There are a number of other places that manually set the autostart pref in testing:
https://searchfox.org/mozilla-central/search?q=remote.autostart&case=false&regexp=false&path=testing

Rather than fixing this particular location that sets a pref, it may make more sense to add a helper for checking the Fission pref that returns false if e10s is disabled.

Comment 3

[Andrew McCreight [:mccr8]]

I think my analysis was a little wrong in comment 2. It feels like there's some issue where doing mach build after changing firefox.js doesn't actually fix everything up, but if you then touch runtests.py, it does run whatever.

I think the actual issue is that setting the Fission pref via --setpref fission.autostart=true instead of in firefox.js causes issue. (Even setting it in all.js seems to work, though that does seem like a bad idea because the e10s pref is set to false there.) So I'm not sure if this is actually related to the issue kmag saw in automation for OSX. This immediate issue might even just be wontfix...

Comment 4

[Kris Maglione [:kmag]]

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

I think the actual issue is that setting the Fission pref via --setpref fission.autostart=true instead of in firefox.js causes issue. (Even setting it in all.js seems to work, though that does seem like a bad idea because the e10s pref is set to false there.) So I'm not sure if this is actually related to the issue kmag saw in automation for OSX. This immediate issue might even just be wontfix...

The fission test group in automation works by passing --setpref to the harness, so it likely is the same issue.

Comment 5

[Andrew McCreight [:mccr8]]

Here's some more weird data points.

If I do this, it seems to load properly:
./mach mochitest --setpref fission.autostart=true dom/tests/mochitest/whatwg/test_postMessage.html

However, if I do this instead, it does not, and I hit the condition I described before where e10s is enabled but Fission is not:
./mach mochitest --setpref fission.autostart=true dom/tests/mochitest/whatwg/test_postMessage
Also with the less weird:
./mach mochitest --setpref fission.autostart=true dom/tests/mochitest/whatwg/

So there's something different in the way the prefs are set up when multiple tests are being run, it looks like.

Comment 6

[Andrew McCreight [:mccr8]]

This is less weird than I thought. There are some mochitest-chrome in that directory, so if you run mochitests in a "wildcard" way, then the first thing it does is try to run the mochitest-chrome. Those of course always run with e10s disabled, so setting Fission enabled then does not work out. So we're back to this probably not having anything to do with the weird OS X behavior in automation.

Comment 7

[Andrew McCreight [:mccr8]]

I guess this is really about how we should fail more visibly in testing when e10s is disabled and Fission is enabled.

There's also the question of what we should do in non-testing situations. We don't want to crash, because that'll just make the user's profile useless. We could either enable e10s or disable Fission. I feel like the latter is more true to the user's intent. If we roll out Fission, and some user has e10s disabled, they probably don't want Fission either. I think Kris and Nika were arguing for the former. It would certainly be easier to do, because e10s checking is more centralized.

Comment 8

[Chris Peterson [:cpeterson]]

Roll unfixed test bugs from Fission Milestone M4 to M4.1

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 9

[Andrew McCreight [:mccr8]]

I think the --enable-fission works well enough that developers aren't typically running into this problem of misconfiguration.

Comment 10

[Chris Peterson [:cpeterson]]

Doesn't need to block Fission MVP because e10s-disabled is not supported, but it was used for some tests.

Comment 11

[Andrew McCreight [:mccr8]]

I don't think I've seen developers getting confused by this, so hopefully it isn't too important.