Expose nsOfflineCacheUpdate::Cancel() via nsIOfflineCacheUpdate.idl

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

Trunk
B2G C3 (12dec-1jan)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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
Blocks: 819974
blocking-basecamp: --- → ?
Blocks: 818848
No longer blocks: 818848
Assignee: nobody → ferjmoreno
Posted patch v1 (obsolete) — Splinter Review
Attachment #690460 - Flags: review?(honzab.moz)
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"
Attachment #690460 - Flags: review?(honzab.moz) → feedback?(honzab.moz)
Add "if (mState == STATE_CANCELLED) return NS_ERROR_NOT_INITIALIZED;" before that line to nsOfflineCacheUpdate::ProcessNextURI()
Posted patch v2 (obsolete) — Splinter Review
Thanks Honza!

I also had to expose a new state in nsIOfflineCacheUpdateObserver to notify the observers about the cancellation.
Attachment #690460 - Attachment is obsolete: true
Attachment #690460 - Flags: feedback?(honzab.moz)
Attachment #691318 - Flags: review?(honzab.moz)
Posted patch v2 (obsolete) — Splinter Review
Sorry for the spam, wrong patch :\
Attachment #691318 - Attachment is obsolete: true
Attachment #691318 - Flags: review?(honzab.moz)
Attachment #691319 - Flags: review?(honzab.moz)
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.
Posted patch v3Splinter Review
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 :).
Attachment #691319 - Attachment is obsolete: true
Attachment #691319 - Flags: review?(honzab.moz)
Attachment #692278 - Flags: review?(honzab.moz)
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.
Attachment #692278 - Flags: review?(honzab.moz) → review+
(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?
Flags: needinfo?(ferjmoreno)
Posted patch TestsSplinter Review
Attachment #693148 - Flags: review?(honzab.moz)
Flags: needinfo?(ferjmoreno)
(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 :)

:)
Target Milestone: --- → B2G C3 (12dec-1jan)
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.
Attachment #693148 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/921863b9cd28
https://hg.mozilla.org/mozilla-central/rev/a0d0ac42bacd
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.