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.

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.

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

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.

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

Now with all files in the patch...

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.

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

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

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.

Rebased now that we can cancel appcache downloads.

Just rebased on current inbound

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.

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

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