Proposal: remove support for running desktop Firefox in single-process mode (e10s disabled) anywhere but in tests

[Gijs Kruitbosch]

(Copied to fx-dev; Replies to dev-platform please.)

Hello,

Just over a year ago, I started a discussion[0] about our support for
disabling e10s. The outcome of that was that we removed support for
disabling e10s with a pref on Desktop Firefox with version 68, except
for use from automation. We kept support for using the environment
variable. [1]

Last week, we released Firefox 77, which turned out to break all
webpages sent using compression (like gzip) if you had disabled e10s
using this environment variable. [2]

So here we are again. I'd like to propose we also stop honouring the
environment variable unless we're running tests in automation. We
clearly do not have sufficient test coverage to guarantee basic things
like "the browser works", it lacks security sandboxing, and a number of
other projects require it (fission, gpu process, socket process, ...),
so I think it's time to stop supporting this configuration at all.

I hope to make this change for the 79 cycle. I'm open to arguments
either way about what to do for 78 esr (assuming the patch for 79 turns
out to be simple; the work to remove the pref had a number of annoying
corner-cases at the time).

Please speak up if you think that this plan needs adjusting.

~ Gijs


[0]
https://groups.google.com/d/msg/mozilla.dev.platform/cJMzxi7_PmI/Pi1IOg_wCQAJ
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1548941
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1638652

[Dave Townsend]

Non-e10s is such a different environment that I don't think we have any
hope of keeping it working without running the full test suite in that mode
and I don't think anyone wants to do that. Now that this has started
breaking I think it is actively harmful to our users for us to allow them
to disable e10s.

On Wed, Jun 10, 2020 at 11:30 AM Gijs Kruitbosch <gijskru...@gmail.com>
wrote:

> _______________________________________________
> firefox-dev mailing list
> firef...@mozilla.org
> https://mail.mozilla.org/listinfo/firefox-dev
>

[Gijs Kruitbosch]

I was asked off-list why I'm not suggesting we remove support for the
environment variable entirely (ie why keep it for tests). That's a good
question, so I will attempt to address it.

I think that's a laudable goal, but it's more work. Practically
speaking, AIUI valgrind still runs with e10s disabled [0], as does gtest
[1], some xpcshell tests but not others [2], and we run some mochitest
stuff in both configurations. There might be others.

I think it's valuable to migrate all of those to e10s, or stop reading
it, but I believe it may take some time, while I also think that we
should get rid of the user footgun ASAP.

Blanket-disabling all the non-e10s tests feels like a separate decision;
I assume we are unwilling to lose valgrind coverage, some portions of
gtest, and I don't know what else would break. I'd be happy to explore
that in the 80 cycle if others agree that's worth doing, though I am
probably not the best person to try to fix our valgrind/gtest stuff...

[0] https://bugzilla.mozilla.org/show_bug.cgi?id=1549856
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1552140
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1568333

[Emilio Cobos Álvarez]

What is the situation of Thunderbird? I think they don't have e10s enabled
yet, and it may be worth at least knowing what their plans are.

-- Emilio

On Wed, Jun 10, 2020, 8:44 PM Dave Townsend <dtow...@mozilla.com> wrote:

> Non-e10s is such a different environment that I don't think we have any
> hope of keeping it working without running the full test suite in that mode
> and I don't think anyone wants to do that. Now that this has started
> breaking I think it is actively harmful to our users for us to allow them
> to disable e10s.
>
> On Wed, Jun 10, 2020 at 11:30 AM Gijs Kruitbosch <gijskru...@gmail.com
> >
> wrote:
>

> > _______________________________________________
> > firefox-dev mailing list
> > firef...@mozilla.org
> > https://mail.mozilla.org/listinfo/firefox-dev
> >

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[David Major]

I agree that it's a bad idea for users to be running permanently with this
setting on their daily driver browsers.

But the environment variable has been a huge productivity enhancer to
reduce my mental load when setting up an extra-hairy debug session or
taking system traces.

I wish we could have a way to allow this for one-off cases but not
long-term usage. Unfortunately I can't settle for a proposal like "allow it
only in debug or only in nightlies" because I often need to debug actual
user-facing builds. Is there any way we could build some auto-expiration
into this setting, like maybe you'd have to set the env var equal to the
build ID or today's date?

On Wed, Jun 10, 2020 at 2:44 PM Dave Townsend <dtow...@mozilla.com> wrote:

> Non-e10s is such a different environment that I don't think we have any
> hope of keeping it working without running the full test suite in that mode
> and I don't think anyone wants to do that. Now that this has started
> breaking I think it is actively harmful to our users for us to allow them
> to disable e10s.
>
> On Wed, Jun 10, 2020 at 11:30 AM Gijs Kruitbosch <gijskru...@gmail.com
> >
> wrote:
>

[Gijs Kruitbosch]

I can't speak for Thunderbird's plans, but either way these plans
shouldn't affect them and is restricted to desktop Firefox; the pref
still works there:
https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/toolkit/xre/nsAppRunner.cpp#5020-5024
, and they set it:
https://searchfox.org/comm-central/rev/e62a0af3cba6e1c65b2d4be02d3fefce88cf3f8f/mail/app/profile/all-thunderbird.js#654

Of course, if TB needs this configuration then that may complicate
removing support for non-e10s entirely...

~ Gijs

[tom...@gmail.com]

I agree about not shipping this to our users, but I see several needs to keep this option for developers working on Firefox:

* GeckoView still supports running in non-e10s mode, and inability to mimic that environment on desktop builds would complicate writing code that works on android.

* As David mentioned, debugging with multiple processes is still a pain, and particularly on Windows where even basic `printf` and `dump` from the child process don't show up in the console.

On Wednesday, June 10, 2020 at 8:56:47 PM UTC+2, David Major wrote:
> I agree that it's a bad idea for users to be running permanently with this
> setting on their daily driver browsers.
>
> But the environment variable has been a huge productivity enhancer to
> reduce my mental load when setting up an extra-hairy debug session or
> taking system traces.
>
> I wish we could have a way to allow this for one-off cases but not
> long-term usage. Unfortunately I can't settle for a proposal like "allow it
> only in debug or only in nightlies" because I often need to debug actual
> user-facing builds. Is there any way we could build some auto-expiration
> into this setting, like maybe you'd have to set the env var equal to the
> build ID or today's date?
>
>
>
> On Wed, Jun 10, 2020 at 2:44 PM Dave Townsend <dt***@mozilla.com> wrote:
>
> > Non-e10s is such a different environment that I don't think we have any
> > hope of keeping it working without running the full test suite in that mode
> > and I don't think anyone wants to do that. Now that this has started
> > breaking I think it is actively harmful to our users for us to allow them
> > to disable e10s.
> >

> > On Wed, Jun 10, 2020 at 11:30 AM Gijs Kruitbosch <gi***@gmail.com

[James Teh]

In general, this obviously makes a lot of sense. However, because there is
so much extra complication for accessibility when e10s is enabled, I find
myself disabling e10s in local opt/debug builds to isolate problems to the
core a11y engine (vs the a11y e10s stuff). The ability to do this was
instrumental in some of the perf work I've been doing lately. For example,
it allowed me to determine that some of the perf problems I had originally
attributed to the a11y e10s layer were actually problems in the core a11y
engine. I'm sure there's some way I could have achieved that with e10s
enabled, but it probably would have taken me weeks longer.

All of that said, I realise this is an obscure case and I don't want to
stand in the way of progress; I'm well aware legacy has to die eventually.
Nevertheless, I thought I'd at least flag this concern.

Btw, the need to isolate the core a11y engine from the a11y e10s stuff is
also why some of our a11y tests still run with e10s disabled in automation.

Jamie

On Thu, Jun 11, 2020 at 4:56 AM David Major <dma...@mozilla.com> wrote:

> I agree that it's a bad idea for users to be running permanently with this
> setting on their daily driver browsers.
>
> But the environment variable has been a huge productivity enhancer to
> reduce my mental load when setting up an extra-hairy debug session or
> taking system traces.
>
> I wish we could have a way to allow this for one-off cases but not
> long-term usage. Unfortunately I can't settle for a proposal like "allow it
> only in debug or only in nightlies" because I often need to debug actual
> user-facing builds. Is there any way we could build some auto-expiration
> into this setting, like maybe you'd have to set the env var equal to the
> build ID or today's date?
>
>
>

> On Wed, Jun 10, 2020 at 2:44 PM Dave Townsend <dtow...@mozilla.com>

> wrote:
>
> > Non-e10s is such a different environment that I don't think we have any
> > hope of keeping it working without running the full test suite in that
> mode
> > and I don't think anyone wants to do that. Now that this has started
> > breaking I think it is actively harmful to our users for us to allow them
> > to disable e10s.
> >
> > On Wed, Jun 10, 2020 at 11:30 AM Gijs Kruitbosch <

> gijskru...@gmail.com

[ISHIKAWA,chiaki]

I can't speak for what TB development plan is.

One thing I observe as an occasional submitter of TB patches is this.
Thunderbird ditched |mozmill| test December 2019, and switched to
mochitest in place of mozmill test.
Unfortunately, valgrind no longer works locally for mochitest.
This is quite a loss for keeping the code in sane state. I have
uncovered quite a few memory-related bugs over the years by running TB
under valgrind during mozmill-based test.
(I don't know if there is any official valgrind run of TB on tryserver
despite arcane description I read years ago that
it was performed from time to time.).
xpcshell-tests still runs under valgrind. I found a few memory-related
errors by running TB under valgrind during xpcshell-tests this year.

So for me, valgrind test is very important for keeping TB in sane state
after a large patch set lands, etc.
(Actually, I have a large patch set that enables buffered-write for
message handling and this will boost write performance very much. But it
is a rather lage patch set, and I want to run tests under valgrind to
make sure nothing undesirable is introduced.)

Looking at the posts, I have a feeling that TB may be able to run under
valgrind during mochitest if this
e10s setting is properly handled.  Right now, TB under valgrind during
mochitest starts more than 1500 threads (too many IMHO), but less than
5000 threads (I have not figured out exactly how many threads are
required), and valgrind barfs. I need to increase the number of threads
valgrind supports,
but maybe because of the large number of threads, tests time out before
anything useful is performed. Thus no useful memory test is done under
mochitest currently.

Something is wrong there.

I suspect this e10s setting *may* have a bit to do with the situation.
I think I have to make sure TB runs in non e10s mode to run under
valgrind during mochitest.
Specifically I am not sure if the setting is honored in test user
profile prepared for  mochitest.:
https://searchfox.org/comm-central/rev/e62a0af3cba6e1c65b2d4be02d3fefce88cf3f8f/mail/app/profile/all-thunderbird.js#654

||
|// No e10s in Thunderbird for now. |
|pref("browser.tabs.remote.autostart", false); |
||
*Maybe*, by making sure that this is set to false, valgrind might work
under mochitest.
Maybe a fishing trip in the coming weekend.

Chiaki

[Aaron Klotz]

On 6/10/2020 2:09 PM, tom...@gmail.com wrote:
> * GeckoView still supports running in non-e10s mode, and inability to mimic that environment on desktop builds would complicate writing code that works on android.
>

GeckoView's only technical reason for supporting non-e10s was FxR, but
they have since switched to e10s we can (and want to) deprecate GV
support for non-e10s.

- Aaron

[Henri Sivonen]

On Wed, Jun 10, 2020 at 11:13 PM James Teh <jt...@mozilla.com> wrote:
> In general, this obviously makes a lot of sense. However, because there is
> so much extra complication for accessibility when e10s is enabled, I find
> myself disabling e10s in local opt/debug builds to isolate problems to the
> core a11y engine (vs the a11y e10s stuff).

This is also relevant to other debugging scenarios, especially when
not being able to use Pernosco to search for the right process.

What does this proposal mean for ./mach run --disable-e10s ?

--
Henri Sivonen
hsiv...@mozilla.com

[Gijs Kruitbosch]

Having read all the responses here, I guess an adjusted proposal would
be to switch to requiring the variable to be set to the build's version.
Does that seem like it'd address your concerns?

There were two other points raised that I wanted to briefly respond to:

> What does this proposal mean for ./mach run --disable-e10s ?

In the adjusted proposal, this would set the right env var, as it does
today, so there should be no difference.

> * particularly on Windows where even basic `printf` and `dump` from the child process don't show up in the console.

As I have suggested to you elsewhere, I think this is a serious problem
that inhibits adoption of Windows as a development platform for Firefox,
and we should address this orthogonal to whatever we do with e10s.
Inasmuch as this isn't already covered in
https://bugzilla.mozilla.org/show_bug.cgi?id=1257155, please file bugs.

Note that you do not actually need to disable e10s, running with the
`-attach-console` commandline switch (which you need anyway, also if you
disable e10s) and the `MOZ_DISABLE_CONTENT_SANDBOX=1` env var are
sufficient to get dump logging from the content process in my testing.


Longer-term, it would be useful to think about how else we could cater
to your debugging needs, without completely disabling e10s - whether
that's something like the layout debugger (which was switched to using
e10s a while back) to reduce the noise of the rest of Firefox, or a way
to directly inspect the a11y API output of the content process, or
whatever it is - it is not ultimately feasible to keep "supporting"
several modes of operation forever.

~ Gijs

[Magnus Melin]

Currently Thunderbird doesn't work with e10s.

Longer term I'm assuming we'll need to do necessary adaptions so that we
can - but I suspect this is a slightly larger project...

I've filed bug 1646648 to track this work.

-Magnus

[ISHIKAWA,chiaki]

I think I have found an inconsistency in TB's pref setting during
mochitest regarding e10s.
The gory detail is in
https://bugzilla.mozilla.org/show_bug.cgi?id=1629433#c10

Major find (excerpt from the above bugzilla.)

--- begin quote ---
1) Dynamically generated user.js files during tests of C-C TB mochitest
execution, contain conflicting stetting.

pref("browser.tabs.remote.autostart", {true|false});

The value is either true or false. Not consistent. In mochitest of C-C
TB, there may be a few very browser-like test. In those cases,
|browser.tabs.remote.autostart| can be true? However, if we ALWAYS need
to set the value for false for C-C TB mochitest,

    >> // No e10s in Thunderbird for now.

    >> pref("browser.tabs.remote.autostart", false); <=== This

we may be in a big trouble.

I observe that there are multiple lines to set
"browser.tabs.remote.autostart" in a user.js with different
setting. Of course, the last one sets the used value. Inquiring mind
wants to know how the setting is composed (presumably concatenate some
settings prepackaged in user.js files in test file directories? I have
noticed that there are user.js files in test directory. See (3)
below.) Clear description of how the run-time user.js is created for
each test during c-c TB mochitest run would be desirable.

In any case, I DID find cases where sub-tests (albeit web
browser-oriented?) run with a user.js with
|pref("browser.tabs.remote.autostart", true);| which may not sit well
for C-C thunderbird binary.

I have a nagging feeling that this bad setting may/could be one of the
causes of the failure to run valgrind because we need more than 1500+
threads to run the C-C TB mochitest(!)

(2) user.js file under MOZ_OBJ/temp,
/NEW-SSD/moz-obj-dir/objdir-tb3/temp/user.js, does NOT set
|pref("browser.tabs.remote.autostart", false);|

I wonder if this user.js in MOZ_OBJ directory is used or not during C-C
mochitest. If it is used, I think we are in a big trouble since it does
not set the value to false.

(3) There are a few |user.js| files with conflicting settings in the
test directories. Some set the pref to false, but some set to true.
I hope they are used appropriately. Since the C-C mochitest works more
or less, I believe they are used correctly, but a clear
documentation/explanation of how each is used and how each test creates
user.js dynamically from the prepackaged user.js for TB
mochitest would be desirable.
--- end quote ---


So if the testing of web browser function in C-C TB mochitest runs with
e10s enabled, then it is not testing the real world usage because
e10s is disabled for TB usually.
(OTOH, it is hard to believe that if TB needs to disable e10s, the
built-in web renderer in TB can run with e10s enabled without disrupting
the behavior of the rest of the code. Something is screwed up in
dynamically generated mochitest user.js )

I hope someone familiar with C-C TB mochitest investigates this and
figures if the current state of the affairs is correct or not.

Chiaki

[Dave Townsend]

Since there were no further concerns voiced to the final proposal I've gone
ahead and landed the change in
https://bugzilla.mozilla.org/show_bug.cgi?id=1653384. To be respected you
must now set
the MOZ_FORCE_DISABLE_E10S environment variable to the full application
version. `mach run --disable-e10s` has been updated to do this
automatically.

On Wed, Jun 17, 2020 at 1:45 PM Gijs Kruitbosch <gijskru...@gmail.com>
wrote: