810988 - Announcements broadcast causes all Firefox processes to launch

Status: VERIFIED FIXED

(Android Background Services Graveyard :: Product Announcements, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Take a device with a recent nightly (org.mozilla.fennec) and also a local developer build (e.g. org.mozilla.fennec_kats). Start the developer build by clicking on the icon. Run "adb shell ps | grep fennec"

Expected results:
Only org.mozilla.fennec_kats shows up in the process list

Actual results:
Both org.mozilla.fennec and org.mozilla.fennec_kats show up.

BenWa was saying this behaviour (or something similar) was causing problems with the profiler as well because it doesn't like seeing two running fennec processes.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

The inverse also seems to be the case (i.e. starting org.mozilla.fennec will start org.mozilla.fennec_kats)

Comment 2

[Kartikaya Gupta (email:[e-mail])]

From the logcat this appears to be because of this:

11-12 20:53:41.223 I/ActivityManager( 1291): Start proc org.mozilla.fennec_kats for broadcast org.mozilla.fennec_kats/org.mozilla.gecko.background.announcements.AnnouncementsBroadcastReceiver: pid=2395 uid=10040 gids={3003, 1015, 1006}

CC'ing rnewman who worked on the announcements code, it looks like.

Comment 3

[Richard Newman [:rnewman]]

Goddamnit, Android.

Comment 4

[Richard Newman [:rnewman]]

Okay, reading more carefully, no "goddamnit Android".

The fundamentals of this behavior are not wrong: we now have an alarm-managed service that kicks off at system startup and on Fennec launch (with the goal of running *all the time*). All that's happening here is that there's no filtering; the two apps share an intent action, so launching one makes sure that the other is also instantiated.

If no activity is being launched, I'm not concerned at all.

That it's causing problems with the profiler is out of scope for this bug: our tooling should expect henceforth that there will be inactive Fennec processes intermittently running for any Fennec package installed on the system.

The only real issue here is if these two applications cross-mojinate fetching prefs. I'll address that.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I've noticed that the update service seems to run as a separate process. I'll see org.mozilla.fennec_kats and org.mozilla.f3nn3c.UpdateService (or something like that) as separate entries in the process list. Can something like that be done here?

Comment 6

[Richard Newman [:rnewman]]

(In reply to Kartikaya Gupta (:kats) from comment #5)
> I've noticed that the update service seems to run as a separate process.
> I'll see org.mozilla.fennec_kats and org.mozilla.f3nn3c.UpdateService (or
> something like that) as separate entries in the process list. Can something
> like that be done here?

We access GeckoPreferences when deciding what to do here, so multiprocessing isn't really on the table.

It's best to not think about processes too much on Android, I think.

We're not in charge of deciding when our process runs; Android just uses it as a handy bucket in which to put various service handlers (including Sync, authenticators, intent handlers, etc.).

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Ok, fair enough. It's just disconcerting to see all these processes running and never being sure if it's something I need to worry about or not.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Part 1: services part. v1

https://github.com/mozilla-services/android-sync/pull/268

Comment 9

[Richard Newman [:rnewman]]

(In reply to Kartikaya Gupta (:kats) from comment #7)
> Ok, fair enough. It's just disconcerting to see all these processes running
> and never being sure if it's something I need to worry about or not.

I quite understand!

The following patches will stop the broadcast from being caught by other Fennecs, but you can still expect them to occasionally be brought to life by Android itself.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Part 2: Fennec part. v1

All this does is make a tiny change to the Makefile to cause preprocessing of announcements constants to succeed, then has GeckoPreferences directly pull the preprocessed name out of AnnouncementsConstants.

This builds, but I haven't tested it yet.

Comment 11

[Richard Newman [:rnewman]]

Try build:

https://tbpl.mozilla.org/?tree=Try&rev=d35ad0d934d1

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 680905 [details] [diff] [review]
Part 2: Fennec part. v1

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

LGTM. For the first patch you should probably move the file using "hg cp" to preserve history. Or if you're using git to generate the patch, use the -M -C options to diff/show.

Comment 13

[Richard Newman [:rnewman]]

Attachment: Part 1: services part. v2

With `hg mv` first.

Comment 14

[Richard Newman [:rnewman]]

Try is green.

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 680917 [details] [diff] [review]
Part 1: services part. v2

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

Fine by me.

Braindump: I wondered if there is any reason to make this broadcast intent be package private.  I do not think it is difficult to send the other (global) Android intents this receiver listens for (`adb -d shell am broadcast -a android.intent.action.BOOT_COMPLETED`, for example), so making the intent private doesn't make it hard to start this code running.

On the other hand, this does leak user settings information.  rnewman and code inspection verify that the announcement is specific to the announcements preference alone.  I see no attack based on this one leaked bit.

Comment 16

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #15)

> Braindump: I wondered if there is any reason to make this broadcast intent
> be package private.

Simply to avoid Fennec A starting Fennec B's process (and telling it the wrong pref value).

When we have access to the compatibility libraries, I'll make this a local broadcast intent, rather than a true broadcast. Simply a deployment limitation right now.

> On the other hand, this does leak user settings information.  rnewman and
> code inspection verify that the announcement is specific to the
> announcements preference alone.  I see no attack based on this one leaked
> bit.

The approach went through sec review, so… :D

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #16)
> (In reply to Nick Alexander :nalexander from comment #15)
> 
> > Braindump: I wondered if there is any reason to make this broadcast intent
> > be package private.
> 
> Simply to avoid Fennec A starting Fennec B's process (and telling it the
> wrong pref value).
> 
> When we have access to the compatibility libraries, I'll make this a local
> broadcast intent, rather than a true broadcast. Simply a deployment
> limitation right now.

Is this due to a change in Android's broadcast intents system over time?  We can arrange for this by using a permission and modifying calling and receiving code a little.  We do this per account type for deleting sync accounts cleanly; it would be easy to duplicate it per fennec package.  So if this is desirable, I think you can have it now.

> > On the other hand, this does leak user settings information.  rnewman and
> > code inspection verify that the announcement is specific to the
> > announcements preference alone.  I see no attack based on this one leaked
> > bit.
> 
> The approach went through sec review, so… :D

Surely you're not suggesting we not consider security ramifications because it's officially someone else's job... :)

Comment 18

[Richard Newman [:rnewman]]

> > When we have access to the compatibility libraries, I'll make this a local
> > broadcast intent, rather than a true broadcast. Simply a deployment
> > limitation right now.
> 
> Is this due to a change in Android's broadcast intents system over time?

http://developer.android.com/reference/android/support/v4/content/LocalBroadcastManager.html

So long as we support 2.x, we need the compat lib to do true local broadcasts.

> We do this per account type for deleting sync
> accounts cleanly; it would be easy to duplicate it per fennec package.  So
> if this is desirable, I think you can have it now.

Unlike Sync account cleanup, there's no firm need for these broadcast intents to be received by more than one Fennec.

> Surely you're not suggesting we not consider security ramifications because
> it's officially someone else's job... :)

:D

WE ARE ALL ON THE MOBILE^W SECURITY TEAM
:D

Comment 19

[Richard Newman [:rnewman]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/387a105a6e41
   https://hg.mozilla.org/integration/mozilla-inbound/rev/67fd0ece8d38

Strictly speaking doesn't require uplift, because only one version of Firefox will end up using org.mozilla.gecko.ANNOUNCEMENTS_PREF.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/387a105a6e41
https://hg.mozilla.org/mozilla-central/rev/67fd0ece8d38