819971 - Expose nsOfflineCacheUpdate::Cancel() via nsIOfflineCacheUpdate.idl

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

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"

Comment 3

[Honza Bambas (:mayhemer)]

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

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Thanks Honza!

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

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Sorry for the spam, wrong patch :\

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 7

[Honza Bambas (:mayhemer)]

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

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

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 9

[Honza Bambas (:mayhemer)]

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.

Comment 10

[Julien Wajsberg [:julienw]]

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

Comment 11

[Honza Bambas (:mayhemer)]

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

Comment 12

[Julien Wajsberg [:julienw]]

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.

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

(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

Comment 14

[Andrew Overholt [:overholt]]

Can we land this?

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Andrew Overholt [:overholt] from comment #14)
> Can we land this?

As soon as I get the r+ for the tests :)

Comment 17

[Andrew Overholt [:overholt]]

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

[Honza Bambas (:mayhemer)]

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.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Thanks Honza!

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

Comment 20

[Ed Morley [:emorley]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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