Closed Bug 810988 Opened 9 years ago Closed 9 years ago

Announcements broadcast causes all Firefox processes to launch

Categories

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

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla19

People

(Reporter: kats, Assigned: rnewman)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
The inverse also seems to be the case (i.e. starting org.mozilla.fennec will start org.mozilla.fennec_kats)
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.
Goddamnit, Android.
Component: General → Android: Product Announcements
Product: Firefox for Android → Mozilla Services
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
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.
Summary: Starting developer fennec build also starts nightly build → Announcements broadcast causes all Firefox processes to launch
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?
(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.).
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.
(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.
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.
Attachment #680905 - Flags: review?(bugmail.mozilla)
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.
Attachment #680905 - Flags: review?(bugmail.mozilla) → review+
With `hg mv` first.
Attachment #680901 - Attachment is obsolete: true
Attachment #680901 - Flags: review?(nalexander)
Attachment #680917 - Flags: review?(nalexander)
Try is green.
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.
Attachment #680917 - Flags: review?(nalexander) → review+
(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
(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... :)
> > 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
   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.
Target Milestone: --- → mozilla19
Blocks: 811358
Status: RESOLVED → VERIFIED
Component: Android: Product Announcements → Product Announcements
Product: Mozilla Services → Android Background Services
You need to log in before you can comment on or make changes to this bug.