Auto-update doesn't respect mobile hotspot setting

RESOLVED FIXED in Firefox 34

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: tgkokk, Assigned: tgkokk)

Tracking

34 Branch
Firefox 34
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140807084340

Steps to reproduce:

1. Designate a Wi-Fi as a mobile hotspot (Settings -> Data Usage -> Mobile hotspots (overflow button)).
2. Connect to that Wi-Fi.
2. Set auto-update to "Only over Wi-Fi".
3. Wait for an update (side note: you need to have a non-Play Store version, since Play Store manages updates on apps downloaded through it).


Actual results:

I get the notification that there is an update for Nightly and the update starts downloading automatically.


Expected results:

Nightly should behave as if I were on mobile data (i.e. show a notification that there is an update and offer the choice to download it).
(Assignee)

Updated

4 years ago
OS: Mac OS X → Android
Hardware: x86 → ARM

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

4 years ago
Created attachment 8471583 [details] [diff] [review]
Make updater respect mobile hotspot setting
Attachment #8471583 - Flags: review?(snorp)
Assignee: nobody → t.kokkoris
(Assignee)

Comment 2

4 years ago
Created attachment 8471643 [details] [diff] [review]
Make auto-updater respect mobile hotspot setting
Attachment #8471643 - Flags: review?(snorp)
(Assignee)

Comment 3

4 years ago
Comment on attachment 8471643 [details] [diff] [review]
Make auto-updater respect mobile hotspot setting

OK, this is it. I haven't been able to test whether it works, because the updater requires a URL (which is hard-coded in UpdateServiceHelper.java). However, I don't see why it might not, since the change is pretty insignificant. I'll make sure to test it when it lands.
(Assignee)

Updated

4 years ago
Attachment #8471583 - Attachment is obsolete: true
Attachment #8471583 - Flags: review?(snorp)
Comment on attachment 8471643 [details] [diff] [review]
Make auto-updater respect mobile hotspot setting

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

Looks ok, but we don't want to break things for folks using ethernet (all three of them)

::: mobile/android/base/updater/UpdateService.java
@@ +220,4 @@
>           */
>          boolean shouldStartDownload = hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) ||
>              autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_ENABLED ||
> +            (autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_WIFI && !mConnectivityManager.isActiveNetworkMetered());

I think you need to keep the TYPE_ETHERNET bit in there
Attachment #8471643 - Flags: review?(snorp) → review-
(Assignee)

Comment 5

4 years ago
Nope, they're fine.

isActiveNetworkMetered() boils down to this:

http://androidxref.com/4.4.4_r1/xref/frameworks/base/services/java/com/android/server/net/NetworkPolicyManagerService.java#1588 

which checks whether the network is a mobile one or if there's a policy that has metered = true (by default, no policies exist).

Now, the screen for changing a WiFi network to metered basically creates a new policy with metered = true.

(http://androidxref.com/4.4.4_r1/xref/packages/apps/Settings/src/com/android/settings/net/DataUsageMeteredSettings.java#140 which calls http://androidxref.com/4.4.4_r1/xref/packages/apps/Settings/src/com/android/settings/net/NetworkPolicyEditor.java#208 which creates a new policy).

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> Comment on attachment 8471643 [details] [diff] [review]
> Make auto-updater respect mobile hotspot setting
> 
> Review of attachment 8471643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks ok, but we don't want to break things for folks using ethernet (all
> three of them)
> 
> ::: mobile/android/base/updater/UpdateService.java
> @@ +220,4 @@
> >           */
> >          boolean shouldStartDownload = hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) ||
> >              autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_ENABLED ||
> > +            (autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_WIFI && !mConnectivityManager.isActiveNetworkMetered());
> 
> I think you need to keep the TYPE_ETHERNET bit in there
(Assignee)

Updated

4 years ago
Attachment #8471643 - Flags: review- → review?(snorp)
(In reply to Theodore Kokkoris [:tgkokk] from comment #5)

Indeed, I misread that line and thought we were checking that the connection type is WIFI. But we're just checking the autodownload policy.
Comment on attachment 8471643 [details] [diff] [review]
Make auto-updater respect mobile hotspot setting

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

Looks ok, but we don't want to break things for folks using ethernet (all three of them)

::: mobile/android/base/updater/UpdateService.java
@@ +220,4 @@
>           */
>          boolean shouldStartDownload = hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) ||
>              autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_ENABLED ||
> +            (autoDownloadPolicy == UpdateServiceHelper.AUTODOWNLOAD_WIFI && !mConnectivityManager.isActiveNetworkMetered());

I think you need to keep the TYPE_ETHERNET bit in there
Attachment #8471643 - Flags: review?(snorp) → review+
Not sure why it reposted my comments there...
(Assignee)

Comment 9

4 years ago
No, actually, you're right. We're checking both if the user is on WiFi and if they are not on a mobile network. We should only check if they aren't on a mobile network. Sorry, I'm going to fix that right away.
(Assignee)

Comment 10

4 years ago
(In reply to Theodore Kokkoris [:tgkokk] from comment #9)
> No, actually, you're right. We're checking both if the user is on WiFi and
> if they are not on a mobile network. We should only check if they aren't on
> a mobile network. Sorry, I'm going to fix that right away.

Sorry everyone for spamming, the patch is actually right.
(In reply to Theodore Kokkoris [:tgkokk] from comment #10)
> (In reply to Theodore Kokkoris [:tgkokk] from comment #9)
> > No, actually, you're right. We're checking both if the user is on WiFi and
> > if they are not on a mobile network. We should only check if they aren't on
> > a mobile network. Sorry, I'm going to fix that right away.
> 
> Sorry everyone for spamming, the patch is actually right.

Yeah, you basically changed the meaning from "Autodownload on WiFi (and ethernet)" to "Autodownload on unmetered connection". I'm alright with it.

Updated

4 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3d9d71e1de77
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34

Updated

4 years ago
Depends on: 1057626
This doesn't work on Gingerbread (isActiveNetworkMetered API 16).
You need to log in before you can comment on or make changes to this bug.