Open Bug 759448 Opened 8 years ago Updated Last year

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

Categories

(Core :: Widget: Android, defect, P5)

All
Android
defect

Tracking

()

ASSIGNED
Tracking Status
blocking-fennec1.0 --- -
fennec + ---

People

(Reporter: blassey, Assigned: rbarker)

Details

Attachments

(2 files, 3 obsolete files)

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)
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.
(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?
(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.
tracking-fennec: --- → 15+
blocking-fennec1.0: ? → -
i haven't been able to get the time to look at this.
Assignee: doug.turner → nobody
Status: NEW → UNCONFIRMED
Ever confirmed: false
got time now?
Assignee: nobody → doug.turner
tracking-fennec: 15+ → +
unassigning things that I am not working on.
Assignee: doug.turner → nobody
Assignee: nobody → snorp
Assignee: snorp → rbarker
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.
Attachment #8393243 - Flags: feedback?(snorp)
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
Attachment #8393243 - Flags: feedback?(snorp) → feedback-
Removed the start and stop functions.
Attachment #8393243 - Attachment is obsolete: true
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.
Attachment #8393625 - Flags: feedback?(snorp)
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.
Attachment #8393625 - Flags: feedback?(snorp) → feedback+
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.
Flags: needinfo?(blassey.bugs)
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...
Flags: needinfo?(blassey.bugs) → needinfo?(rbarker)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Attachment #8393625 - Attachment is obsolete: true
Flags: needinfo?(rbarker)
filter on [mass-p5]
Priority: -- → P5
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
You need to log in before you can comment on or make changes to this bug.