612703 - Downloading a 1 gig file will cause the Droid 2 to run out of memory and reboot

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1. go to the app store and install SysMonitor
2. go to http://www.thinkbroadband.com/download.html
3. long tap the 1 gb file and save
4. open the System Notifications
5. wait

Expected: the device would say that there's not enough space
Actual: the device will try to download and reboot after a while.

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101116 Firefox/4.0b8pre Fennec/4.0b3pre

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes, do we get the right file size? Does getAvailableSpace just not work?

Comment 3

[Wesley Johnston (:wesj)]

I do see a number coming from getAvailableSpace that seems reasonable. Trying to reproduce...

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

why is this Mozilla confidential?

Comment 5

[Wesley Johnston (:wesj)]

Not sure. If I did that it was an unintentional click.

I did eventually see the device reboot, even though I have plenty of space available on my SD card. (> 14GB)

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Logcat

Attaching a logcat report from the download. There's probably a lot there we don't care about, but I wanted to save it somewhere. At a first glance, this looks like an OOM (not disk space) problem to me.

Comment 7

[Wesley Johnston (:wesj)]

This is likely due to progressbars in the notification we are using. A simple script that just creates a progress notification and increments it a whole bunch over a few minutes will also eventually freeze. Likewise, there are some reports on stack overflow about similar problems:

http://stackoverflow.com/questions/3390280/android-memory-leak-in-notification-service
http://stackoverflow.com/questions/3608684/huge-memory-usage-in-notifications

Still building to test. If this is the problem, maybe we limit updates to the notifications (i.e. only update the bar if the change is greater than x%)?

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This changes the progress bars to only update when a certain percentage change has occurred (1% here), or when the text changes (trying to make sure we support changing the text without changing the percentage and vice versa).

I have run some simple tests with faked progress bars here and this seems fine. Trying a download with a larger file (ubuntu iso) now.

Comment 9

[Alon Zakai (:azakai)]

Good catch!

From those links it seems this is indeed a bug in the Android OS. I can't find a bug in their tracker for it though, perhaps we should file one, once we are sure that is the issue here.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 494055 [details] [diff] [review]
Patch v1


>         String text;
>+        int percent = 0;
>+        if (aProgressMax > 0)
>+            percent = (int)(100 * aProgress / aProgressMax);

While you're here, please use NumberFormat.getPercentInstance().format(aProgress / aProgressMax); 

r+ other than that, but I want to double check your formatting code

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 494055 [details] [diff] [review]
Patch v1


>             text = percent + "%";
err.... I meant use that here. I think percent should be a double for it to work though.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Revised to use NumberFormat stuff.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch v2 again

Arrgh. Forgot to mark as a patch.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 494406 [details] [diff] [review]
Patch v2 again

as discussed on irc, percent ranges from 0 to 1, but the threshold is 1

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Sorry bout that. Testing this right now, but it has been running for around 15 minutes (the numbers update and everything now) and my system is still responsive.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 494448 [details] [diff] [review]
Patch v3


>+        if (!mPrevAlertText.equals(text) || Math.abs(mPrevPercent - percent) >= UPDATE_THRESHOLD) {

one last nit, just return early here

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Fix v4

Reversed the logic of the if. Tested again with my little manual script.

Comment 18

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/5ae1f2fa0d9f

Comment 19

[Anna (Waverley)]

I can't verify this bug because I get "The connection has timed out" page.. Does someone else see this too?

Comment 20

[Catalin Suciu [:csuciu]]

I verified this on Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110911 Firefox/9.0a1 Fennec/9.0a1.
1. The device is not rebooted anymore
2. The message saying that "there's not enough space" is still missing. Instead, an empty message box is displayed.(for this issue, bug #686286 was logged)