Bug 853350 is going to add a fanotify-based component that will notify about a low device free space situation. We need to observe this notification and avoid any new offline cache download.

Needed as part of the blocking work in bug 853350 for protecting against low storage conditions and not bricking one's phone.

Blocking along with the other late low-storage bugs.

Honza, could you give me some feedback about this approach, please? Please, ignore the "sms-sent" and "sms-received" events. I am just using them for testing purposes. They are supposed to be changed by the events sent by the component build in bug 853350. That would be an event notifying about a low device storage situation and another event notifying about the recovery of that situation. I have several doubts about this approach. I am not sure if we are supposed to throw right away in nsOfflineUpdateService functions when we know that the update is gonna fail or if we better do that asynchronously (I am still not sure about how to do that). I've checked that there are calls to this functions that are not within a try-catch block. For example [1]. So we would need to fix that if we finally take this approach. Thanks!

Comment on attachment 738148 [details] [diff] [review] WIP Review of attachment 738148 [details] [diff] [review]: ----------------------------------------------------------------- ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp @@ +603,5 @@ > mDisabled = true; > } > > +#ifdef MOZ_WIDGET_GONK > + if (!strcmp(aTopic, "sms-received")) { as discussed on IRC, we'd also want to add some code at boot time to make sure the appcache service is running by the time any low-disk-space event is sent (so we don't miss the notification). We did this early init for cookies in bug 810209: the same approach would probably work here (unless it's very expensive to init the appcache service--I don't see an init function for it in nsApplicationCacheService.cpp, so I assume it's cheap).

Right! A different approach could be checking for a persistent flag that the free space component built in bug 853350 might expose during the initialization of the appcache service.

Comment on attachment 738148 [details] [diff] [review] WIP Olli, could you also provide some feedback about this approach, please?

(In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC, please) from comment #3) I've checked that there are calls to this > functions that are not within a try-catch block. For example [1]. So we > would need to fix that if we finally take this approach. I meant to add https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1191 as [1]

Comment on attachment 738148 [details] [diff] [review] WIP Review of attachment 738148 [details] [diff] [review]: ----------------------------------------------------------------- The approach seems correct to me. Early init and only notifications available are probably enough. However, I believe the low-disk-space service will also provide a global flag. So it's better IMO to use that flag to init mDisabled setting instead of an early init to catch all notifications. ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp @@ +290,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + rv = observerService->AddObserver(this, "sms-received", true); > + NS_ENSURE_SUCCESS(rv, rv); > +#endif I wonder why this should be limited to a single platform. We may need this be testable on any platform. I can imagine a test that will artificially invoke the notification to confirm updates are canceled and then no updates are accepted. @@ +353,5 @@ > nsIURI *aDocumentURI, > nsIDOMDocument *aDocument) > { > + if (mDisabled) > + return NS_ERROR_ABORT; 4 spaces indention.. @@ +603,5 @@ > mDisabled = true; > } > > +#ifdef MOZ_WIDGET_GONK > + if (!strcmp(aTopic, "sms-received")) { Jason, this is not naApplicationCacheService (nsIApplicationCacheService). This is service solely responsible for appcache updates. And it is cheap to instantiate. And I can see it is properly initialized in b2g shell. @@ +607,5 @@ > + if (!strcmp(aTopic, "sms-received")) { > + for (uint32_t i = 0; i < mUpdates.Length(); i++) { > + mUpdates[i]->Cancel(); > + } > + mDisabled = true; This code is wrong. You have to: - set the mDisabled flag to true first - cancel only the first update - clear the array (this will release all updates, no need to notify any consumers IIRC)

Is the patch here causing us to throw exceptions? It's better if we treat it as an asynchronous IO error.

Comment on attachment 738148 [details] [diff] [review] WIP Honza's feedback to this is more important than mine :)

> Is the patch here causing us to throw exceptions? It's better if we treat it as an asynchronous IO error. Yeah ferjm and I talked about this but were hoping it was ok to just throw :) For async failure we'll need to change to generate a nsIOfflineCacheUpdate as normal, but mark it so that it fails immediately (but asychronously) with the usual nsIOfflineCacheUpdateObserver notifications.

(In reply to Jonas Sicking (:sicking) from comment #9) > Is the patch here causing us to throw exceptions? It's better if we treat it > as an asynchronous IO error. Yes, but we should be catching that exceptions in chrome code, so we shouldn't be throwing any exception to content. Is that still unacceptable? In any case, I'll work on the alternative approach that Jason mentioned.

This is only a space watcher mock to ease the testing process. It emulates low storage notifications listening for headphones state changes.

Alternative approach. Not properly tested yet.

I've tested this approach and it seems to be working ok. With this patch, we asynchronously notify about a failure offline cache update even if we reach the low device storage situation while an update is running. I'll be working on automatic tests for this.

Comment on attachment 739471 [details] [diff] [review] v1 Review of attachment 739471 [details] [diff] [review]: ----------------------------------------------------------------- ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp @@ +1867,5 @@ > + if (lowFreeSpace) { > + mSucceeded = false; > + NotifyState(nsIOfflineCacheUpdateObserver::STATE_ERROR); > + return Finish(); > + } If low-disk-space notification comes during this update run, then the service will cancel the running update. There is no need for this additional check here. ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp @@ +284,5 @@ > true); > NS_ENSURE_SUCCESS(rv, rv); > > + // Observe low device storage notifications. > + rv = observerService->AddObserver(this, "disk-space-watcher", false); Why are you adding this as a hard referred observer? You may use weak referencing. Change the third arg to true. @@ +594,5 @@ > + if ((!strcmp(aTopic, "disk-space-watcher")) && > + (!nsCRT::strcmp(aData, NS_LITERAL_STRING("full").get()))) { > + if (mUpdates.Length() > 0) > + mUpdates[0]->Cancel(); > + } This code will just cancel the running update. It will finish asynchronously (correct) in LoadFinished() after an item finished load, but it will also trigger the next waiting update that first has to check the manifest change to get to the point it discovers we are low on disk space. Manifest load also involves some space allocation and generally speaking, it's wasting. As I'm thinking of it, you should cancel all updates here. This will set state of all of them to CANCELED. When you enter the nsOfflineCacheUpdate::Begin() method and the update state is CANCELED, you have to *asynchronously* (do a dispatch) trigger a failure notification you've added in ProcessNextURI. There is nsRunnableMethod for this.

You should also have a test for this. You can directly call the service Schedule method few times for always a different manifest (just use ?somecrap to have more URLs). Then in the "onchecking" event you should trigger artificial call to low-disk-space notification on the update service. You should receive the same number of "onerror" calls as you have scheduled the updates. There is a whole test suit in dom/tests/mochitest/ajax/offline to check on for examples.

(In reply to Honza Bambas (:mayhemer) from comment #18) > Comment on attachment 739471 [details] [diff] [review] > v1 > > Review of attachment 739471 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp > @@ +1867,5 @@ > > + if (lowFreeSpace) { > > + mSucceeded = false; > > + NotifyState(nsIOfflineCacheUpdateObserver::STATE_ERROR); > > + return Finish(); > > + } > > If low-disk-space notification comes during this update run, then the > service will cancel the running update. There is no need for this > additional check here. The think is that we might receive the notification while the update is not running. We need a way to avoid any write in the offline cache while we are in a low storage situation. > > ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp > @@ +284,5 @@ > > true); > > NS_ENSURE_SUCCESS(rv, rv); > > > > + // Observe low device storage notifications. > > + rv = observerService->AddObserver(this, "disk-space-watcher", false); > > Why are you adding this as a hard referred observer? You may use weak > referencing. Change the third arg to true. > Right! My mistake. > @@ +594,5 @@ > > + if ((!strcmp(aTopic, "disk-space-watcher")) && > > + (!nsCRT::strcmp(aData, NS_LITERAL_STRING("full").get()))) { > > + if (mUpdates.Length() > 0) > > + mUpdates[0]->Cancel(); > > + } > > This code will just cancel the running update. It will finish > asynchronously (correct) in LoadFinished() after an item finished load, but > it will also trigger the next waiting update that first has to check the > manifest change to get to the point it discovers we are low on disk space. > Manifest load also involves some space allocation and generally speaking, > it's wasting. > > As I'm thinking of it, you should cancel all updates here. This will set > state of all of them to CANCELED. When you enter the > nsOfflineCacheUpdate::Begin() method and the update state is CANCELED, you > have to *asynchronously* (do a dispatch) trigger a failure notification > you've added in ProcessNextURI. > > There is nsRunnableMethod for this. Ok, thanks! I'll do that.

(In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC, please) from comment #20) > (In reply to Honza Bambas (:mayhemer) from comment #18) > > Comment on attachment 739471 [details] [diff] [review] > > v1 > > > > Review of attachment 739471 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp > > @@ +1867,5 @@ > > > + if (lowFreeSpace) { > > > + mSucceeded = false; > > > + NotifyState(nsIOfflineCacheUpdateObserver::STATE_ERROR); > > > + return Finish(); > > > + } > > > > If low-disk-space notification comes during this update run, then the > > service will cancel the running update. There is no need for this > > additional check here. > > The think is that we might receive the notification while the update is not > running. > > We need a way to avoid any write in the offline cache while we are in a low > storage situation. The proposal I made for the Begin() method is taking care of it. Running updates will cancel in LoadFinished().

Thanks for the feedback Honza! I think this patch addresses your suggestions. I've successfully tested it with inbound and b2g18. I am currently working on making the mochitests run.

Comment on attachment 740826 [details] [diff] [review] v2 Doug, we may need to find somebody to provide quick feedback here.

Comment on attachment 740826 [details] [diff] [review] v2 Review of attachment 740826 [details] [diff] [review]: ----------------------------------------------------------------- The patch is OK. However, to give it r+ you must add a test.

Comment on attachment 740826 [details] [diff] [review] v2 Review of attachment 740826 [details] [diff] [review]: ----------------------------------------------------------------- ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp @@ +1769,5 @@ > nsCOMPtr<nsIOfflineCacheUpdate> kungFuDeathGrip(this); > > mItemsInProgress = 0; > > + if (mState == STATE_CANCELLED) { We forgot one important check here. When a device goes to a low disk space state and we schedule an update after that transition, the update will proceed. Updates should be Cancel()'ed prior call to Begin() on them when in low-disk-space state.

Comment on attachment 743783 [details] [diff] [review] Mochitest Review of attachment 743783 [details] [diff] [review]: ----------------------------------------------------------------- Add also a second test that will run an update AFTER we have transited to low-disk-space. Doing the artificial transition in a <head> script is OK, since updates are started after window.onload. ::: dom/tests/mochitest/ajax/offline/test_lowDeviceStorage.html @@ +35,5 @@ > + > +function onChecking() { > + var obs = SpecialPowers.Cc["@mozilla.org/observer-service;1"] > + .getService(SpecialPowers.Ci.nsIObserverService); > + obs.notifyObservers(null, "disk-space-watcher", "full"); Please notify the offline cache update service only. This affects the whole application and since this is a mochitest, you are actually not doing a proper clean up (you should notify "free" at the end of the test). You actually have to do the clean up anyway, there are more offline tests that would fail because of leaving the update service believe we are low-on-disk-space.

(In reply to Honza Bambas (:mayhemer) from comment #29) > Comment on attachment 740826 [details] [diff] [review] > v2 > > Review of attachment 740826 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp > @@ +1769,5 @@ > > nsCOMPtr<nsIOfflineCacheUpdate> kungFuDeathGrip(this); > > > > mItemsInProgress = 0; > > > > + if (mState == STATE_CANCELLED) { > > We forgot one important check here. > > When a device goes to a low disk space state and we schedule an update after > that transition, the update will proceed. > > Updates should be Cancel()'ed prior call to Begin() on them when in > low-disk-space state. That was my point in comment 20 :).

Can somebody explain this bug to triage? What user scenario is this associated with? Are we trying to prevent downloading new appcache resources for offline apps when low on space?

(In reply to Alex Keybl [:akeybl] from comment #32) > Are we trying to prevent downloading new appcache resources for offline apps > when low on space? Yes

This patch includes the check for the disk space status before an update begins, so it can be properly canceled in advance.

This patch adds a new mochitest that schedules an update *after* a low disk space situation is notified. It also adds the suggested clean up notifying "free" at the end of both mochitests. Thanks for the feedback, Honza!

Comment on attachment 745070 [details] [diff] [review] v3 Review of attachment 745070 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with the comments addressed. Disclaimer: not tested locally. ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp @@ +1783,5 @@ > + NS_NewRunnableMethod(this, &nsOfflineCacheUpdate::NotifyError); > + nsresult rv = NS_DispatchToMainThread(errorNotification); > + NS_ENSURE_SUCCESS(rv, rv); > + > + return Finish(); Put also Finish() to the async method and rename your new method to AsyncFinish() or so. Here return just NS_OK. ::: uriloader/prefetch/nsOfflineCacheUpdate.h @@ +355,5 @@ > nsTArray<nsRefPtr<nsOfflineCacheUpdate> > mUpdates; > > bool mDisabled; > bool mUpdateRunning; > + bool mLowFreeSpace; Unused. ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp @@ +415,5 @@ > + if (!mLowFreeSpace) { > + nsCOMPtr<nsIDiskSpaceWatcher> diskSpaceWatcherService = > + do_GetService("@mozilla.org/toolkit/disk-space-watcher;1"); > + diskSpaceWatcherService->GetLowFreeSpace(&mLowFreeSpace); > + } Should we just load mLowFreeSpace at nsOfflineCacheUpdateService::Init() ? @@ +421,4 @@ > if (mUpdates.Length() > 0) { > mUpdateRunning = true; > + if (mLowFreeSpace) { > + mUpdates[0]->Cancel(); Add a comment here that canceling the update before Begin() call will make the update asynchronously finish with an error.

Comment on attachment 745071 [details] [diff] [review] Mochitests - v2 Review of attachment 745071 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab Sorry for the delay ::: dom/tests/mochitest/ajax/offline/test_lowDeviceStorage.html @@ +13,5 @@ > + * storage situation appears is canceled. It basically does: > + * > + * 1. Notifies to the offline cache update service about a fake > + * low device storage situation. > + * 2. Schedules an update an observe for its notifications. update and observe @@ +49,5 @@ > + aUpdate.removeObserver(this); > + OfflineTest.ok(false, "No update"); > + finish(); > + break; > + case Ci.nsIOfflineCacheUpdateObserver.STATE_FINISHED: FINISHED is I think expected...

https://hg.mozilla.org/projects/birch/rev/6d2e81a6bf1b https://hg.mozilla.org/projects/birch/rev/04623bf25b64

Backed out for Windows bustage. https://hg.mozilla.org/projects/birch/rev/07f18a9becbe https://tbpl.mozilla.org/php/getParsedLog.php?id=22819987&tree=Birch

New try run https://tbpl.mozilla.org/?tree=Try&rev=18f6afd6825d

http://hg.mozilla.org/projects/birch/rev/81e072111e98 http://hg.mozilla.org/projects/birch/rev/d0aa77e7825b

Backed out for mochitest failures. https://hg.mozilla.org/projects/birch/rev/519b04170b2f https://tbpl.mozilla.org/php/getParsedLog.php?id=22835525&tree=Birch

I fixed the permissionManager problem but it seems like there is more to fix :( https://tbpl.mozilla.org/?tree=Try&rev=461f78dcb971

It seems that offline cache mochitests are disabled for b2g https://hg.mozilla.org/mozilla-central/file/4ff8aa4a7295/testing/mochitest/b2g.json#l213 :(

Should I add this test to the mochitest blacklist for b2g and file a follow-up bug to get all these tests fixed?

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #45) > Should I add this test to the mochitest blacklist for b2g and file a > follow-up bug to get all these tests fixed? That sounds reasonable to me.

http://hg.mozilla.org/projects/birch/rev/044d554846ff http://hg.mozilla.org/projects/birch/rev/6380de732887 I filed bug 871323

https://hg.mozilla.org/mozilla-central/rev/044d554846ff https://hg.mozilla.org/mozilla-central/rev/6380de732887

Does just the one b2g18 patch need to land?

No, the same patches that were pushed to birch and m-c needs to land in b2g18. Sorry for the confusion, the "Space watcher mock (b2g18/inbound)" patch were only needed for testing purposes. I've marked them as obsolete.

Are the tests going to pass on b2g18?

https://hg.mozilla.org/releases/mozilla-b2g18/rev/606e986085a5 https://hg.mozilla.org/releases/mozilla-b2g18/rev/132656b8a33e https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/be22e17717f9 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/08196b630a4a

Confirmed to be tested, but this patch definitely doesn't work for me. See bug 877356 for a followup.

Which device are you testing with? Note that you need the kernel patch to enable fanotify.

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #54) > Which device are you testing with? Note that you need the kernel patch to > enable fanotify. Clarified in IRC. I was testing on unagi, which does not have that enabled. I'll test this on Inari next.

Turns out I can reproduce this on Inari as well, which should have the fanotify patch. Filed bug 877499 as a followup.

Apparently both inari and unagi don't have the fanotify kernel patch.

Sorry I wasn't clear about this Jason. Comment 54 was done through mobile in a rush and I couldn't explain too much. It seems that Ikura devices have the fanotify kernel patch. Also I've been testing with a Keon with a custom boot.img that includes the kernel patch that I can share with you if you need it.

Finally got this working on a partner eng build from 6/5. Sorry for the confusion here. Sanity test looked okay where I tried to write a 20 MB appcache via a HTML page with 24 MB left - when I hit the 5 MB limit, the appcache I tried to write was rolled back and I got a toast indicating the low storage warning with 24 MB left.

lgtm on a 6/6 partner 1.01 build. I did tests around: - Ensuring the low storage notification fires hitting the 5 MB limit - Ensuring that you can write appcache resources in between 10 MB and 5 MB before you hit the 5 MB limit - Ensure you cannot write appcache resources in between 10 MB and 5 MB after hitting the 5 MB limit - Ensuring that after you hit the above 10 MB limit again after hitting 5 MB, you can write appcache resources