949637 - Use LocalBroadcastManager to create more efficient and secure broadcasts

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I don't think we make many cross-process/app broadcasts so it seems we can use the LocalBroadcastManager to make better broadcasts. See more about LocalBroadcastManager at [1].

[1]: https://developer.android.com/reference/android/support/v4/content/LocalBroadcastManager.html

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Finkle, is this desirable?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I noticed LocalBroadcastManager a little while ago and thought it might be something useful for Fennec. 
* Do we have more than a few places where we could use it?
* Does it cleanup our code?
* What kind of efficiencies does it give?

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> * Do we have more than a few places where we could use it?

Using an mxr search for all references to `sendBroadcast(`: [1], [2]

Background services has a few potential changes ([3], [4]), but since that code runs in a service, I'm not sure what the implications may be for using LocalBroadcastManager.

> * Does it cleanup our code?

Roughly equivalent: `Context.sendBroadcast(Intent)` vs. `LocalBroadcastManager.getInstance(Context).sendBroadcast(Intent)`

I believe we can continue to use the statically intialized receivers (i.e. `<receiver ...') though the docs are unclear.
 
> * What kind of efficiencies does it give?

LBM appears to iterate over a list of receivers, filter the appropriate intents to receivers, and distribute the broadcasts [5].

Context.sendBroadcast is harder to follow, but requires IPC so I'd wager it's pretty inefficient for what we're trying to achieve [6]. Additionally, it must filter against all registered receivers, including those installed by other apps.

An additional benefit of LBM is that it's impossible for local broadcasts to leave our package, making them more secure.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/PasswordsProvider.java?rev=c95c77bbd0d1#192
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/updater/UpdateService.java?rev=ce80d022ae3f#144

[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/setup/SyncAuthenticatorService.java?rev=d1597e9d7d89#252
[4]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java?rev=c2b850565e10#458

[5]: http://androidxref.com/4.4.2_r1/xref/frameworks/support/v4/java/android/support/v4/content/LocalBroadcastManager.java#200
[6]: http://androidxref.com/4.4.2_r1/xref/frameworks/base/core/java/android/app/ContextImpl.java#1122

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Feel free to only land [1] and [2] in comment 3, and save the research for [3] and [4] in a followup.

Comment 5

[Alexandru Chiriac]

Hello, 

I would like to work on this bug, if that's ok with you.

Thanks, 
Alex

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sure, I've assigned you to the bug! Thanks for your help!

If you decide to look at the LocalBroadcastManagers in the background Services, as mentioned in comment 3, please let me know because you'll actually need to land your changes on github first [1]. I can give you some instructions as to that process.

If that's something that you're not interested in dealing with, that's also fine - I can file a followup.

[1]: https://github.com/mozilla-services/android-sync

Comment 7

[Alexandru Chiriac]

Hello Michael, 

The statically initialized receivers in the manifest will not handle any broadcasts sent via LocalBroadcastManager.

It seems that using LocalBroadcastManager implies registering/unregistering the receivers to the broadcast manager before sending/handling any local broadcast intents. Registering/unregistering a local broadcast receiver implies the receiver's instantiation (e.g LocalBroadcastManager.get(context).registerReceiver(receiver, intentFilters) ).

From the code cleanup standpoint, the two broadcast dispatching modes are unfortunately not equivalent, using LocalBroadcastManager increasing the code complexity.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Thanks for the research Alexandru!

I think the added code complexity is acceptable. Finkle, what do you think? Is it worth continuing with the LocalBroadcastManager? Registering a receiver would amount to:

final LocalBroadcastManager lbm = LocalBroadcastManager.getInstance(Context);
lbm.registerReceiver(BroadcastReceiver, new IntentFilter(String));

Our BroadcastReceivers are likely already defined, and the intent filters are not likely complex.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

We should take a look at a simple patch and see what changes are needed to get something useful landed. If we think the complexity is worth the benefits, we should continue.

I am thinking the small amount of complexity will be worth it, but let's not commit until we see a patch.

Comment 10

[Alexandru Chiriac]

Attachment: Improved the efficiency and security of some application broadcasts, using the broadcast system managed by Android's LocalBroadcastManager component.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8422424 [details] [diff] [review]
Improved the efficiency and security of some application broadcasts, using the broadcast system managed by Android's LocalBroadcastManager component.

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

Hmm, thinking about this now, both the UpdateService and PasswordsProvider can have a lifecycle that may occur outside the lifecycle of the main Activity. Sorry to make this into a larger research bug, but do you know what the lifecycle of the LocalBroadcastManager is? Will we still receive broadcasts if we're not the foreground Activity?

(By the way, if this is too much research and not enough code, feel free to pass on the bug - I don't want you getting caught up in it! I already appreciate your hard work! :)

If the lifecycles look useable, make sure you have the opportunity to test this code as well (I know it's a bit of a pain) - it may even help you answer the questions above! You can ask snorp how he tests the updater.

You also want to remove the now unusued static `<receiver...` tags from the manifest, right?

Also, using `final`! Nice! ^_^

::: mobile/android/base/BrowserApp.java
@@ +212,5 @@
>      private boolean mHideWebContentOnAnimationEnd = false;
>  
>      private DynamicToolbar mDynamicToolbar = new DynamicToolbar();
>  
> +    private List<BroadcastReceiver> mLocalBroadcastReceivers;

Why not set create the new List in `onCreate` so we don't have to do null checks?

Also, making this an array seems reasonable to me too.

@@ +596,5 @@
>  
>          // Set the maximum bits-per-pixel the favicon system cares about.
>          IconDirectoryEntry.setMaxBPP(GeckoAppShell.getScreenDepth());
> +
> +        registerLocalBroadcastReceivers();

Related to lifecycles, I wonder if this would need to occur in foreground lifecycle calls (which I believe would make LocalBroadcastManager useless to us).

@@ +718,5 @@
> +
> +    private void unregisterLocalBroadcastReceivers() {
> +        if (mLocalBroadcastReceivers != null) {
> +            final LocalBroadcastManager lbm = LocalBroadcastManager.getInstance(this);
> +            for (BroadcastReceiver receiver : mLocalBroadcastReceivers) {

nit: `final BroadcastReceiver receiver : ...`, if I'm not mistaken. :)

Comment 12

[Alexandru Chiriac]

Attachment: Improved the efficiency and security of some application broadcasts, using the broadcast system managed by Android's LocalBroadcastManager component.

1. Fixed the issues found after reviewing the previous patch. 

2. Investigated the LocalBroadcastManager's lifecycle with regards to the lifecycle of an activity:

- Given the fact that the broadcast receivers are registered to the LocalBroadcastManager programmatically, at some point they must be unregistered, for cleanup purposes. If done inside an activity, the receivers register and unregister operations are done in symmetric activity lifecycle callbacks (e.g. onCreate() - onDestroy(), onStart() - onStop(), onResume() - onPause()), keeping  the application's workflow consistent.

- However, as it was implemented in the first patch (registering/unregistering were done in BrowserApp activity in onCreate()/onDestroy()), if the main activity is not in the foreground and the OS destroys it because of low memory issues for example, we will definitely not receive any broadcasts, until the activity is recreated and the receivers are re-registered.

- Therefore I've moved the receivers register/unregister operations to the app's android.app.Application implementation, respectively in onCreate()/onTerminate() callbacks. In this way, we will receive local broadcasts even if there is no activity in the foreground, until the application's process is eventually killed.

In case the approach adopted in this patch is acceptable, I will ask snorp for help on how can I test this code. :)

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8424881 [details] [diff] [review]
Improved the efficiency and security of some application broadcasts, using the broadcast system managed by Android's LocalBroadcastManager component.

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

This approach looks good to me - let me know the results of your testing!

::: mobile/android/base/GeckoLocalBroadcastManager.java
@@ +26,5 @@
> +    }
> +
> +    public void init(Context context) {
> +        mContext = context;
> +        mBroadcastReceivers = new ArrayList<BroadcastReceiver>();

I think I'd prefer a regular array here - less memory and cpu overhead all around (especially important when called from Application.onCreate).

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8424881 [details] [diff] [review]
Improved the efficiency and security of some application broadcasts, using the broadcast system managed by Android's LocalBroadcastManager component.

Bringing in Finkle to see what he thinks of this patch sample.

(Alexandru, you may want to wait until this feedback request is finished before testing)

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8424881 [details] [diff] [review]
Improved the efficiency and security of some application broadcasts, using the broadcast system managed by Android's LocalBroadcastManager component.

GeckoLocalBroadcastManager is not really a manager. Maybe just rename it to GeckoLocalBroabcasts.

The patch doesn't look to add any complexity to the existing system and if it makes things more efficient, then I am for it.

You should probably loop RNewman into reviews when you are ready.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Alexandru, I'd ping snorp for testing info on the UpdateService, and then when you've tested the two changes, I'd flag myself and rnewman for review.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Alexandru, are you still working on this?

Comment 18

[Mike Hoye [:mhoye]]

OK, it's been two weeks now. I'm throwing this one back in the pool. Alexandru, if you're interested in pick it back up, please do! And thanks for bringing it this far.

Comment 19

[Dominique Vincent [:domivinc]]

Hello,
I started to work on this bug.

1- I have one question regarding the scope of the bug, should we implement a fix only for the “Paint flashing” checkbox or should we implement a fix for all the checkboxes in the settings menu? There are probably other localisations with the same issue?

2- On a device with a trakball or D-pad (tested on my Desire-Z for instance), it works without any issue. When the checkbox is selected using the D-pad, the text scrolls automatically to the left and you can read the end of the text.

3- I found that xml attributes cannot be used in this case. The xml element is “CheckBoxPreference” (http://developer.android.com/reference/android/preference/CheckBoxPreference.html) and there is no solution to change the style in the XML.
But the documentation gives another way to implement the wrap/ellipse properties. In the method onBindView: “This is a good place to grab references to custom Views in the layout and set properties on them.”. I made a test, and that solution works (see attached apk). 

Let me know what you want to do.
Regards, Dom

Comment 20

[Dominique Vincent [:domivinc]]

Sorry wrong bug, it was for 1048418

Comment 21

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 25

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.