Open Bug 949637 Opened 7 years ago Updated 2 years ago

Use LocalBroadcastManager to create more efficient and secure broadcasts

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

People

(Reporter: mcomella, Unassigned, Mentored)

Details

(Whiteboard: [lang=java])

Attachments

(1 file, 1 obsolete file)

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
Finkle, is this desirable?
Flags: needinfo?(mark.finkle)
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?
Flags: needinfo?(mark.finkle)
(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
Feel free to only land [1] and [2] in comment 3, and save the research for [3] and [4] in a followup.
Whiteboard: [mentor=mcomella][lang=java]
Hello, 

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

Thanks, 
Alex
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
Assignee: nobody → alexandru.chiriac
Status: NEW → ASSIGNED
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.
Flags: needinfo?(michael.l.comella)
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.
Flags: needinfo?(michael.l.comella) → needinfo?(mark.finkle)
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.
Flags: needinfo?(mark.finkle)
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. :)
Attachment #8422424 - Flags: review?(michael.l.comella) → feedback+
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. :)
Attachment #8422424 - Attachment is obsolete: true
Attachment #8424881 - Flags: review?(michael.l.comella)
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).
Attachment #8424881 - Flags: review?(michael.l.comella) → feedback+
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)
Attachment #8424881 - Flags: feedback?(mark.finkle)
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.
Attachment #8424881 - Flags: feedback?(mark.finkle) → feedback+
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.
Flags: needinfo?(alexandru.chiriac)
Mentor: michael.l.comella
Whiteboard: [mentor=mcomella][lang=java] → [lang=java]
Alexandru, are you still working on this?
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.
Assignee: alexandru.chiriac → nobody
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
Sorry wrong bug, it was for 1048418
Flags: needinfo?(alexandru.chiriac)
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.