1331808 - Firefox (Android) used several GB of mobile/wifi data in background overnight

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: FirefoxAndroid-DataUsage.png

On Firefox for Android Beta 51.0b13 buildid 20170110094327 with *zero* tabs open. Overnight the Android system alerts that my data plan has reached the soft limit I set. Initially I thought maybe MLS was using data in the background so I disabled it. Yesterday I forgot to switch on wifi connection again. Overnight the Android system again alerts that all my data was used. I also checked the wifi usage and it shows the same data usage pattern.

I haven't done any heavy browsing on this phone so it's impossible to use almost 10G worth of data in this short time.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: FirefoxAndroid-WifiUsage.png

Comment 2

[Richard Newman [:rnewman]]

Do you have a Firefox Account configured?

What tabs did you have open before you closed them?

Do you have any add-ons installed?

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Richard Newman [:rnewman] from comment #2)
> Do you have a Firefox Account configured?

Yes.

> What tabs did you have open before you closed them?

I don't remember but from the history "Recently closed" there are pages from:

https://www.gouletpens.com
https://www.fujirumors.com
https://crash-stats.mozilla.com

> Do you have any add-ons installed?

No. But I installed a lightweight theme "Japanese Tattoo" yesterday when I was checking if I have any add-ons installed.

Comment 4

[:Grisha Kruglov]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #3)
> (In reply to Richard Newman [:rnewman] from comment #2)
> > Do you have a Firefox Account configured?
> 
> Yes.

Pretty unlikely, but perhaps Bug 1324600 might be relevant, if device is/was on the go-sync server. Clients would start uploading a collection, fail after two batches, do this for every collection, and then try again... Server-side fix was deployed on Jan 11th, so attached data usage image is within the affected timeframe.

Comment 5

[Rachelle Yang [:ryang][[e-mail]]]

Is this bug similar to Bug 1273552 ? are they relevant ?

Thank you !!

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Rachelle Yang [:ryang][[e-mail]] from comment #5)
> Is this bug similar to Bug 1273552 ? are they relevant ?

It's unlikely because I don't have any tabs open.

I captured the network traffic on the phone and find Firefox is constantly requesting a image from a proxy server at the rate about once per second. The request looks like this:

==> connect to cloudproxy10002.sucuri.net
GET /wp-content/uploads/new-logo57.jpg HTTP/1.1
User-Agent: Mozilla/5.0 (Android 7.1.1; Mobile; rv:51.0) Gecko/51.0 Firefox/51.0
Host: www.fujirumors.com
Connection: Keep-Alive
Accept-Encoding: gzip

The response size is 3,323 bytes so it's about 11.25 MiB per hour. It's not fast enough to consume 1 GiB data per night but maybe if there are more requests like this they can pile up.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #6)
> The response size is 3,323 bytes so it's about 11.25 MiB per hour. It's not
> fast enough to consume 1 GiB data per night but maybe if there are more
> requests like this they can pile up.

The rate was wrong. Actually I can tell from the captured packets, it's 25 MiB per hour.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

The image is linked as apple-touch-icon-precomposed in the page's markup and after enabling debugging I can see our icon code downloading the icon over and over again. I'll take the bug and debug.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

This one is interesting.

We are storing the icon URLs (and some other metadata) in a TreeSet with a custom Comparator so that we can cycle through them from potentially best icon to potentially worst icon.

new-logo57.jpg from above is the first icon we try [1] We fail to decode it (maybe that's worth investigating in an other bug too) and want to remove it from the set [2]. However removing fails and we continue with the next icon - but that's the same icon again - and so on.

The culprit is our custom Comparator: If we do not know which icon is better then we just return 1 [3]. For our main use case, ordering the icons, it does not matter. However when removing an icon the comparator is used too and because the set is ordered there's an optimization to stop searching for the item to remove if we moved past its assumed position. The solution is to always pick and return a consistent order from the comparator - for example by using hashCode().

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java#76
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java#167
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/icons/IconDescriptorComparator.java#46-47

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1331808 - IconDescriptorComparator: Return consistent order for compared items.

Previously if we could not compare two icon descriptions we'd always return the "right" one. This does
not create a consistent order if the parameters are flipped. As a result some operations on a TreeSet
can fail (like remove()).

Review commit: https://reviewboard.mozilla.org/r/105512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105512/

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1331808 - IconRequest.moveToNextIcon(): Throw exception if removing current item failed.

Review commit: https://reviewboard.mozilla.org/r/105514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105514/

Comment 12

[Richard Newman [:rnewman]]

This would make for an excellent little blog post, Sebastian! Yikes.

Comment 13

[:Grisha Kruglov]

Comment on attachment 8827941 [details]
Bug 1331808 - IconRequest.moveToNextIcon(): Throw exception if removing current item failed.

https://reviewboard.mozilla.org/r/105514/#review106382

Comment 14

[:Grisha Kruglov]

Comment on attachment 8827940 [details]
Bug 1331808 - IconDescriptorComparator: Return consistent order for compared items.

https://reviewboard.mozilla.org/r/105512/#review106394

Changes lgtm; curious little bug.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8827940 [details]
Bug 1331808 - IconDescriptorComparator: Return consistent order for compared items.

It might be too late for 51 because merge day is on Monday. However I still try to request uplift because this can be a very big issue for affected users.

Approval Request Comment

[Feature/Bug causing the regression]: This specific class was introduced in Firefox 51 (bug 1290014). However this code was moved around and existed before. So it might be possible that this issue existed way longer.

[User impact if declined]: In some limited scenarios our icon downloading code can get into an endless loop and will download the same icon over and over again until the app is killed by the user or the system. This can have an disastrous impact on the data plan of the user.

[Is this code covered by automated tests?]: I added new unit tests for this scenario and the new code.

[Has the fix been verified in Nightly?]: Not yet, code just landed. I verified that it's now working with the reported site. Additionally the second patch makes sure that the app will crash if it's stuck in an endless loop.

[Needs manual test from QE? If yes, steps to reproduce]: This is hard to test without enabling logging in the icon code or looking at the network traffic.

[List of other uplifts needed for the feature/fix]: Patch 1 (608b4954ab8f2fdab0c4504016c92b364f7c7449) in this bug fixes the issue. Patch 2 (b69410e3e57ec64840a9b880ff1c6cb5328d6389) throws an exception if we get stuck again. It would be an option to just uplift patch 1 if we want to avoid the exception and a potential crash in release (unlikely!) without more time; although I think that a crash might be a better option than burning through the user's data plan.

[Is the change risky?]: [Why is the change risky/not risky?]: In the worst case this could introduce a crash in the icon download code. However this does not seem to be a likely scenario. Some more time in Nightly would give us more confidence; but merge day is coming. Overall I still think the risk is low: Our icon code can recover from errors. In Nightly we usually crash, but in release versions we will move on and skip loading the current icon:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/icons/IconTask.java#153

[String changes made/needed]: -

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8827941 [details]
Bug 1331808 - IconRequest.moveToNextIcon(): Throw exception if removing current item failed.

See comment 15.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Somehow the pulsebot comments are missing although I landed the patch via mozreview. Anyways, here are the commits:

* Check-in: https://hg.mozilla.org/integration/autoland/rev/5c43a389e735 - Sebastian Kaspari - Bug 1331808 - IconDescriptorComparator: Return consistent order for compared items. r=Grisha

* Check-in: https://hg.mozilla.org/integration/autoland/rev/bc9d8f30b7f0 - Sebastian Kaspari - Bug 1331808 - IconRequest.moveToNextIcon(): Throw exception if removing current item failed. r=Grisha

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c43a389e735
https://hg.mozilla.org/mozilla-central/rev/bc9d8f30b7f0

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 51+

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8827940 [details]
Bug 1331808 - IconDescriptorComparator: Return consistent order for compared items.

We might consider doing another Fennec RC build for this specific issue.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Blocker for 51 fennec release, we'll have to uplift this to m-r and do a mobile RC3.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8827940 [details]
Bug 1331808 - IconDescriptorComparator: Return consistent order for compared items.

Yeah this is super bad. Let's get this patch into m-r as soon as possible today and start an RC3 build for fennec only.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Both patches, I think, for uplift.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb526a55f19a
https://hg.mozilla.org/releases/mozilla-aurora/rev/338a1630d1db

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/70f25d2bdbe0
https://hg.mozilla.org/releases/mozilla-release/rev/a67a1682be8f

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

No need to track anymore.

Comment 27

[Richard Newman [:rnewman]]

Fixed in 51, so tracking 51+ to match.