"Enable E10S (multi-process)" preference checkbox should not be displayed in Aurora's about:preferences (E10S_TESTING_ONLY somehow defined for Aurora builds)

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: mconley)

Tracking

unspecified
Firefox 36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox35 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
STR:
1. In Aurora 35, open about:preferences

RESULT:
Witness the "Enable E10S (multi-process)" checkbox which should only be shown on the Nightly channel.

Possibly related issues:

* The configure.in check for NIGHTLY_BUILD looks backwards because it is sets E10S_TESTING_ONLY=1 if NIGHTLY_BUILD is not defined:

https://hg.mozilla.org/mozilla-central/rev/7d04d18f5eeb#l8.16

* browser/base/moz.build may be defining inadvertently "defining" DEFINES['E10S_TESTING_ONLY'] with a zero value from CONFIG['E10S_TESTING_ONLY']. #ifdef will be true, even if the macro is defined even if the macro *value* is zero.

https://hg.mozilla.org/mozilla-central/rev/7d04d18f5eeb#l5.12
From just-built objdirs from Aurora/Nightly:

[gavin@gavin-mbp:~/mozilla/aurora]$ ag NIGHTLY obj-ff-opt/config.status
354:    (''' NIGHTLY_BUILD ''', r'''  '''),

(from "defines")

[gavin@gavin-mbp:~/mozilla/mozilla-central]$ ag NIGHTLY obj-ff-opt/config.status
74:    (''' NIGHTLY_BUILD ''', ' 1 '),
358:    (''' NIGHTLY_BUILD ''', r''' 1 '''),

from ("defines" and "substs", respectively)
Reporter

Comment 2

5 years ago
This is not a high priority issue because about:preferences because non-Nightly channels still default to the Preferences dialog (which does not have this problem).
Isn't E10S_TESTING_ONLY used in other places in similar ways? Those might be more problematic.
Assignee: nobody → mconley
I'm seeing the "Open e10s window" toolbar buttons in Aurora, which seems to suggest that E10S_TESTING_ONLY is busted.
I'm not, thankfully, seeing it in Beta.
The bigger mystery then is, since it shows up on Aurora, why doesn't it show up in Beta/Release?
Summary: "Enable E10S (multi-process)" preference checkbox should not be displayed in Aurora's about:preferences → "Enable E10S (multi-process)" preference checkbox should not be displayed in Aurora's about:preferences (E10S_TESTING_ONLY somehow defined for Aurora builds)
We were defining the E10S_TESTING_ONLY build-time define with the value of the E10S_TESTING_ONLY config variable,
regardless of the value of E10S_TESTING_ONLY. Even if E10S_TESTING_ONLY in configure was blank, we'd still define
E10S_TESTING_ONLY, which resulted in E10S_TESTING_ONLY ifdef'd code to be included when we didn't want it to.

This also fixes a busted E10S_TESTING_ONLY ifdef that didn't have an endif.
Comment on attachment 8516216 [details] [diff] [review]
Fix E10S_TESTING_ONLY define so that it only ever applies when E10S_TESTING_ONLY config is true. r=?

So the E10S_TESTING_ONLY build-time config and define are both pretty busted.

Here's what went wrong:

We were using test -z on $NIGHTLY_BUILD to set the E10S_TESTING_ONLY to 1. With test -z, this roughly translates to:

"If NIGHTLY_BUILD is not set, then set E10S_TESTING_ONLY=1."

That's wrong - we wanted the relationship to be that if NIGHTLY_BUILD *is* set, then E10S_TESTING_ONLY=1.

Secondly, it looks like regardless of what value E10S_TESTING_ONLY had, we were always defining it. In our moz.build files, we had code like this:

DEFINES['E10S_TESTING_ONLY'] = CONFIG['E10S_TESTING_ONLY']

Even if E10S_TESTING_ONLY was set to ' ', which seems to be the case if NIGHTLY_BUILD is not set, then we'd still define E10S_TESTING_ONLY.

Meaning that E10S_TESTING_ONLY was *always* defined.

I've switched it so that the E10S_TESTING_ONLY define is only ever set in a moz.build if CONFIG['E10S_TESTING_ONLY'] is truthy - otherwise, it remains undefined.

I also fixed a case in tabbrowser.xml where we had no #endif for an #ifdef E10S_TESTING_ONLY, which resulted in a busted browser when I first fixed this.

Kinda bad news all around. Luckily, the tabbrowser.xml stuff hasn't reached beta yet - it's only on Aurora and Central.

It's pretty mysterious why the e10s stuff isn't visible in beta or release. I honestly have no idea. Perhaps the build rules are drastically different - I'm really not sure.

Anyhow, this patch applied on central still shows the e10s testing UI. Applied to Aurora (after manually applying) gets rid of the e10s testing UI.

r?ing glandium for the configure.in change, ally for everything else.
Attachment #8516216 - Flags: review?(mh+mozilla)
Attachment #8516216 - Flags: review?(ally)
We should probably AC_DEFINE this variable so that we don't need to explicitly add it to DEFINES in a whole bunch of separate moz.build files.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #9)
> We should probably AC_DEFINE this variable so that we don't need to
> explicitly add it to DEFINES in a whole bunch of separate moz.build files.

Oh, yeah - thanks, I wanted to ask about that.

I agree with gavin that this probably makes the most sense - and I'm curious about why we didn't do it that way in the first place.

Incidentally, bug 1004533 (which introduced E10S_TESTING_ONLY) actually discussed this DEFINE[foo] = CONFIG[foo], with nalexander suggesting the conditional: https://bugzilla.mozilla.org/show_bug.cgi?id=1004533#c6

glandium - can you think of any objections to just using AC_DEFINE in configure.in? It'd be temporary - likely until e10s ships.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8516216 [details] [diff] [review]
Fix E10S_TESTING_ONLY define so that it only ever applies when E10S_TESTING_ONLY config is true. r=?

Review of attachment 8516216 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +3546,5 @@
>  dnl ========================================================
>  dnl Multiprocess Firefox Nightly Testing UI
>  dnl To be removed in Bug 1003313
>  dnl ========================================================
> +if test -n "$NIGHTLY_BUILD"; then

Seems to me at least this should be backported.
Attachment #8516216 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8516216 [details] [diff] [review]
Fix E10S_TESTING_ONLY define so that it only ever applies when E10S_TESTING_ONLY config is true. r=?

Review of attachment 8516216 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds like AC_DEFINES is the way to go. So let's see that patch. :)

As for why it wasn't used in the original place, because no one from #build bothered to suggest it to the author.
Attachment #8516216 - Flags: review?(ally)
Posted patch Patch v2Splinter Review
Alright, let's go the AC_DEFINE route.

Once this gets r+'d, the backporting to mozilla-aurora will be straight-forward.
Attachment #8516216 - Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)
Attachment #8516759 - Flags: review?(mh+mozilla)
Attachment #8516759 - Flags: review?(ally)
Comment on attachment 8516759 [details] [diff] [review]
Patch v2

Review of attachment 8516759 [details] [diff] [review]:
-----------------------------------------------------------------

r+ w/nit

::: browser/base/moz.build
@@ +23,5 @@
>      'content/test/social/browser.ini',
>  ]
>  
>  DEFINES['MOZ_APP_VERSION'] = CONFIG['MOZ_APP_VERSION']
> +

nit: whitespace?
Attachment #8516759 - Flags: review?(ally) → review+
Reporter

Comment 15

5 years ago
This bug also affects Firefox Developer Edition, so we should try to land a fix this week. :)
Attachment #8516759 - Flags: review?(mh+mozilla) → review+
Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/032d2db91f36
Whiteboard: [fixed-in-fx-team]
We were defining the E10S_TESTING_ONLY build-time define with the value of the E10S_TESTING_ONLY config variable,
regardless of the value of E10S_TESTING_ONLY. Even if E10S_TESTING_ONLY in configure was blank, we'd still define
E10S_TESTING_ONLY, which resulted in E10S_TESTING_ONLY ifdef'd code to be included when we didn't want it to.

We now only ever define E10S_TESTING_ONLY iff NIGHTLY_BUILD is defined, and we do it globally.

This also fixes a busted E10S_TESTING_ONLY ifdef that didn't have an endif.
Comment on attachment 8517187 [details] [diff] [review]
mozilla-aurora backport

I'll wait for the other patch to merge to central before requesting approval.
Attachment #8517187 - Attachment description: Fix E10S_TESTING_ONLY define so that it only ever applies when E10S_TESTING_ONLY config is true. a=? → mozilla-aurora backport
Flags: needinfo?(mconley)
Comment on attachment 8516759 [details] [diff] [review]
Patch v2

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+#ifdef E10S_TESTING_ONLY
>     gRemoteTabsUI.init();
>+#endif

There are two of these calls in browser.js - you should probably ifdef both? (I assume this is not a crucial change.)
Yikes - sorry I missed that. I'll ifdef the other one in a follow-up.
Flags: needinfo?(mconley)
Assignee

Updated

5 years ago
Attachment #8517187 - Attachment is obsolete: true
Landed follow-up:

remote:   https://hg.mozilla.org/integration/fx-team/rev/c41cd725fd1e

I'll fold that into my Aurora backport patch and post a new one.
Comment on attachment 8517187 [details] [diff] [review]
mozilla-aurora backport

Huh, there's only a single call to gRemoteTabsUI.init in mozilla-aurora.

Again, needinfo'ing myself to request approval once the other patch merges to central.
Attachment #8517187 - Attachment is obsolete: false
Flags: needinfo?(mconley)
Attachment #8517187 - Flags: approval-mozilla-aurora+
Alright, eff it, why wait - I've landed on mozilla-aurora:

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e4a43029e627
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> Alright, eff it, why wait - I've landed on mozilla-aurora:

Because standard practice says to?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> > Alright, eff it, why wait - I've landed on mozilla-aurora:
> 
> Because standard practice says to?

Hrm - I figured gavin was signaling me to jump the gun because of that approval. Sorry if that was a faulty assumption.

I'll keep my eye on Aurora, and if you feel strongly about it, I can back out if we want to wait for baking on central.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> > Alright, eff it, why wait - I've landed on mozilla-aurora:
> 
> Because standard practice says to?

"Standard practice" is "use your judgement", and that's what we did here. No harm.
https://hg.mozilla.org/mozilla-central/rev/032d2db91f36
https://hg.mozilla.org/mozilla-central/rev/c41cd725fd1e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
It's good to have rules to clarify expected behavior in the common case, or to establish a baseline when relevant judgement is lacking, but it's also fine to break rules when there's no downside to doing so.
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.