We should keep a small space on storage during download/install

VERIFIED FIXED in Firefox 19

Status

defect
P1
major
VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: julienw, Assigned: fabrice)

Tracking

Trunk
B2G C3 (12dec-1jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

7 years ago
I'm still testing out-of-space use cases.

I think we should keep a safety space for download and install. Otherwise we fill up the /data partition and the phone becomes broken (broken homescreen especially) so this is very bad (AbortError (homescreen's state.js line 142) and UnknownError (homescreen's state.js line 139)).

There are basically two different cases :

1. packaged apps with a 'size' property in update manifest: we check the available free space before downloading. So we should take into account a safety space in the check.

2. during download, we fail with a downloaderror when there is no space left. But I think this is too late because then the phone is unusable. We should fail before that and take into account a safety space.

Nominating for bb+ because otherwise the phone can become a pile of sh^H^Hbrick.
Reporter

Updated

7 years ago
Severity: normal → major
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM

Updated

7 years ago
blocking-basecamp: ? → +

Updated

7 years ago
Assignee: nobody → fabrice
Priority: -- → P1
Target Milestone: --- → B2G C3 (12dec-1jan)
Assignee: fabrice → ferjmoreno
Depends on: 819974
No longer depends on: 819971
Reporter

Comment 1

7 years ago
I think that if we hit this small space limit, the platform should trigger UNSUFFICIENT_ERROR so that the already implemented dialog about this error would be displayed and the user would have the correct information.
Reporter

Comment 2

7 years ago
Should also handle the file-output-stream created in Bug 820456.
Assignee

Comment 3

7 years ago
Posted patch wip: tracking free space (obsolete) — Splinter Review
To solve this issue properly, I think we need to monitor free space and cancel downloads when we go below some free space limit.

This patch implements a simple free space watcher, by polling the devicestorage for apps. There's no other way to be notified without polling unfortunately.

To not poll constently, we need to only poll when we have downloads (packages or appcache) in progress, and cancel the timer once they are done.
Thanks Fabrice!

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Created attachment 692093 [details] [diff] [review]
> wip: tracking free space
> 
> To solve this issue properly, I think we need to monitor free space and
> cancel downloads when we go below some free space limit.
> 
> This patch implements a simple free space watcher, by polling the
> devicestorage for apps. There's no other way to be notified without polling
> unfortunately.
> 

Did you forget to include FreeSpaceWatcher.jsm in your patch :) ?

> To not poll constently, we need to only poll when we have downloads
> (packages or appcache) in progress, and cancel the timer once they are done.

I think that we might not need to poll for packages app downloads since we know in advance the size of the download. We would need to watch if the download is higher than the size that the manifest says and, in that case, cancel the download or start polling.
Reporter

Comment 5

7 years ago
ferjm> we need to account for the hosted-apps-with-appcache case as well.

fabrice> just so I understand correctly, you can not ask for the available free space at each progress because this is an asynchronous request, that's it ?
(In reply to Julien Wajsberg [:julienw] from comment #5)
> ferjm> we need to account for the hosted-apps-with-appcache case as well.

Of course, I was just referring to an exception for packaged apps :)
Assignee

Comment 7

7 years ago
Posted patch wip: tracking free space (obsolete) — Splinter Review
Now with all files in the patch...
Attachment #692093 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Assignee: ferjmoreno → fabrice
Assignee

Comment 8

7 years ago
Julien: yes, stat() is async, so not usable during the onprogress handler itself.

Fernando: we need that even for packaged apps I think. Consider the following use case:
1) We start with 150MB of free space
2) User installs packaged app, size 100MB - we start the download
3) User installs another app, size 100MB - we start the download also, since we have not yet downloaded much of the first app.

At some point, downloading one of the two apps will fail. And we could also be downloading some appcache...

I'd rather have a real notification than polling, but if our threshold is bigger that what we can write to disk during the polling interval, we are ok there.
Assignee

Comment 9

7 years ago
Posted patch patch (obsolete) — Splinter Review
This patch works fine for me, but this kind of error condition is hard to trigger so I'd like to have your testing feedback.
Attachment #692698 - Attachment is obsolete: true
Attachment #693174 - Flags: feedback?(ferjmoreno)
Attachment #693174 - Flags: feedback?(felash)
Reporter

Comment 10

7 years ago
Comment on attachment 693174 [details] [diff] [review]
patch

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

works ok for packaged app (we have the error before installing it, that's good) but not for hosted app (it should stop during the download at one point, right ?).

I think we should also check if (available_freespace < min_remaining_freespace) and not downloading if this is the case, whether hosted_appcache or packaged.

::: dom/apps/src/AppDownloadManager.jsm
@@ +13,5 @@
> +this.EXPORTED_SYMBOLS = ["AppDownloadManager"];
> +
> +// Minimum disk free space we want to keep, in bytes.
> +// Keep synchronized with Webapps.jsm
> +const MIN_REMAINING_FREESPACE = 5 * 1024 * 1024;

couldn't this be a property of the AppDownloadManager ? You wouldn't need to synchronize it between this file and Webapps.jsm

::: dom/apps/src/Webapps.jsm
@@ +1141,5 @@
>        let manifest = new ManifestHelper(aManifest, app.origin);
>  
>        app.installState = "installed";
>        app.downloading = false;
> +      app.downloadSize = 0;

well spotted
Attachment #693174 - Flags: feedback?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #10)
> Comment on attachment 693174 [details] [diff] [review]
> patch
> 
> Review of attachment 693174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> works ok for packaged app (we have the error before installing it, that's
> good) but not for hosted app (it should stop during the download at one
> point, right ?).
> 

Actually, it can't stop the hosted app download until Bug 819974 lands. That's why I wasn't working on this yet :).

(In reply to Fabrice Desré [:fabrice] from comment #8)
> Fernando: we need that even for packaged apps I think. Consider the
> following use case:
> 1) We start with 150MB of free space
> 2) User installs packaged app, size 100MB - we start the download
> 3) User installs another app, size 100MB - we start the download also, since
> we have not yet downloaded much of the first app.
> 
> At some point, downloading one of the two apps will fail. And we could also
> be downloading some appcache...
> 
> I'd rather have a real notification than polling, but if our threshold is
> bigger that what we can write to disk during the polling interval, we are ok
> there.

Makes sense.

I am currently building to test you patch.
Reporter

Comment 12

7 years ago
Wondering if we should check the space during system update as well. But I don't know if this is doable right now.
Comment on attachment 693174 [details] [diff] [review]
patch

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

I was finally able to test it and I can also confirm that it is working, with the hosted app exception mentioned in previous comments.
I would test it again once Bug 819974 lands.
Attachment #693174 - Flags: feedback?(ferjmoreno) → feedback+
Assignee

Comment 14

7 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Rebased now that we can cancel appcache downloads.
Attachment #693174 - Attachment is obsolete: true
Attachment #694043 - Flags: review?(ferjmoreno)
Assignee

Comment 15

7 years ago
Posted patch patch v2Splinter Review
Just rebased on current inbound
Attachment #694043 - Attachment is obsolete: true
Attachment #694043 - Flags: review?(ferjmoreno)
Comment on attachment 694525 [details] [diff] [review]
patch v2

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

Awesome patch Fabrice! Only a few nits.
I couldn't test the patch today, but I'll do it again tomorrow, now that we can check both packaged and hosted apps.

::: dom/apps/src/AppDownloadManager.jsm
@@ +52,5 @@
> +    this.downloads[aManifestURL] = aDownload;
> +  },
> +
> +  /**
> +   * Retrives a download from the list of current downloads.

typo: 'Retrieves'

::: dom/apps/src/FreeSpaceWatcher.jsm
@@ +11,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +this.EXPORTED_SYMBOLS = ["FreeSpaceWatcher"];
> +
> +

nit: remove this extra line, please.

@@ +23,5 @@
> +this.FreeSpaceWatcher = {
> +  timers: {},
> +  id: 0,
> +
> +  /** This function  will call aOnStatusChange

nit: 
/**
 * This function ...

@@ +40,5 @@
> +    debug("Creating new FreeSpaceWatcher");
> +    let callback = {
> +      currentStatus: null,
> +      notify: function(aTimer) {
> +        try {

nit: indentation is incorrect in the try-catch block.

::: dom/apps/src/Webapps.jsm
@@ +1750,5 @@
>  
>        let requestChannel = NetUtil.newChannel(aManifest.fullPackagePath())
>                                    .QueryInterface(Ci.nsIHttpChannel);
> +      AppDownloadManager.add(aApp.manifestURL,
> +          {channel: requestChannel,

nit: leave the { in the line above, please.
Attachment #694525 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a64c9917f19e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith

Updated

7 years ago
Duplicate of this bug: 818909
Verified today doing some low space testing with appcache preloading for hosted apps and packaged apps.
Status: RESOLVED → VERIFIED
Keywords: verifyme

Updated

2 years ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.