Based on https://bugzilla.mozilla.org/show_bug.cgi?id=816128#c6 we need to expose nsOfflineCacheUpdate::Cancel() [1] in order to cancel an on going appcache download from the Apps API implmentation. [1] http://mxr.mozilla.org/mozilla-central/source/uriloader/prefetch/nsOfflineCacheUpdate.cpp#1811

I am not sure if the patch is correct since the download is cancelled, but I am getting this assertion: "-*-*- Webapps.jsm : ************* ABOUT TO CALL CANCEL [Parent 76547] ###!!! ASSERTION: ProcessNextURI should only be called from the DOWNLOADING state: 'mState == STATE_DOWNLOADING', file /Volumes/Data/dev/mozilla/mozilla-inbound/uriloader/prefetch/nsOfflineCacheUpdate.cpp, line 1868"

Add "if (mState == STATE_CANCELLED) return NS_ERROR_NOT_INITIALIZED;" before that line to nsOfflineCacheUpdate::ProcessNextURI()

Thanks Honza! I also had to expose a new state in nsIOfflineCacheUpdateObserver to notify the observers about the cancellation.

Sorry for the spam, wrong patch :\

The attached patch has been successfully tested with the WIP patch from Bug 819974

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #4) > I also had to expose a new state in nsIOfflineCacheUpdateObserver to notify > the observers about the cancellation. Why is this needed? I didn't check deeper but my first thought was to abstract this as an error state since canceled update was actually faulty - rollbacks updated data and leads to applicationCache.onerror event trigger. Otherwise I think this will break the contract about expected even notifications. checking, downloading and progress notifications might already been fired, so there is expected either cached, updateready or error notification. But that should already be ensured while calling Finish() method of canceled updated. Also, I don't see any consumers of the new state in your patch. It this needed for some followup work? I will ask you to add a test that will catch the expected notification (onerror). You can use e.g. the test for parallelism as a template. Otherwise the patch looks good. I will do a final review later tomorrow.

Thanks Honza! You are right, there is no need for exposing a new state, I can reuse the error state. However I still need to notify about the error before calling Finish(). I've updated the patch according to that. I'll write some tests for it in another patch as soon as I localize where these tests live :).

Comment on attachment 692278 [details] [diff] [review] v3 Review of attachment 692278 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with comments addressed. ::: uriloader/prefetch/nsIOfflineCacheUpdate.idl @@ +184,5 @@ > */ > void removeObserver(in nsIOfflineCacheUpdateObserver aObserver); > > /** > + * Cancell all pending downloads. "Cancel the update when still in progress. This stops all running resource downloads and discards the downloaded cache version. Throws when update has already finished and made the new cache version active." ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp @@ +1872,2 @@ > NS_ASSERTION(mState == STATE_DOWNLOADING, > "ProcessNextURI should only be called from the DOWNLOADING state"); Hmm.. we may also get here when the state gets to STATE_FINISHED I think and that was the reason for this assertion failure on try IMO. I more and more think of removing this assertion at all, since we have more and more cases when call to this method out of STATE_DOWNLOADING is legal. Please remove it (including your new mState == STATE_CANCELLED early return) @@ +2349,5 @@ > > NS_IMETHODIMP > +nsOfflineCacheUpdate::Cancel() > +{ > + LOG(("nsOfflineCacheUpdate::Cancel [%p]", this)); We should throw when state is STATE_FINISHED or STATE_CANCELED already to tell the consumers they are late with canceling.

(In reply to Honza Bambas (:mayhemer) from comment #9) > @@ +2349,5 @@ > > > > NS_IMETHODIMP > > +nsOfflineCacheUpdate::Cancel() > > +{ > > + LOG(("nsOfflineCacheUpdate::Cancel [%p]", this)); > > We should throw when state is STATE_FINISHED or STATE_CANCELED already to > tell the consumers they are late with canceling. I'm wondering if this could not just be a silent catch ?

(In reply to Julien Wajsberg [:julienw] from comment #10) > I'm wondering if this could not just be a silent catch ? Maybe, depends on what consumer do in case the update is already done. If there is a believe call to cancel() has done some kind of cleanup but that didn't happen, then we may get to trouble. It's a suggestion - therefor the SHOULD word - so, if you have arguments against, feel free to present them here.

Thinking more about this, throwing an exception seems better because it gives more options to the consumers. So forget about my suggestion. Let's just make sure that the consumers catch this exception so please open new bugs for Gaia.

(In reply to Julien Wajsberg [:julienw] from comment #12) > Thinking more about this, throwing an exception seems better because it > gives more options to the consumers. > > So forget about my suggestion. Let's just make sure that the consumers catch > this exception so please open new bugs for Gaia. The exception won't be exposed to content and will be, for the Gaia use case, handled by the Apps API implementation in Bug 819974

Can we land this?

(In reply to Andrew Overholt [:overholt] from comment #14) > Can we land this? As soon as I get the r+ for the tests :)

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #16) > (In reply to Andrew Overholt [:overholt] from comment #14) > > Can we land this? > > As soon as I get the r+ for the tests :) :)

Comment on attachment 693148 [details] [diff] [review] Tests Review of attachment 693148 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/ajax/offline/Makefile.in @@ +97,5 @@ > test_xhtmlManifest.xhtml \ > test_missingManifest.html \ > missing.html \ > jupiter.jpg \ > + test_cancelOfflineCache.html \ Check indention. There is a tab char, not spaces. ::: dom/tests/mochitest/ajax/offline/test_cancelOfflineCache.html @@ +32,5 @@ > +function onProgress () { > + var i = 0; > + while (i < updateService.numUpdates) { > + var update = updateService.getUpdate(i); > + if (update.manifestURI.path == manifestURI.path) { compare .spec might be better.

Thanks Honza! https://hg.mozilla.org/integration/mozilla-inbound/rev/921863b9cd28 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d0ac42bacd

https://hg.mozilla.org/mozilla-central/rev/921863b9cd28 https://hg.mozilla.org/mozilla-central/rev/a0d0ac42bacd

https://hg.mozilla.org/releases/mozilla-aurora/rev/18637d4be2a1 https://hg.mozilla.org/releases/mozilla-aurora/rev/10b241489bb6 https://hg.mozilla.org/releases/mozilla-b2g18/rev/7eddf6772d61 https://hg.mozilla.org/releases/mozilla-b2g18/rev/99c102e33068