1884814 - Respect MOZ_FORCE_DISABLE_E10S if MOZILLA_OFFICIAL is not set

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Rob Wu [:robwu]]

The intent of bug 1724089 was to remove a non-functional pref, with the explicit note that the intended behavior (of disabling e10s) can be achieved through the MOZ_FORCE_DISABLE_E10S pref.

The ultimately landed patch does not only remove the pref, it also effectively disables the ability to use the MOZ_FORCE_DISABLE_E10S environment variable outside of automation. The relevant diff is at https://hg.mozilla.org/mozilla-central/rev/4de25e9a9500#l27.59

Before the patch, the condition to disable e10s was:

• if automation (or non-official build, or non-browser build), check the browser.tabs.remote.autostart pref --- this logic didn't work, hence bug 1724089
• if Android, disable if MOZ_FORCE_DISABLE_E10S is set
• otherwise, disable if MOZ_FORCE_DISABLE_E10S is set to the application version.

Now, the logic has become:

• if automation, disable if MOZ_FORCE_DISABLE_E10S is set (and note: xpcshell tests are not always "in automation" - bug 1598804)

This means that it has become impossible to launch Firefox with e10s disabled. This was not intentional. The following condition should also be accepted:

• Disable if MOZ_FORCE_DISABLE_E10S is set to the application version.

The original logic populated MOZ_FORCE_DISABLE_E10S with the content of version.txt https://hg.mozilla.org/mozilla-central/rev/4de25e9a9500#l11.12

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1724089

:gregp, since you are the author of the regressor, bug 1724089, could you take a look?

For more information, please visit BugBot documentation.

Comment 2

[Gregory Pappas [:gregp]]

if automation, disable if MOZ_FORCE_DISABLE_E10S is set

This is not the logic. The logic is

• Disable if MOZ_DISABLE_NONLOCAL_CONNECTIONS and MOZ_FORCE_DISABLE_E10S are both set.

it has become impossible to launch Firefox with e10s disabled

You can set MOZ_DISABLE_NONLOCAL_CONNECTIONS alongside MOZ_FORCE_DISABLE_E10S to disable E10S. Of course, that might not be particularly useful depending on what you're trying to accomplish.

Comment 3

[Gregory Pappas [:gregp]]

(didn't mean to cancel NI, I will post a patch to bring back the application version logic if that is welcome).

Mossop, what do you think? Should we bring back the ability to disable E10S if the env variable is set to the current version of the application?

Comment 4

[Rob Wu [:robwu]]

(In reply to Gregory Pappas [:gregp] from comment #2)

if automation, disable if MOZ_FORCE_DISABLE_E10S is set

This is not the logic. The logic is

• Disable if MOZ_DISABLE_NONLOCAL_CONNECTIONS and MOZ_FORCE_DISABLE_E10S are both set.

This is equivalent to automation. In practice, this condition is true for unit tests (e.g. mochitests), but not always for xpcshell tests (bug 1598804). When MOZ_DISABLE_NONLOCAL_CONNECTIONS is set, the application crashes when outbound connections are made, which renders it useless for testing anything that requires an internet connection.

it has become impossible to launch Firefox with e10s disabled

You can set MOZ_DISABLE_NONLOCAL_CONNECTIONS alongside MOZ_FORCE_DISABLE_E10S to disable E10S. Of course, that might not be particularly useful depending on what you're trying to accomplish.

Firefox crashes immediately when I use this combination:

$ MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 MOZ_FORCE_DISABLE_E10S=1 firefox --no-remote -profile /tmp/profdir
FATAL ERROR: Non-local network connections are disabled and a connection attempt to location.services.mozilla.com (44.239.216.242) was made.
You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.
FATAL ERROR: Non-local network connections are disabled and a connection attempt to location.services.mozilla.com (44.239.216.242) was made.
You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.
ExceptionHandler::GenerateDump cloned child 1123054
ExceptionHandler::SendContinueSignalToChild sent continue signal to child
ExceptionHandler::WaitForContinueSignal waiting for continue signal...

Comment 5

[Gregory Pappas [:gregp]]

In practice, this condition is true for unit tests (e.g. mochitests), but not always for xpcshell tests (bug 1598804)

I might be missing something, but it seems like non-local connections are disabled in XPCShell tests?
https://searchfox.org/mozilla-central/rev/a3d5a112ddb2d665b0c7ac2919b6f4fc6c97366c/testing/xpcshell/runxpcshelltests.py#1275

Comment 6

[Rob Wu [:robwu]]

(In reply to Gregory Pappas [:gregp] from comment #5)

In practice, this condition is true for unit tests (e.g. mochitests), but not always for xpcshell tests (bug 1598804)

I might be missing something, but it seems like non-local connections are disabled in XPCShell tests?
https://searchfox.org/mozilla-central/rev/a3d5a112ddb2d665b0c7ac2919b6f4fc6c97366c/testing/xpcshell/runxpcshelltests.py#1275

You're right, I'm conflating the two. isInAutomation requires MOZ_DISABLE_NONLOCAL_CONNECTIONS=1, but that doesn't automatically imply the opposite - it is possible to set MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 without isInAutomation being true. So e10s can always be disabled in unit tests.

Outside of automation, MOZ_FORCE_DISABLE_E10S does not work in practice though.

Comment 7

[Gregory Pappas [:gregp]]

Outside of automation, MOZ_FORCE_DISABLE_E10S does not work in practice though.

I am able to set the two env variables and run an existing Nightly profile with E10S disabled without crashing. Offhand, I'd guess that you're getting instant crashes because the first time a profile is run, location services are needed to download the correct search engines for the region.

Comment 8

[Dave Townsend [:mossop]]

(In reply to Rob Wu [:robwu] from comment #4)

(In reply to Gregory Pappas [:gregp] from comment #2)

if automation, disable if MOZ_FORCE_DISABLE_E10S is set

This is not the logic. The logic is

• Disable if MOZ_DISABLE_NONLOCAL_CONNECTIONS and MOZ_FORCE_DISABLE_E10S are both set.

This is equivalent to automation. In practice, this condition is true for unit tests (e.g. mochitests), but not always for xpcshell tests (bug 1598804). When MOZ_DISABLE_NONLOCAL_CONNECTIONS is set, the application crashes when outbound connections are made, which renders it useless for testing anything that requires an internet connection.

What tests do we have that require this?

Comment 9

[Rob Wu [:robwu]]

(In reply to Dave Townsend [:mossop] from comment #8)

(In reply to Rob Wu [:robwu] from comment #4)

When MOZ_DISABLE_NONLOCAL_CONNECTIONS is set, the application crashes when outbound connections are made, which renders it useless for testing anything that requires an internet connection.

What tests do we have that require this?

None (maybe remote tests on Android? Not sure). The MOZ_DISABLE_NONLOCAL_CONNECTIONS flag exists to make sure that we don't depend on external services in unit tests. The requirement of MOZ_DISABLE_NONLOCAL_CONNECTIONS to test non-e10s makes it impossible to manually debug a live Firefox instance with e10s off. I hit this bug when I tried to rule out process-specific issues as cause of bugs.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Rob Wu [:robwu] from comment #9)

(In reply to Dave Townsend [:mossop] from comment #8)

(In reply to Rob Wu [:robwu] from comment #4)

When MOZ_DISABLE_NONLOCAL_CONNECTIONS is set, the application crashes when outbound connections are made, which renders it useless for testing anything that requires an internet connection.

What tests do we have that require this?

None (maybe remote tests on Android? Not sure). The MOZ_DISABLE_NONLOCAL_CONNECTIONS flag exists to make sure that we don't depend on external services in unit tests. The requirement of MOZ_DISABLE_NONLOCAL_CONNECTIONS to test non-e10s makes it impossible to manually debug a live Firefox instance with e10s off. I hit this bug when I tried to rule out process-specific issues as cause of bugs.

What I'm asking is under what circumstances would you need to do that? non-e10s is known to be broken in an unknown amount of ways. The intent is to not be able to do this outside of automated tests because it is a footgun (and will likely become more so in the future).

Comment 11

[Rob Wu [:robwu]]

(In reply to Dave Townsend [:mossop] from comment #10)

(In reply to Rob Wu [:robwu] from comment #9)

The requirement of MOZ_DISABLE_NONLOCAL_CONNECTIONS to test non-e10s makes it impossible to manually debug a live Firefox instance with e10s off. I hit this bug when I tried to rule out process-specific issues as cause of bugs.

What I'm asking is under what circumstances would you need to do that? non-e10s is known to be broken in an unknown amount of ways. The intent is to not be able to do this outside of automated tests because it is a footgun (and will likely become more so in the future).

The two recent cases that I had:

• bug 1880735: a file on a ramdisk cannot be read from a content process, but from a main process. I wanted to check whether this is due to it being in a content process or something else.
• bug 1884996: Firefox crashed when launched with faketime. To get past one of the crash causes, I disabled a libfaketime feature that synchronizes state across processes. So to rule out that as a cause for the next crash, I wanted to start Firefox in single-process (non-e10s) mode to rule out process synchronization issues. Ultimately the issue was something else (https://github.com/wolfcw/libfaketime/issues/464), but having any way to disable e10s would have made it easier to narrow down options.

Comment 12

[Dave Townsend [:mossop]]

I'm inclined to say that we don't support this configuration and so allowing it to be used outside of the specific automated testing cases that we use it for currently is just asking for trouble in the future. But I think this is really Nika's call as owner of IPC.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

I think we want to continue blocking disabling e10s on shipped builds, even if they set the environment variable. non-e10s is known to be quite a broken & insecure configuration, and we have basically no tests for it.

In terms of using non-e10s for testing & local development, I'm a bit more uncertain. Doing this is a thing which I certainly used to do all of the time, but haven't reached for as a tool in quite a while. I suppose I'm not inherently opposed to having a difficult-to-enable-for-an-actual-user mechanism for turning e10s off for testing, but I don't feel strongly that it's something we need to introduce, and I don't want it available on shipping builds. Theoretically a dev could modify the BrowserTabsRemoteAutostart function and re-build (it should be relatively fast) if they really need to enable non-e10s for some local testing, though I expect it to break even more over time.

Comment 14

[Dave Townsend [:mossop]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #13)

I think we want to continue blocking disabling e10s on shipped builds, even if they set the environment variable. non-e10s is known to be quite a broken & insecure configuration, and we have basically no tests for it.

In terms of using non-e10s for testing & local development, I'm a bit more uncertain. Doing this is a thing which I certainly used to do all of the time, but haven't reached for as a tool in quite a while. I suppose I'm not inherently opposed to having a difficult-to-enable-for-an-actual-user mechanism for turning e10s off for testing, but I don't feel strongly that it's something we need to introduce, and I don't want it available on shipping builds. Theoretically a dev could modify the BrowserTabsRemoteAutostart function and re-build (it should be relatively fast) if they really need to enable non-e10s for some local testing, though I expect it to break even more over time.

How about we allow MOZ_FORCE_DISABLE_E10S to apply if MOZILLA_OFFICIAL is unset? We already use this to enable additional developer things in the main menu for developers and it works for local builds (artifact or full). But it wouldn't work for Nightly, Beta and Release builds.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

I think that'd be fine. We could also require it to be set to the version number as well like it was required to before, though that might require more tweaks to the test running code.

Comment 16

[Dave Townsend [:mossop]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #15)

I think that'd be fine. We could also require it to be set to the version number as well like it was required to before, though that might require more tweaks to the test running code.

I'm not sure that's necessary at that point. As you say if folks are building their own Firefox they could just manually change the code to turn this off anyway.

Amending this to reflect what we want. I personally don't have time to work on this but would review a patch for it.

Comment 17

[wildbydesign]

I have a working code example that I use in my own builds. I have tested this with MOZILLA_OFFICIAL set and without it being set. Both builds have been tested against this. The builds with MOZILLA_OFFICIAL set can never have e10s disabled because of the following code changes. While the builds without MOZILLA_OFFICIAL set can have e10s disabled, so long as the environment variable MOZ_FORCE_DISABLE_E10S is set to "1".

The only problem is that I have never submitted a patch for Firefox before. So I don't know if the following code is worthwhile to send as a patch or not. I am new to C++. However, the following code does work well and does exactly what this bug report intends to achieve.

Please have a look and let me know if I should proceed with a patch which would be my first ever so I would have to figure that out. Or if someone else should just submit this as a patch. Thank you for your time.

Code:

  // We are checking to ensure that MOZILLA_OFFICIAL is not set for this
  // build before proceeding to check the MOZ_FORCE_DISABLE_E10S environment
  // variable. If MOZILLA_OFFICIAL is not set and MOZ_FORCE_DISABLE_E10S
  // equals "1", we can proceed with disabling e10s.
  #if !defined(MOZILLA_OFFICIAL)
  if (gBrowserTabsRemoteAutostart) {
    const char* forceDisable = PR_GetEnv("MOZ_FORCE_DISABLE_E10S");
    if (forceDisable && !strcmp(forceDisable, "1")) {
      gBrowserTabsRemoteAutostart = false;
      status = kE10sForceDisabled;
    }
  }

  gBrowserTabsRemoteStatus = status;

  return gBrowserTabsRemoteAutostart;
  #endif

Comment 18

[Rob Wu [:robwu]]

If you'd like to submit a patch and get it reviewed, see https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

Comment 19

[wildbydesign]

(In reply to Rob Wu [:robwu] from comment #18)

If you'd like to submit a patch and get it reviewed, see https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

Thank you, Rob. Who would be the correct person to put as the reviewer?

Comment 20

[Rob Wu [:robwu]]

(In reply to wildbydesign from comment #19)

(In reply to Rob Wu [:robwu] from comment #18)

If you'd like to submit a patch and get it reviewed, see https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

Thank you, Rob. Who would be the correct person to put as the reviewer?

r=mossop would work here. He is the triage owner of this component on Bugzilla, and listed as an owner at https://firefox-source-docs.mozilla.org/mots/index.html. In comment 16 above he also expressed willingness to review a patch.

Comment 21

[wildbydesign]

Attachment: Bug 1884814 - Respect MOZ_FORCE_DISABLE_E10S if MOZILLA_OFFICIAL is not set. r=mossop

Therefore, e10s cannot be disabled via the MOZ_FORCE_DISABLE_E10S
environment variable on official Nightly, Beta and Release builds. The
check for MOZ_DISABLE_NONLOCAL_CONNECTIONS has been removed. This code
first checks to ensure that MOZILLA_OFFICIAL is not set as a contingency
before moving on to the code that checks for the MOZ_FORCE_DISABLE_E10S
environment variable. MOZ_FORCE_DISABLE_E10S must be set to "1" for e10s
to be disabled and only on non official builds.

HG: Enter commit message. Lines beginning with 'HG:' are removed.
HG: Leave message empty to abort commit.
HG: --
HG: user: WildByDesign <[e-mail]>
HG: branch 'default'
HG: changed toolkit/xre/nsAppRunner.cpp

Comment 22

[wildbydesign]

Attachment: Bug 1884814 - Respect MOZ_FORCE_DISABLE_E10S if MOZILLA_OFFICIAL is not set. r=mossop

We are checking to ensure that either MOZILLA_OFFICIAL is undefined or that
xpc::AreNonLocalConnectionsDisabled() is true. If either check matches,
we move on to check the MOZ_FORCE_DISABLE_E10S environment variable which
must equal "1" to proceed with disabling e10s.

Comment 23

[wildbydesign]

Attachment: Bug 1884814 - Respect MOZ_FORCE_DISABLE_E10S if MOZILLA_OFFICIAL is not set. r=mossop

We are checking to ensure that either MOZILLA_OFFICIAL is undefined or that
xpc::AreNonLocalConnectionsDisabled() is true. If either check matches,
we move on to check the MOZ_FORCE_DISABLE_E10S environment variable which
must equal "1" to proceed with disabling e10s.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/535ce4b77986
Respect MOZ_FORCE_DISABLE_E10S if MOZILLA_OFFICIAL is not set. r=mossop DONTBUILD

Comment 25

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/535ce4b77986