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

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect, P1)

Description

[Julien Wajsberg [:julienw]]

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.

Comment 1

[Julien Wajsberg [:julienw]]

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.

Comment 2

[Julien Wajsberg [:julienw]]

Should also handle the file-output-stream created in Bug 820456.

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: 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.

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.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 5

[Julien Wajsberg [:julienw]]

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 ?

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

(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 :)

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: wip: tracking free space

Now with all files in the patch...

Comment 8

[[:fabrice] Fabrice Desré]

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.

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: patch

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.

Comment 10

[Julien Wajsberg [:julienw]]

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

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

(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.

Comment 12

[Julien Wajsberg [:julienw]]

Wondering if we should check the space during system update as well. But I don't know if this is doable right now.

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 14

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Rebased now that we can cancel appcache downloads.

Comment 15

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Just rebased on current inbound

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 17

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a64c9917f19e

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/a64c9917f19e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cd45b948227b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7bc7ea123b2f

Comment 21

[Jason Smith [:jsmith]]

Verified today doing some low space testing with appcache preloading for hosted apps and packaged apps.