[PackagedAppService] Make sure a failed request on the network will fallback on the cache

RESOLVED INVALID

Status

()

Core
Networking
RESOLVED INVALID
3 years ago
a year ago

People

(Reporter: valentin, Assigned: valentin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-would-take])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
We need to make sure that requests to the packaged app service don't fail when:
- there is no network
- the package has been removed from the server
- the server returns an error code
- the server connection times out
- other?

Some of these cases may already work, but we need unit tests for all of them.
From what I know, for normal web pages, if the 'offline mode' is OFF and 
the request is failed due to network errors, we are still not able to access 
the content even if it's in the cache. If I am right, why should we need
to treat the packaged web app differently?

p.s. 
On Firefox OS, if we detect the network gets disconnected, we explicitly 
set nsIIOService.offline so the cache will be used instead and the user 
will not beware of the network error.

[1] http://hg.mozilla.org/mozilla-central/file/cb8bdb8ffaef/dom/system/gonk/NetworkManager.js#l824
For pinned package, it makes sense not to let it fail because we expect it to work offline.
But For unpinned package, I think we can endure certain failures?
Seems like you are mixing two things: pinning and offline accessibility (smells like Offline Application Cache (=appcache) to me!)

Pinning means to not evict the pinned cached entries automatically when the space on disk/device goes down.

But pinning says nothing about accessing the resources despite some network error - which is a very wide term.


Anyway, when you fail to re-validate/re-download a package (by means of network error or an error response from the server), you may leave the already cached resources intact and just ignore any errors (indeed except partial content, which also breaks atomicity ; probably not a concern for this bug?)

We already set LOAD_FROM_CACHE (I think, Valentin?) on channel loading the end resources (the !// URLs), so you are safe.  LOAD_FROM_CACHE prevents re-validation.


How is the "pinned"/"not pinned" package info propagated?
(Assignee)

Comment 4

3 years ago
Created attachment 8653626 [details] [diff] [review]
[PackagedAppService] Make sure a failed request on the network will fallback on the cache
(Assignee)

Updated

3 years ago
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 years ago
Jonas, do we want to allow any redirects of the package?
Such as, if I go to http://example.org/package.pak!//index.html, if instead of the package I get a redirect to another package, do we allow that? I'm thinking we don't want that. At the moment that wouldn't work, and the attached patch makes sure redirects give an error.
Also, I'm working on a patch using the AppCache as a backend, to make sure that package updates are atomic.
Flags: needinfo?(jonas)
Ugh, I feel like we are mixing too many things here.

The "!//" URL feature has nothing to do with "apps", with "offline" or with "pinning".

"!//" URLs should behave no different from normal URLs. I.e. if the user is offline and we don't have a URL cached, it should fail to load. If a cached "!//" resource is expired and the server returns a 404, we should return a 404. If a cached "!//" resource is expired and we fail to connect to the server due to network issues or timeouts, then we should fail the request.

In short, "!//" should behave like normal URLs. So please don't think of them as apps or as anything that requires special offline behavior.

Similarly, if we load a URL like "http://example.org/package.pak!//index.html", and attempting to load "http://example.org/package.pak" from the server results in a redirect to "http://otherwebsite.org/otherPackage.pak", then we should follow that redirect like normal. We should also fire a AsyncOnChannelRedirect callback where the new channel has a URI of "http://otherwebsite.org/otherPackage.pak!//index.html". I.e. we should largely do the same thing that we do for normal loads, except that we keep the part after the "!//" on the client side, and don't expose it to the network.


Orthogonally, we are in a number of bugs discussing the pinning feature. When a resource is pinned we indeed want to guarantee that it can always be loaded. But this is being discussed in a number of other bugs, so I hope that we don't need to cover it here, do we?
Flags: needinfo?(jonas)
(Assignee)

Comment 8

3 years ago
(In reply to Jonas Sicking (:sicking) from comment #7)
Thanks for that Jonas, that's much clearer now.
I'll make the fallback-on-the-cache behaviour conditional on if the the package is pinned.
(In reply to Jonas Sicking (:sicking) from comment #7)
> Similarly, if we load a URL like
> "http://example.org/package.pak!//index.html", and attempting to load
> "http://example.org/package.pak" from the server results in a redirect to
> "http://otherwebsite.org/otherPackage.pak", then we should follow that
> redirect like normal. We should also fire a AsyncOnChannelRedirect callback
> where the new channel has a URI of
> "http://otherwebsite.org/otherPackage.pak!//index.html". I.e. we should
> largely do the same thing that we do for normal loads, except that we keep
> the part after the "!//" on the client side, and don't expose it to the
> network.

Interesting point.  First, I was never thinking about how we should handle package redirects (my fault!).  For what you suggest we need to redirect the resource loading channel to the new !// URL.  It's doable, tho a bit complicated.

There should be a new bug filed on that.

> 
> 
> Orthogonally, we are in a number of bugs discussing the pinning feature.
> When a resource is pinned we indeed want to guarantee that it can always be
> loaded. But this is being discussed in a number of other bugs, so I hope
> that we don't need to cover it here, do we?

It's already implemented in the pinning bug 1032254.

(In reply to Valentin Gosu [:valentin] from comment #8)
> (In reply to Jonas Sicking (:sicking) from comment #7)
> Thanks for that Jonas, that's much clearer now.
> I'll make the fallback-on-the-cache behaviour conditional on if the the
> package is pinned.

You don't need to do anything.


See https://bugzilla.mozilla.org/attachment.cgi?id=8650055&action=diff#a/netwerk/cache2/CacheEntry.cpp_sec5
Whiteboard: [necko-would-take]
(Assignee)

Comment 10

a year ago
Web packaged apps were removed.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.