Closed Bug 1117095 Opened 5 years ago Closed 5 years ago

Update downloader should use a wifi lock

Categories

(Firefox for Android :: Download Manager, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38

People

(Reporter: kats, Assigned: esawin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

I often find when I wake up in the morning that sometime during the night Fennec started to download an update, but then the download failed. I suspect what is happening is that Android turns on the wi-fi for short bursts. Since I have Fennec set to only download updates over wi-fi, it finds an update and starts downloading it during this short burst. However then Android cycles off the wi-fi and the download fails. We can avoid this by holding a WifiLock [1] while downloading the update. It might be useful to hold this while the browser is in the foreground as well, as it might explain some of our poor network behaviour (page load stalls, etc.).

[1] http://developer.android.com/reference/android/net/wifi/WifiManager.WifiLock.html
I could definitely see this affecting the updater. If it also affects Fennec proper, while in the foreground, I would be mildly surprised. I wonder if we can monitor the wifi state somehow to determine this. WifiManager doesn't appear to tell us when it goes to sleep.

Anyway, we should definitely take a wifi lock while we are downloading updates in the background if we are in fact using wifi. I am not sure about a lock for when Fennec is in the foreground. Needs more investigation.
Assignee: nobody → esawin
I vaguely remember somebody commenting on some bug somewhere (can't find it now) that Fennec's network behaviour was much better if Chrome was open, even if in the background. Chrome acquiring a wifi lock might explain that.

I can't see any reason why Android would hold wifi open for us even if we are in the foreground. I could conceivably see them keeping wifi on while the screen is on but that doesn't overlap exactly with Fennec being in the foreground.
Is this something we ought to do while syncing, or can we trust Android to be smart enough to hold interfaces open for the duration of SyncAdapters it triggers?
I don't think we should assume it does (unless you find code to that effect in the Android source).
Attached patch updater-wifi-lock (obsolete) — Splinter Review
With this patch, the update service holds the WiFi lock for the duration of the package downloading.
Attachment #8548323 - Flags: review?(snorp)
Status: NEW → ASSIGNED
Comment on attachment 8548323 [details] [diff] [review]
updater-wifi-lock

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

::: mobile/android/base/updater/UpdateService.java
@@ +119,2 @@
>      private Builder mBuilder;
> +    private WifiLock mWifiLock;

onCreate (which sets this and destroys it) is run on the main thread, and onHandleIntent is invoked on the worker thread. Consider making this volatile to avoid thread consistency issues.

(The rest of onCreate probably suffers the same fate.)

@@ +137,5 @@
>  
>          mPrefs = getSharedPreferences(PREFS_NAME, 0);
>          mNotificationManager = (NotificationManager)getSystemService(Context.NOTIFICATION_SERVICE);
>          mConnectivityManager = (ConnectivityManager)getSystemService(Context.CONNECTIVITY_SERVICE);
> +        mWifiManager = (WifiManager)getSystemService(Context.WIFI_SERVICE);

You don't need mWifiManager; just a local variable will suffice.

@@ +525,5 @@
>          OutputStream output = null;
>          InputStream input = null;
>  
> +        NetworkInfo netInfo = mConnectivityManager.getActiveNetworkInfo();
> +        if (netInfo.getType() == ConnectivityManager.TYPE_WIFI) {

gANI can return null, so you can't immediately call getType.

You should also check .isConnected before using the connection.
Attachment #8548323 - Flags: review?(snorp) → review-
Attached patch updater-wifi-lock (obsolete) — Splinter Review
Declared the lock member volatile, removed the manager member and moved the (unsafe) netInfo check into the download try block.

I think at the current state, this is a sane solution, since all of downloadUpdatePackage expects an active connection and that way the cleanup would be triggered, should the connectivity state change in the process.

I'm preparing some refactoring work for the update service (starting with bug 1115377), where we can rethink the downloading procedure to make it safer regarding network changes.
Attachment #8548323 - Attachment is obsolete: true
Attachment #8548386 - Flags: review?(rnewman)
Comment on attachment 8548386 [details] [diff] [review]
updater-wifi-lock

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

r+ with that null check.

::: mobile/android/base/updater/UpdateService.java
@@ +530,5 @@
>          showDownloadNotification(downloadFile);
>  
>          try {
> +            NetworkInfo netInfo = mConnectivityManager.getActiveNetworkInfo();
> +            if (netInfo.getType() == ConnectivityManager.TYPE_WIFI) {

if (netInfo != null &&
        netInfo.getType() == ConnectivityManager.TYPE_WIFI &&
        netInfo.isConnected()) {

I read startUpdate, and it does a bunch of work before it gets here, so gANI can definitely return something different, hence the null check. And it's possible we're disconnected by the time we get here, in which case there's no point acquiring the wifi lock.

Citation:

"You should always check isConnected() before initiating network traffic. This may return null when there is no default network."

http://developer.android.com/reference/android/net/ConnectivityManager.html#getActiveNetworkInfo%28%29
Attachment #8548386 - Flags: review?(rnewman) → review+
Attachment #8548386 - Attachment is obsolete: true
Attachment #8548503 - Flags: review+
The "refactoring" bug mentioned in comment 7 is actually bug 792992.
https://hg.mozilla.org/mozilla-central/rev/f3285709bf99
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Unfortunately I still get download failures pretty frequently on nightly :(
You need to log in before you can comment on or make changes to this bug.