759448 - Don't register as a sensor data receiver until there is someone registered as a listener

Status: NEW

(GeckoView :: General, defect, P5)

Description

[Brad Lassey [:blassey] (use needinfo?)]

3 reasons for this:
1) we're doing a small amount of work onPause and onResume that we don't need to do
2) we're the incoming information that we'll wind up just dropping on the floor
3) a large portion of our crash bugs are associated with registering and unregister when this isn't a common thing on the web right now (for example #1 top crasher right now which is 2x the #2 crasher by volume)

Comment 1

[Mounir Lamouri (:mounir)]

I currently have four listeners in Native Fennec:
- Network and Screen Orientation: both have start() and stop() being called on application being resumed and stopped but those methods will not start and stop listening if there is no registered listeners. Basically, there are two booleans in the class, one for "is the application in a state that allows me to listen" and one fore "is someone interested in what i'm listening". start() and stop() will change the value of the former. If both are true, we are listening, when one of them goes from true to false, listening is stopped.
- Battery: it is listening from startup to destroy. The reason is that getting the battery status is async and the DOM API requires it to be sync. Basically, on Android, you say that you want to listen and immediately after that you get an event firing with the battery info. We could have a system that would make us wait for that event when we begin listening so we can return a value synchronously if needed. We could have something more complex with enableBatteryNotifications doing the listener registration and getCurrentBatteryInformation waiting for the result if it didn't come yet.
- SMS: on all official builds, SmsManager.getInstance() will return null so nothing happens here.

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to Brad Lassey [:blassey] from comment #0)
> 1) we're doing a small amount of work onPause and onResume that we don't
> need to do

I see that start/stop now happen in onActivityResume/onActivityPause. I haven't find any doc about that. I remember that I wrote in the past something to make those methods being called asynchronously because even if they are nearly no-op when there are no listeners, they will do something if there is at least one. I guess those two methods are called after onPause/onResume?

> 3) a large portion of our crash bugs are associated with registering and
> unregister when this isn't a common thing on the web right now (for example
> #1 top crasher right now which is 2x the #2 crasher by volume)

I only see this one: https://crash-stats.mozilla.com/report/list?range_value=7&range_unit=days&date=2012-05-30&signature=java.lang.IllegalArgumentException%3A%20Receiver%20not%20registered%3A%20org.mozilla.gecko.GeckoConnectivityReceiver%40%3Caddr%3E%3A%20at%20android.app.LoadedApk.forgetReceiverDispatcher%28LoadedApk.java%29&version=FennecAndroid%3A15.0a1

Which is #7 but still too high :(

Actually, for that crash, the bug is marked as fixed but it seems to still happen. Am I mis-reading crash-stats?

Comment 3

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

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)

> Actually, for that crash, the bug is marked as fixed but it seems to still
> happen. Am I mis-reading crash-stats?

Timing is everything when looking at crash-stats and "fixed" bugs. The bug (bug 744850) only jjst landed and is not really out in any release yet. We should see an affect, if any, after we release the next beta.

Comment 4

[Doug Turner (:dougt)]

i haven't been able to get the time to look at this.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

got time now?

Comment 6

[Doug Turner (:dougt)]

unassigning things that I am not working on.

Comment 7

[Randall Barker [:rbarker]]

Attachment: part 1, register for battery events when listener is registered.

This patch registers for battery events when a listener is registered. Plan to do the same for Connectivity and Network but wanted to make sure this was correct before converting them as well.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8393243 [details] [diff] [review]
part 1, register for battery events when listener is registered.

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

I was thinking more along the lines of:

* Don't call start() or anything else from GeckoApplication.onActivityResume
* Make start() private and rename to ensureStarted() or something to indicate idempotence
* Call ensureStarted() from enableNotifications
* Maybe outside the scope of this patch, but consider changing enable/disableNotifications to a counter instead of true/false
* Just call stop() from disableNotifications

Comment 9

[Randall Barker [:rbarker]]

Attachment: Part 1: Activate Battery Manager when listener is registered

Removed the start and stop functions.

Comment 10

[Randall Barker [:rbarker]]

Comment on attachment 8393625 [details] [diff] [review]
Part 1: Activate Battery Manager when listener is registered

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

I just got rid of the start and stop functions in the battery manager, seemed simpler. The Network Manager should not take long to convert as well. The connectivity receiver does not have an enable and disable calls so it will be more involved to convert.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8393625 [details] [diff] [review]
Part 1: Activate Battery Manager when listener is registered

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

Looks a lot better

::: mobile/android/base/GeckoBatteryManager.java
@@ +25,4 @@
>      private final static double  kUnknownRemainingTime = -1.0;
>  
>      private static long    sLastLevelChange            = 0;
> +    private static long    sNotificationsEnabled       = 0;

sNumListeners maybe? Doesn't need to be a long either.

@@ +155,4 @@
>          return sRemainingTime;
>      }
>  
> +    public static void enableNotifications(Context applicationContext) {

'applicationContext' sort of implies that it is the Android Application context, which may not be true. Just 'context' might be better.

Comment 12

[Randall Barker [:rbarker]]

How far should I take this to ensure the NetworkManager only registers for network change notifications when the JavaScript side register an event listener? The problem is the Tickler needs the DhcpGateway. Unfortunately, the call to get the DhcpGateway causes the NetworkManager to register for network change notifications. I see two solutions:

1) Modify the Connection class so that the notify callback is only registered when an event listener is registered and otherwise call through to the Android NetworkManager for any queries when an event listener has not been registered.

2) Add and extra function to nsINetworkProperties that allows the DhcpGateway to be queried without causing the notify callback to be registered.

I think option 1 is the the cleanest but will touch the most files and has greater risk of unintended side effects.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Where does the tickler need the DHCP? I don't see that anywhere in Tickler.cpp. I see it enables network notifications, but doesn't ever query for anything or process notification events, AFAICT...

Comment 14

[Randall Barker [:rbarker]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #13)
> Where does the tickler need the DHCP? I don't see that anywhere in
> Tickler.cpp

It is actually used in nsHttpHandler::TickleWifi. I'm planning to remove the call to GeckoAppShell::EnableNetworkNotifications in Tickler.cpp. The Enable call was made in the Tickler so that nsHttpHandler can get the DhcpGatway.

Comment 15

[Randall Barker [:rbarker]]

Attachment: Part 1: Activate Battery Manager when listener is registered V4

Comment 16

[Randall Barker [:rbarker]]

Attachment: Part 2: Activate Network Manager when listener is registered V1

Comment 17

[Randall Barker [:rbarker]]

Attachment: Part 2: Activate Network Manager when listener is registered V2

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 19

[David Lawrence [:dkl]]

Moving all open Core::Widget: Android bugs to GeckoView::General (then the triage owner of GeckoView will decide which ones are valuable and which ones should be closed).

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:fluffyemily, could you have a look please?
For more information, please visit auto_nag documentation.