Closed Bug 1510785 Opened 6 years ago Closed 5 years ago

Exclude XBL-related bits in GeckoView/Fennec builds

Categories

(GeckoView :: General, task, P3)

Product:
GeckoView
Component:
General
Type:
task

Tracking

(geckoview66 wontfix, firefox-esr60 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70 wontfix, firefox71 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
geckoview66 --- wontfix
firefox-esr60 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: timdream, Assigned: bdahl)

References

(Blocks 3 open bugs)

Details

Attachments

(5 files, 2 obsolete files)

Bug 1510785 - Add build option for disabling XBL. r=chmanchester
(deleted), text/x-phabricator-request
Details
Bug 1510785 - Only build XBL related code when MOZ_XBL is defined. r=bzbarsky
(deleted), text/x-phabricator-request
Details
Bug 1510785 - Add way to disable XBL in servo. r=emilio
(deleted), text/x-phabricator-request
Details
Bug 1510785 - Support disabling XBL related tests. r=jmaher
(deleted), text/x-phabricator-request
Details
Bug 1510785 - Skip all XBL related tests when XBL is disabled. r=bzbarsky
(deleted), text/x-phabricator-request
Details 
With in-content XBL bindings pref'd off, we are very close to getting rid of all XBL usage in Fennec and GeckoView.

The one last remaining known usage is <xul:browser> and will be addressed in bug 1441935.

Once the XBL usage is gone, we should be able to cash out these work in terms of build size and/or performance by excluding XBL-related machinery from the build.

I don't have a concrete idea on how to approach this -- perhaps we want to |#ifndef ANDROID| everything related, or if there can be something less tedious.

I am not entirely sure how to measure the potential benefit without actually doing it either. Brian suggested stubbing the classes and measure the apk size; if that's the right avenue to approach this problem we could just do that.

I tentatively set this to depend on bug 1507895 because we don't want the browser to break if the pref is turned off. However, I don't see why we couldn't force it on on Android if we want to.

Please share your ideas!
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #0)
> With in-content XBL bindings pref'd off, we are very close to getting rid of
> all XBL usage in Fennec and GeckoView.
> 
> The one last remaining known usage is <xul:browser> and will be addressed in
> bug 1441935.
> 
> Once the XBL usage is gone, we should be able to cash out these work in
> terms of build size and/or performance by excluding XBL-related machinery
> from the build.
> 
> I don't have a concrete idea on how to approach this -- perhaps we want to
> |#ifndef ANDROID| everything related, or if there can be something less
> tedious.

I would envision a new directive `#ifdef MOZ_XBL`, since we'll eventually want to shut this off for the rest of the browser as well.
Strongly prefer MOZ_XBL to generic platform flags: it's really hard to search for generic flags, and even harder to reason about why they're the option used!
Agree, MOZ_XBL seems fine to me.
Attached file Bug 1510785 - Part 1: Add MOZ_XBL build flag. (obsolete) (deleted) — 
Defaults to MOZ_XBL=1 for !mobile/android and unset for
mobile/android.

We'll progressively enclose more and more native code in #ifdef
MOZ_XBL blocks until we can unset the flag globally and remove all of
that code.
bgrins, others -- if you'd like to do some initial experiments, a try push with those patches should help.
Flags: needinfo?(bgrinstead)
BTW, this makes remote XUL not usable on mobile.
Product: Firefox for Android → GeckoView

(In reply to Nick Alexander :nalexander [he/him] from comment #6)

bgrins, others -- if you'd like to do some initial experiments, a try push
with those patches should help.

Brendan started looking into this. It sounds like ifdefs through the tree wherever XBL service is used is the most straightforward approach so far.

Flags: needinfo?(bgrinstead)

(In reply to Brian Grinstead [:bgrins] from comment #8)

Brendan started looking into this. It sounds like ifdefs through the tree wherever XBL service is used is the most straightforward approach so far.

Note that Fennec will move to ESR 68:

https://docs.google.com/document/d/1oRPkwP3l7QzdQYj0Wn7d_3EfTZaakocA_i_pGKlG0dI/edit

There will be no Fennec 69 release or later, but we'll continue to support Fennec 68.x with ESR releases at least until mid-2020. We'll continue to ship GeckoView from mozilla-central/beta/release, but I assume GeckoView doesn't include any XBL code.

However, we will keep building and running tests on Fennec 69 and 70 until Fennec 68 ESR successfully ships to the Release channel. If there is a major problem with ESR 68, we will want Fennec 69-70 as a backup release option. So you can start ripping out Fennec code in the 71 Nightly cycle (starting 2019-09-02), but I would keep using #ifdefs until then.

status-firefox66: --- → wontfix
status-firefox67: --- → wontfix
status-firefox68: --- → fix-optional
status-firefox-esr60: --- → wontfix
status-geckoview66: --- → wontfix

(In reply to Chris Peterson [:cpeterson] from comment #9)

There will be no Fennec 69 release or later, but we'll continue to support Fennec 68.x with ESR releases at least until mid-2020. We'll continue to ship GeckoView from mozilla-central/beta/release, but I assume GeckoView doesn't include any XBL code.

GeckoView shouldn't have any XBL bindings, but all the platform code to support XBL is still there.

Type: enhancement → task

Hey bz, I wanted to get your feedback before going forward...

I have a patch working that adds a MOZ_XBL define which doesn't compile anything in the dom/xbl folder and #ifdefs XBL out of most of the places it's used in the tree. I also had to add some stubs in stylo. The gecko view tests are passing, but we have a number of other android failures because XBL is still needed there. My next steps will be to address those failures, but I first wanted to check with you and see if this is a sane approach going foward. Adding some 100+ ifdefs seems less than ideal, but it could be our first steps in removing XBL.

Flags: needinfo?(bzbarsky)

I agree that we're going to need something like this anyway for XBL removal, in the sense of having to do this audit at some point.

I don't have a problem with the number of ifdefs given that these are somewhat temporary (I know, famous last words).

Note that if we really wanted to, we could make GetAnonymousElementByAttribute completely conditional on MOZ_XBL instead of just stubbing it out ifndef MOZ_XBL. That would actually reduce the number of ifdefs a bit, I suspect, though would require preprocessing Document.webidl.

Flags: needinfo?(bzbarsky)
Blocks: 1566674
Blocks: 1566675
Depends on: 1550058
Attachment #9029505 - Attachment is obsolete: true
Attachment #9029506 - Attachment is obsolete: true
Blocks: 1580627

Defaults XBL to disabled on android, but still enabled for desktop.

When XBL is disabled, no code in dom/xbl will be built. Also, adds ifdefs
to remove any of the XBL related code elsewhere. There's definitely more
that can be done here, but I think it's better to wait to do the rest of
the cleanup when we actually remove the code.

Depends on D45612

Adds a feature "moz_xbl" that when disabled causes the XBL code in servo to
be stubbed out.

Depends on D45613

Adds a way for mochitest, reftest, and crashtests to skip XBL related
tests when XBL is disabled. Also, add an app constant so JS can
check whether XBL is enabled.

Depends on D45614

BL will be disabled on android, so these tests must be skipped.

Depends on D45615

Assignee: nobody → bdahl
Status: NEW → ASSIGNED
No longer blocks: 1566674
Depends on: 1587142
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36b84f0fb2d1
Add build option for disabling XBL. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/4a104e817b35
Only build XBL related code when MOZ_XBL is defined. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/02740878ddd6
Add way to disable XBL in servo. r=emilio
https://hg.mozilla.org/integration/autoland/rev/5a2309893b3a
Support disabling XBL related tests. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/75db6763ead5
Skip all XBL related tests when XBL is disabled. r=bzbarsky
Regressions: 1587335
Regressions: 1587347

firefox70=wontfix because we don't need to uplift these GeckoView XBL patches to Beta (70).

status-firefox68: fix-optional → wontfix
status-firefox69: --- → wontfix
status-firefox70: --- → wontfix
Regressions: 1587484
Regressions: 1587465

Hi Brendan!
Since this bug landed, artifact builds as well as windows aarch64 builds no longer have XBL enabled.
This makes try pushes using artifact builds a bit noisy, because several tests still using XBL are failing.
eg devtools/client/inspector/test/browser_inspector_highlighter-xbl.js, dom/base/test/chrome/test_bug683852.xul

Any idea how to fix this? Or should we just make sure that those tests are skipped if !xbl?

(edit: seems that skip-if = !xbl doesn't do the trick here, also some tests skipped if !xbl seem to pass on artifact builds, so not sure exactly what is wrong with artifact builds / xbl)

Flags: needinfo?(bdahl)

I think xbl is being disabled on artifact builds because of when='--enable-compile-environment' in config, but I don't understand why it's disabled for aarch64 builds. I'll be fixing this over in bug 1587662.

Flags: needinfo?(bdahl)
Regressions: 1587674
Blocks: 1587142
No longer depends on: 1587142
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: