As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 794228 - addon-manager services are running in B2G
: addon-manager services are running in B2G
Status: RESOLVED FIXED
[MemShrink][slim:1MB]
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: P1 normal (vote)
: ---
Assigned To: [:fabrice] Fabrice Desré
:
:
Mentors:
Depends on:
Blocks: slim-fast
  Show dependency treegraph
 
Reported: 2012-09-25 13:44 PDT by Dietrich Ayala (:dietrich)
Modified: 2012-10-16 17:49 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
patch (4.03 KB, patch)
2012-10-10 18:35 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
patch v2 (5.03 KB, patch)
2012-10-10 18:40 PDT, [:fabrice] Fabrice Desré
mh+mozilla: review+
Details | Diff | Splinter Review
patch v3 (5.07 KB, patch)
2012-10-12 13:39 PDT, [:fabrice] Fabrice Desré
mh+mozilla: review+
Details | Diff | Splinter Review

Description User image Dietrich Ayala (:dietrich) 2012-09-25 13:44:06 PDT
non-zero performance and memory cost. also making network traffic daily at least, which costs the user money.
Comment 1 User image Dietrich Ayala (:dietrich) 2012-09-25 13:44:49 PDT
requesting blocking for at least turning off network traffic, if for some reason we actually need the services running.
Comment 2 User image Justin Lebar (not reading bugmail) 2012-10-06 19:06:06 PDT
FWIW the memory usage here is ~0.3mb, at least at startup.  (Maybe the compartments grow over time.)
Comment 3 User image [:fabrice] Fabrice Desré 2012-10-09 17:46:36 PDT
(In reply to Justin Lebar [:jlebar] from comment #2)
> FWIW the memory usage here is ~0.3mb, at least at startup.  (Maybe the
> compartments grow over time.)

Justin, how do you get this number? I like to reproduce to see if my fix works.
Comment 4 User image Justin Lebar (not reading bugmail) 2012-10-10 07:16:16 PDT
(In reply to Fabrice Desré [:fabrice] from comment #3)
> (In reply to Justin Lebar [:jlebar] from comment #2)
> > FWIW the memory usage here is ~0.3mb, at least at startup.  (Maybe the
> > compartments grow over time.)
> 
> Justin, how do you get this number? I like to reproduce to see if my fix
> works.

Update your b2g repository to latest (it's not updated by ./repo sync).  On the device, run ./get-about-memory.py.  It will pull a bunch of files.  gunzip merged-reports.gz.  In Firefox nightly on desktop, open about:memory?verbose and scroll down to the bottom of the page.  Select your file and see if the add-on manager compartment is present.
Comment 5 User image [:fabrice] Fabrice Desré 2012-10-10 13:42:22 PDT
http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#490 already disables update pings. So I don't think there's much more we can do unless we're ok to fully remove add-on support (which is a bad idea imho).
Comment 6 User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-10 14:33:29 PDT
(In reply to Fabrice Desré [:fabrice] from comment #5)
> we're ok to fully remove add-on support (which is a bad idea imho).

Why do you think it's a bad idea?  It's 100% unused code.
Comment 7 User image [:fabrice] Fabrice Desré 2012-10-10 14:42:55 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> (In reply to Fabrice Desré [:fabrice] from comment #5)
> > we're ok to fully remove add-on support (which is a bad idea imho).
> 
> Why do you think it's a bad idea?  It's 100% unused code.

For a few reasons:
- It's needed to run tests iirc and the embedded httpd.js when Gaia is in DEBUG mode.
- Some partners said at some point that they would like to ship pre-installed extensions (maybe it's not true anymore).
- I'd like us to keep that for people that want to experiment at a platform level without having to reflash.
Comment 8 User image Justin Lebar (not reading bugmail) 2012-10-10 15:11:46 PDT
Is that worth 300kb of memory, though?  We're so tight at the moment, I'm not convinced it is worth spending 300kb for features that we may use at some point in the future.
Comment 9 User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-10 17:22:36 PDT
(In reply to Fabrice Desré [:fabrice] from comment #7)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> > (In reply to Fabrice Desré [:fabrice] from comment #5)
> > > we're ok to fully remove add-on support (which is a bad idea imho).
> > 
> > Why do you think it's a bad idea?  It's 100% unused code.
> 
> For a few reasons:
> - It's needed to run tests iirc and the embedded httpd.js when Gaia is in
> DEBUG mode.

Why is it needed to run tests?

We can enable it for alternate configs, no problem.  But hopefully not for release.

> - Some partners said at some point that they would like to ship
> pre-installed extensions (maybe it's not true anymore).

They don't need the addon manager for this, I don't believe.

> - I'd like us to keep that for people that want to experiment at a platform
> level without having to reflash.

Sure, no problem --- the question is about the shipping config.
Comment 10 User image [:fabrice] Fabrice Desré 2012-10-10 18:35:55 PDT
Created attachment 670208 [details] [diff] [review]
patch

With this patch, we get rid of :

addonManager.js
addonManager.jsm
LightweightThemeManager.jsm

From a couple of runs with/without, it seems that we get about 1 Mo of memory back.
Comment 11 User image [:fabrice] Fabrice Desré 2012-10-10 18:40:46 PDT
Created attachment 670210 [details] [diff] [review]
patch v2

I'm not sure who can review this?
Comment 12 User image Justin Lebar (not reading bugmail) 2012-10-10 18:48:31 PDT
Maybe Taras for the Telemetry changes?  (He might want us to do something a bit cleaner, like ifndef according to a ENABLE_LIGHTWEIGHT_THEMES variable.)
Comment 13 User image (dormant account) 2012-10-10 22:20:57 PDT
Comment on attachment 670210 [details] [diff] [review]
patch v2

I don't know what this makefile goop does, but glandium's r+ might be sufficient here.
Comment 14 User image Mike Hommey [:glandium] 2012-10-10 23:55:55 PDT
Comment on attachment 670210 [details] [diff] [review]
patch v2

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

::: toolkit/components/telemetry/TelemetryPing.js
@@ +17,1 @@
>  Cu.import("resource://gre/modules/ctypes.jsm"); 

Please remove the trailing whitespace, while here.
Comment 15 User image [:fabrice] Fabrice Desré 2012-10-12 13:39:10 PDT
Created attachment 670934 [details] [diff] [review]
patch v3

Sorry for the churn, but I learned that it was a bad idea to disable addons in desktop builds since it breaks developers using gaia DEBUG mode.

So this patch is using MOZ_WIDGET_GONK defines to only filter these out on device.
Comment 16 User image Mike Hommey [:glandium] 2012-10-14 23:41:02 PDT
Comment on attachment 670934 [details] [diff] [review]
patch v3

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

::: b2g/installer/package-manifest.in
@@ +379,5 @@
>  @BINPATH@/components/GPSDGeolocationProvider.manifest
>  @BINPATH@/components/GPSDGeolocationProvider.js
>  @BINPATH@/components/nsSidebar.manifest
>  @BINPATH@/components/nsSidebar.js
>  @BINPATH@/components/extensions.manifest

I guess you're leaving extensions.manifest for nsBlocklistService? Is it any useful without the addon manager? If not, you can remove extensions.manifest and nsBlocklistService.js.
Comment 17 User image [:fabrice] Fabrice Desré 2012-10-15 11:15:34 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/53146375872f
Comment 18 User image Ed Morley [:emorley] 2012-10-16 01:24:30 PDT
https://hg.mozilla.org/mozilla-central/rev/53146375872f
Comment 19 User image Ryan VanderMeulen [:RyanVM] 2012-10-16 17:49:18 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/de000386f8bf

Note You need to log in before you can comment on or make changes to this bug.