811358 - Use local broadcast

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

Compat library has landed.

Comment 1

[Richard Newman [:rnewman]]

For reference:

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

Comment 2

[Nick Alexander :nalexander [he/him] (PTO until Monday, July 28)]

I'm not sure we want to do this.  The Announcements implementation has two receivers: the relevant one here is a BroadcastReceiver that handles system intents and intents from Fennec notifying that relevant preferences have changed (and hands off to another receiver that actually starts the background service).  This BroadcastReceiver routes system intents back through the Fennec preferences system so that there is a single entry point via a preference change intent.  This last entry point could be a local broadcast (and the system intents could be moved into Fennec proper, avoiding the routing loop).

The sticky point is that local broadcast receivers must be explicitly registered.  We don't have a guaranteed, always on, Service to register/unregister the local broadcast receiver for us: we use Android manifest entries to instantiate the receiver at the appropriate time.  I suppose we could make Fennec register/unregister the local receivers for us, but at this point we're tightly coupling Fennec to the background receivers.

If we could register a service (or a receiver) such that its life cycle were assured, we wouldn't need this receiver listening for system events at all!

Am I missing something here?

Comment 3

[Richard Newman [:rnewman]]

I think I can rephrase your point as "LocalBroadcast listeners cannot be registered in the manifest; Broadcast listeners can".

That's the technique we use to decouple here: Fennec doesn't need to know about the background services, because it just fires a broadcast, and Android takes care of starting those services thanks to manifest registrations.

I had hoped that local broadcast listeners could also be registered in the manifest.

The alternative, in light of our increasing coupling between background services and Fennec, is to bite the bullet: directly call startService on the relevant background services, rather than using a broadcast intent.

If we wish we can define a dispatching system to do so (MessageBus.notifyPrefChange("...")), which allows us to centralize our implicit registrations -- FHR, product announcements, homepage snippets -- and just chalk up broadcasts as an evolutionary dead end. That will allow us to tidy up some of the back and forth.

Comment 4

[Richard Newman [:rnewman]]

Apparently we can apply a permission to a sent broadcast, and require it in the receiver, for broadcasts. That basically gives us local broadcasts that don't suck.

      mContext.sendBroadcast(intent, SyncConstants.PER_ACCOUNT_TYPE_PERMISSION);

Comment 5

[Nick Alexander :nalexander [he/him] (PTO until Monday, July 28)]

Attachment: Protect broadcasts with per-package Android signature-level permission. r=rnewman

Comment 6

[Nick Alexander :nalexander [he/him] (PTO until Monday, July 28)]

Comment on attachment 754569 [details] [diff] [review]
Protect broadcasts with per-package Android signature-level permission. r=rnewman

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

No sense not doing this while I'm here.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 754569 [details] [diff] [review]
Protect broadcasts with per-package Android signature-level permission. r=rnewman

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

::: mobile/android/services/manifests/SyncAndroidManifest_permissions.xml.in
@@ +26,5 @@
> +    <permission
> +        android:name="@ANDROID_PACKAGE_NAME@.permission.PER_ANDROID_PACKAGE"
> +        android:protectionLevel="signature"/>
> +
> +    <uses-permission android:name="@ANDROID_PACKAGE_NAME@.permission.PER_ANDROID_PACKAGE" />

For thoroughness, this should also (or instead) be in AnnouncementsManifest_permissions, no?

Comment 8

[Nick Alexander :nalexander [he/him] (PTO until Monday, July 28)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd95f49deeda

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd95f49deeda

Comment 10

[Nick Alexander :nalexander [he/him] (PTO until Monday, July 28)]

After the fact, we wondered if these additional permissions would affect the Play Store install dialog.  I am confident the answer is "no".  We already have a permission of this type -- see http://mxr.mozilla.org/mozilla-central/source/mobile/android/services/manifests/SyncAndroidManifest_permissions.xml.in#15 -- that has long since been on Beta (and Release).  I tried to install Firefox Beta from the Play Store, and looking at the (long) list of required permissions I don't see our bespoke permissions.  This makes sense, since these permissions are "signatureLevel", meaning that all verification/access is determined by signatures and certificates (and not by user input).

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 754569 [details] [diff] [review]
Protect broadcasts with per-package Android signature-level permission. r=rnewman

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Earlier omission in broadcast logic for Product Announcements. This is a dependency for the FHR uploader.

User impact if declined: 
  Harder for us to land the FHR uploader, slight risk of internal messaging logic leaking to other apps.

Testing completed (on m-c, etc.):
  Baking on m-c for ages.

Risk to taking this patch (and alternatives if risky): 
  Negligible.

String or IDL/UUID changes made by this patch:
  None.

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 754569 [details] [diff] [review]
Protect broadcasts with per-package Android signature-level permission. r=rnewman

In support of mobile FHR

Comment 13

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e5255296ce29