It seems that the FxA feature might not be included in FxOS 1.4, so we need a build time flag to enable/disable the FxA related code to avoid shipping unused code.

ferjm - Thought I'd take this opportunity to learn a bit about gecko optimization. Do we track the size of the shipping codebase somewhere (maybe contrasted with the available size on our lower-powered handsets)? I'm curious what kind of savings is introduced by preffing off the FxA code. Or is this really just code cleanliness, disabling everything we possibly can?

AFAIK we don't have public memory reports yet (bug 962238 will do that). But we can get information about how much memory is consumed in the device via [1]. The idea here is to be able to remove as much FxA code as possible mostly from B2G without backing out anything. We have some code [2] running in the main process that won't ever be used if FxA is not finally part of 1.4. It might not seem that much, but every byte counts in B2G. We will also save some ROM space removing the FxA code from [3]. [1] https://github.com/mozilla-b2g/B2G/blob/master/tools/get_about_memory.py [2] https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#26 [3] https://mxr.mozilla.org/mozilla-central/source/b2g/installer/package-manifest.in#793

This patch should disable/remove FxAccounts code from B2G and pref off FxA related preferences in desktop and Android.

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #3) > Created attachment 8381434 [details] [diff] [review] > Untested WIP > > This patch should disable/remove FxAccounts code from B2G and pref off FxA > related preferences in desktop and Android. ... by adding the --disable-fxaccounts option in the .mozconfig

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #2) > AFAIK we don't have public memory reports yet (bug 962238 will do that). But > we can get information about how much memory is consumed in the device via > [1]. Awesome! Thanks for the info. CC'd myself on that bug. I'll poke at [1] when I get a chance :-) > > The idea here is to be able to remove as much FxA code as possible mostly > from B2G without backing out anything. We have some code [2] running in the > main process that won't ever be used if FxA is not finally part of 1.4. It > might not seem that much, but every byte counts in B2G. We will also save > some ROM space removing the FxA code from [3]. > > [1] https://github.com/mozilla-b2g/B2G/blob/master/tools/get_about_memory.py > [2] > https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#26 > [3] > https://mxr.mozilla.org/mozilla-central/source/b2g/installer/package- > manifest.in#793

This patch should disable/remove FxAccounts code from B2G and pref off sync via FxAccounts in desktop and android by adding --disable-fxaccounts in the .mozconfig

I'm not sure we want the --disable-fxaccounts flag accessible in configure. Using the confvars.sh switch looks good enough to me.

Comment on attachment 8382266 [details] [diff] [review] v1 Looks good in general, although on desktop I think we would also want tie MOZ_SERVICES_SYNC to MOZ_FXACCOUNTS - otherwise we would end up in a state where desktop is re-enabling parts of sync that can't currently be reached, possibly no longer work, and are likely to be removed eventually. IOW, desktop is (slowly) moving to a state where sync simply will not work without FxA. Gavin, do you agree? Also, I think services/moz.build needs to be touched by this patch - PARALLEL_DIRS would only include 'fxaccounts' if this new define is set (and sync would also then be excluded by ensuring MOZ_SERVICES_SYNC was also undefined, which that file already checks) Another alternative would be to have this only as a b2g feature - that desktop ignores the new define - in which case the define having B2G in it might make more sense. I'm not too bothered either way, so f+ as the approach seems reasonable even if it needs some tweaks.

(In reply to Mark Hammond [:markh] from comment #8) > IOW, desktop is (slowly) moving to a state where sync simply will not work > without FxA. > > Gavin, do you agree? Yes. I don't really think we need to worry about what would happen if someone builds Firefox with this flag to disable, though, since no one should ever do that.

Thanks for the feedback Fabrice, Mark and Gavin! I changed the patch to add a MOZ_B2G_FXACCOUNTS var that affects B2G only and got rid of the configure option.

Comment on attachment 8383131 [details] [diff] [review] v1 Expanding the review request to other build config peer.

Gregory, will you have time to review this in the next 24 hours or so, or should we hunt for someone else? We need to land this in the next few days. Thanks so much for your time.

Comment on attachment 8383131 [details] [diff] [review] v1 Review of attachment 8383131 [details] [diff] [review]: ----------------------------------------------------------------- It appears services/fxaccounts didn't land behind a feature flag (likely would have been MOZ_SERVICES_FXACCOUNTS). Boo. Anyway, MOZ_B2G_FXACCOUNTS is a bad name as Firefox Accounts isn't a b2g-specific feature. This should be MOZ_FXACCOUNTS or MOZ_SERVICES_FXACCOUNTS (for consistency with other service-oriented features). You'll likely get r+ with the name change. ::: b2g/app/b2g.js @@ +892,5 @@ > + > +// Disable sync and mozId with Firefox Accounts. > +#ifndef MOZ_B2G_FXACCOUNTS > +pref("services.sync.fxaccounts.enabled", false); > +pref("identity.fxaccounts.enabled", false); If the feature isn't shipping with the code base, what's there to disable? Things typically work the opposite way: #ifdef MOZ_FEATURE pref('feature.enabled', true); #endif

(In reply to Gregory Szorc [:gps] from comment #13) > Comment on attachment 8383131 [details] [diff] [review] > v1 > > Review of attachment 8383131 [details] [diff] [review]: > ----------------------------------------------------------------- > > It appears services/fxaccounts didn't land behind a feature flag (likely > would have been MOZ_SERVICES_FXACCOUNTS). Boo. It landed long before the feature flag request was made. It's used in: services/sync/tps/extensions/tps/modules/fxaccounts.jsm which I *think* is part of desktop. > > Anyway, MOZ_B2G_FXACCOUNTS is a bad name as Firefox Accounts isn't a > b2g-specific feature. This should be MOZ_FXACCOUNTS or > MOZ_SERVICES_FXACCOUNTS (for consistency with other service-oriented > features). > > You'll likely get r+ with the name change. Great! Per previous comment, we've had kind of a tangled history, and the purpose of the flag is to hide FXAccounts from B2G as opposed to desktop and Android. But I'll defer to your naming knowledge. > > ::: b2g/app/b2g.js > @@ +892,5 @@ > > + > > +// Disable sync and mozId with Firefox Accounts. > > +#ifndef MOZ_B2G_FXACCOUNTS > > +pref("services.sync.fxaccounts.enabled", false); > > +pref("identity.fxaccounts.enabled", false); > > If the feature isn't shipping with the code base, what's there to disable? > Things typically work the opposite way: > > #ifdef MOZ_FEATURE > pref('feature.enabled', true); > #endif Makes perfect sense. Thanks for the feedback!

The policy that we try to follow is that major new features land behind feature flags and individual applications opt in to those features when they are fully ready. That's why today we have "MOZ_SERVICES_SYNC=1" in browser/confvars.sh. We try not to "hide" a feature like FXAccounts from B2G. Instead, it's preferred to "expose" it to B2G via an opt-in flag. The fact Firefox Accounts landed globally on all applications without a feature flag is kinda bad. That pattern leads to things like Firefox OS shipping a lot of code and loading a lot of services (increasing memory) that it doesn't need. That's obviously not good.

I changed the flag name to MOZ_SERVICES_FXACCOUNTS, and reversed the boolean test in b/b2g/app/b2g.js per your feedback. Meanwhile I will investigate hiding services/fxaccounts behind the flag, but I'm in a bit over my head on that one and I'm hoping not to hold up these changes while I pursue guidance. Thanks so much for your time.

Previous comment applies; I also think I correctly disabled services/fxaccounts. Thanks so much!

Comment on attachment 8390845 [details] [diff] [review] bug976002.fxa.buildflag.patch Review of attachment 8390845 [details] [diff] [review]: ----------------------------------------------------------------- This patch will inadvertently disable Fx Accounts on browser, mobile/android, and any other application. You'll need to give those directories the same treatment you gave to confvars.sh and package-manifest.in. I'm not 100% certain whether mobile/android needs this code. rnewman can answer that. (This is another why we make things opt in: it forces us to ask questions and make more informed decisions.) ::: b2g/components/moz.build @@ +48,5 @@ > + > +if CONFIG['MOZ_SERVICES_FXACCOUNTS']: > + EXTRA_JS_MODULES += [ > + 'FxAccountsMgmtService.jsm' > + ] You should combine the two added ifs into a single logical unit.

(In reply to Gregory Szorc [:gps] from comment #18) > I'm not 100% certain whether mobile/android needs this code. rnewman can > answer that. It would need a totally different patch (e.g., touching Android manifest fragments). The codebase is entirely different, and currently 100%-baked-in.

Thanks for the review. I unified the conditionals in moz.build and added MOZ_SERVICES_FXACCOUNTS=1 to browser/confvars.sh. For browser/installer/package-manifest.in I'm not sure what to do. services/fxaccounts is not currently listed in it, but is being used by browser builds. I could add it inside an ifdef, but neither services/sync/SyncComponents.manifest nor services/crypto/cryptoComponents.manifest looks obviously like the right model. Searching on package-manifest.in leads to high-level XPCOM documentation. Can you suggest the best path forward, given the goal of saving the memory usage of services/fxaccounts on b2g by code freeze on Monday? ps I'm interpreting Richard's remarks as saying "this patch won't break sync on Android." If I'm mistaken, please let me know.

(In reply to Sam Penrose from comment #20) > ps I'm interpreting Richard's remarks as saying "this patch won't break sync > on Android." If I'm mistaken, please let me know. Correct. (If you want to be able to disable FxA/Sync on Android, please file another bug!)

Oh, right. services/ isn't inside a jar.mn, so we don't need the package-manifest.in foo unless XPCOM manifests are involved. Forgot about that. (We need to fix that. And if we had more resources on the build system, we'd likely move package-manifest.in metadata into moz.build somehow.)

Comment on attachment 8391020 [details] [diff] [review] bug976002.fxa.buildflag.patch Per IRC moving this to r?. Thanks so much for your patience in shepherding your wayward colleague.

Comment on attachment 8391020 [details] [diff] [review] bug976002.fxa.buildflag.patch Review of attachment 8391020 [details] [diff] [review]: ----------------------------------------------------------------- There might be a missing #ifdef MOZ_SERVICES_FXACCOUNTS in a place or two under browser/. But it shouldn't matter too much. This packaging stuff is needlessly complicated and nearly everybody gets it wrong the first 5 or 6 times.

Backed out for Android xpcshell failures. https://hg.mozilla.org/integration/fx-team/rev/72020c045912 https://tbpl.mozilla.org/php/getParsedLog.php?id=36216006&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=36215938&tree=Fx-Team

I rebuilt with the latest m-c and am not seeing this on b2g. Richard, may I throw myself on your mercy for guidance on debugging this Android build failure? The log is quite terse: 15:58:54 WARNING - TEST-UNEXPECTED-FAIL | /builds/panda-0446/test/build/tests/xpcshell/tests/services/common/tests/unit/test_hawkclient.js | test failed (with xpcshell return code: 3), see following log: 15:58:54 INFO - >>>>>>> 15:58:54 INFO - xpcw: cd /mnt/sdcard/tests/xpcshell/services/common/tests/unit 15:58:54 INFO - xpcw: xpcshell -r /mnt/sdcard/tests/xpcshell/c/httpd.manifest --greomni /data/local/xpcb/fennec-30.0a1.en-US.android-arm.apk -m -n -s -e const _HTTPD_JS_PATH = "/mnt/sdcard/tests/xpcshell/c/httpd.js"; -e const _HEAD_JS_PATH = "/mnt/sdcard/tests/xpcshell/head.js"; -e const _TESTING_MODULES_DIR = "/mnt/sdcard/tests/xpcshell/m"; -f /mnt/sdcard/tests/xpcshell/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/mnt/sdcard/tests/xpcshell/services/common/tests/unit/head_global.js", "/mnt/sdcard/tests/xpcshell/services/common/tests/unit/head_helpers.js", "/mnt/sdcard/tests/xpcshell/services/common/tests/unit/head_http.js"]; -e const _TAIL_FILES = []; -e const _TEST_FILE = ["test_hawkclient.js"]; -e _execute_test(); quit(0); 15:58:54 INFO - System JS : ERROR /mnt/sdcard/tests/xpcshell/head.js:199 - TypeError: stack is undefined 15:58:54 INFO - \x00 15:58:54 INFO - <<<<<<<

It's probably failing because we're running e.g., common/test_hawkclient.js on Android, but you're omitting the entire fxaccounts directory in the build, and there's some kind of hidden dependency there. The topmost error is that we're getting a null stack when handling an exception, which implies something dodgy going on like a failure to import a module. Two options: * Don't run FxA-related tests -- including HAWK -- on Android: kick that can down the road. * Find out what the dependency is, and fix it. If I have time tonight I can try to run these xpcshell tests on a device, see what's going on. No promises, though!

Let's just package FxAccounts.jsm on Android and sort this all out in a follow-up. It's clear that the FxA feature is already somewhat busted w.r.t. packaging standards. I don't see another bending of the rules breaking the camel's back.

I will not be able to build this soon, and tryme builds have been taking a very long time to get run. Ryan, since this patch's job is to prevent other patches from running, does it make sense to just add the one-line edit that should fix the breakage that caused the back-out, and check that in? Without some form of this patch we are unstable. Edit coming in just a minute.

I tweaked the patch to always turn FxA on for Android, per comment thread. I expect this will fix the test failure that caused the backout.

Checking in seems like the best road forward, so in the absence of feedback I am going for it.

Requesting blocking-1.4 to save memory and possible bugs, as the FxA UI will not be in 1.4.

I pushed a tweak to the comment in configure.in, since the code behind the flag isn't b2g-specific: https://hg.mozilla.org/integration/fx-team/rev/ee6b216ef4d9