Expose nsOfflineCacheUpdate::Cancel() via nsIOfflineCacheUpdate.idl

RESOLVED FIXED in Firefox 19

Status

()

Core
Networking: Cache
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

Trunk
B2G C3 (12dec-1jan)
Points:
---

Firefox Tracking Flags

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

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Blocks: 819974
blocking-basecamp: --- → ?
(Assignee)

Updated

6 years ago
Blocks: 818848
(Assignee)

Updated

6 years ago
No longer blocks: 818848
(Assignee)

Updated

6 years ago
Assignee: nobody → ferjmoreno
blocking-basecamp: ? → +
(Assignee)

Comment 1

6 years ago
Created attachment 690460 [details] [diff] [review]
v1
Attachment #690460 - Flags: review?(honzab.moz)
(Assignee)

Comment 2

6 years ago
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"
(Assignee)

Updated

6 years ago
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()
(Assignee)

Comment 4

6 years ago
Created attachment 691318 [details] [diff] [review]
v2

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)
(Assignee)

Comment 5

6 years ago
Created attachment 691319 [details] [diff] [review]
v2

Sorry for the spam, wrong patch :\
Attachment #691318 - Attachment is obsolete: true
Attachment #691318 - Flags: review?(honzab.moz)
Attachment #691319 - Flags: review?(honzab.moz)
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 8

6 years ago
Created attachment 692278 [details] [diff] [review]
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 :).
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.
(Assignee)

Comment 13

6 years ago
(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)
(Assignee)

Comment 15

6 years ago
Created attachment 693148 [details] [diff] [review]
Tests
Attachment #693148 - Flags: review?(honzab.moz)
Flags: needinfo?(ferjmoreno)
(Assignee)

Comment 16

6 years ago
(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: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.