Closed Bug 1052100 Opened 10 years ago Closed 10 years ago

Auto-update doesn't respect mobile hotspot setting

Categories

(Firefox for Android Graveyard :: General, defect)

34 Branch
ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: tgkokk, Assigned: tgkokk)

References

Details

Attachments

(1 file, 1 obsolete file)

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).
OS: Mac OS X → Android
Hardware: x86 → ARM
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8471583 - Flags: review?(snorp)
Assignee: nobody → t.kokkoris
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.
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-
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
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...
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.
(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.
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3d9d71e1de77
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Depends on: 1057626
This doesn't work on Gingerbread (isActiveNetworkMetered API 16).
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.