1170837 - Provide way to update packaged apps

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Valentin Gosu [:valentin] (he/him)]

Provide a way to check for ETag/Size/LastModified and update the app/cache entries if anything has changed.

Comment 1

[Honza Bambas (:mayhemer)]

Can you be more specific with the intention here?  The cache and HTTP channel already work well with validation headers.  What exactly it is you need to add?

Comment 2

[Valentin Gosu [:valentin] (he/him)]

The HTTP channel delegates most of the work to the packaged app service for URLs containing !//
At the moment, the packaged app service does not check the headers of the package to decide if the cache entries need updating. The only workaround at the moment is trying to load a resource that does not exist in the package, and the cache miss will force the package to be downloaded and resources inserted in the cache.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

I have a few options for the architecture of this:

1. Use HEAD requests to check that the package has not changed
* Lookup the cache entry
* In OnCacheEntryAvailable open a channel, set the method to be HEAD, and set INHIBIT_CACHING so we can download it later.
* In OnStartRequest check if the response is from cache (meaning the package hasn't changed), if so, deliver OnCacheEntryAvailable to the listener. If the response isn't from the cache, we call OpenNewPackageInternal and download the package.

2. Use GET to check that the package has not changed, and cancel the channel if it hasn't
* We don't look up the entry in the cache, but we open a channel for the package right away
* if it's from the cache, that means it hasn't changed. We lookup the cache entry and serve it to the listener.
* if it has changed, we just download it.
* also, if there was an error in getting the package, I also think we should just serve it from the cache.

--

I have a patch for the first option, although I think the second is better.
Any thoughts?

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Attachment: Make nsMultiMixedConv not return an error when served only a package's metadata from the cache

Comment 5

[Honza Bambas (:mayhemer)]

Resolved on IRC

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8632753 [details] [diff] [review]
Make nsMultiMixedConv not return an error when served only a package's metadata from the cache

Dealt on IRC this needs an update

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: Make nsMultiMixedConv not return an error when served only a package's metadata from the cache

Make the stream converter aware of the "application/package" mimetype.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Attachment: Add static GetPackageURI method to PackagedAppService

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Attachment: Provide way to update packaged apps

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8635639 [details] [diff] [review]
Make nsMultiMixedConv not return an error when served only a package's metadata from the cache

Review of attachment 8635639 [details] [diff] [review]:
-----------------------------------------------------------------

Closing.  Overall good.

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +473,5 @@
>      //  in the raw stream.
>      mFinalListener = aListener;
>  
> +    nsAutoCString packagedAppMime(APPLICATION_PACKAGE);
> +    if (packagedAppMime == aFromType) {

NS_LITERAL_CSTRING(APPLICATION_PACKAGE).Equals(aFromType)

@@ +814,5 @@
>      nsresult rv = NS_OK;
>  
>      // We should definitely have found a token at this point. Not having one
>      // is clearly an error, so we need to pass it to the listener.
> +    if (mToken.IsEmpty() && !(mPackagedApp && mIsFromCache && mFirstOnData)) {

update the comment why exactly you are extending the condition, what the part you are adding exactly means.  honestly I don't follow the condition at all.

@@ +845,5 @@
>          //(void) mFinalListener->OnStartRequest(request, ctxt);
>          
>          (void) mFinalListener->OnStopRequest(request, ctxt, aStatus);
> +    } else if (mIsFromCache && mFirstOnData) {
> +        // If the cache entry only holds metadata, then no we would not call

"then no we would"

few words why mFirstOnData is true here

let you know there are also resources w/o content (not just because you are storing only metadata in the cache)

::: netwerk/streamconv/converters/nsMultiMixedConv.h
@@ +183,5 @@
>      // as it can be ascertained from the package file.
>      bool                mPackagedApp;
> +    nsAutoPtr<mozilla::net::nsHttpResponseHead> mResponseHead;
> +    // It is necessary to know if the content is coming from the cache
> +    // in the case of packaged apps

it would be nice to also say why

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8635640 [details] [diff] [review]
Add static GetPackageURI method to PackagedAppService

Review of attachment 8635640 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +520,5 @@
>  
> +/* static */ nsresult
> +PackagedAppService::GetPackageURI(nsIURI *aURI, nsIURI **aPackageURI)
> +{
> +  nsCOMPtr<nsIURL> url = do_QueryInterface(aURI);

any particular reason why not to pass nsIURL already as an arg?  maybe this helps keep the code cleaner, just asking.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8635641 [details] [diff] [review]
Provide way to update packaged apps

Review of attachment 8635641 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +398,5 @@
>    }
>  
>    // If this is the last part of the package, it means the requested resources
>    // have not been found in the package so we return an appropriate error.
> +  if (NS_SUCCEEDED(aStatusCode) && lastPart && !mIsFromCache) {

why has been && !mIsFromCache added to the cache?  update the comment!

@@ +442,5 @@
>    nsCOMArray<nsICacheEntryOpenCallback>* array = mCallbacks.Get(spec);
>    if (array) {
> +    if (array->Length() == 0) {
> +      // This resource has already been downloaded. It's safe to just serve it
> +      // from the cache.

"This resource download has already been done earlier, hence we don't need to wait for its population to the cache and serve it right now, directly.  See also blabla method bellow"


and a good catch btw!

::: netwerk/protocol/http/PackagedAppService.h
@@ +150,4 @@
>    };
>  
> +  // Intercepts OnStartRequest, OnDataAvailable*, OnStopRequest method calls
> +  // and forwards them to the listener.

what is the underlying channel?  who is the target?  what does this class do?  what is the lifetime?

::: netwerk/test/unit/test_packaged_app_service.js
@@ +195,2 @@
>  function test_request_number() {
> +  equal(packagedAppRequestsMade, 2, "2 requests are expected. First with content, second is a 304 not modified.");

hmm... how is that now it's 2?  I don't see any related test changes.

Comment 13

[Valentin Gosu [:valentin] (he/him)]

> > +/* static */ nsresult
> > +PackagedAppService::GetPackageURI(nsIURI *aURI, nsIURI **aPackageURI)
> > +{
> > +  nsCOMPtr<nsIURL> url = do_QueryInterface(aURI);
> 
> any particular reason why not to pass nsIURL already as an arg?  maybe this
> helps keep the code cleaner, just asking.

It seems cleaner to QI inside the method, instead of having the callers QI before calling it.
Other than that there's no reason.

> ::: netwerk/test/unit/test_packaged_app_service.js
> @@ +195,2 @@
> >  function test_request_number() {
> > +  equal(packagedAppRequestsMade, 2, "2 requests are expected. First with content, second is a 304 not modified.");
> 
> hmm... how is that now it's 2?  I don't see any related test changes.

The test hasn't changed, but the code has. Every time a resource is requested, it opens a channel for the package. That channel may be satisfied from the cache, if its entry is still valid. But as we don't provide any Expires or cache-control headers for the package, a second request will be made, to which we respond with a 304 code.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Attachment: Make nsMultiMixedConv not return an error when served only a package's metadata from the cache

Make the stream converter aware of the "application/package" mimetype.

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Attachment: Make nsMultiMixedConv not return an error when served only a package's metadata from the cache

Oops, forgot to qrefresh.

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8636897 [details] [diff] [review]
Make nsMultiMixedConv not return an error when served only a package's metadata from the cache

Review of attachment 8636897 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +776,5 @@
>      // http://www.w3.org/TR/web-packaging/#streamable-package-format
>      // Although it is compatible with multipart/* this format does not require
>      // the boundary to be included in the header, as it can be ascertained from
>      // the content of the file.
> +    if (delimiter.Find(APPLICATION_PACKAGE) != kNotFound) {

would NS_LITERAL_CSTRING(APP...AGE) work here?  I think there is an overload of Find() for nsACString/Substring too..

@@ +874,5 @@
> +        // `mFirstOnData` is true if the package's cache entry only holds
> +        // metadata and no calls to OnDataAvailable are made.
> +        // In this case we would not call OnStopRequest for any of the parts,
> +        // so we need to call it here.
> +        (void) mFinalListener->OnStopRequest(request, ctxt, aStatus);

one question: how should we behave (fix this omission) when a response from the network doesn't have any content body?

::: netwerk/streamconv/converters/nsMultiMixedConv.h
@@ +184,5 @@
>      bool                mPackagedApp;
> +    nsAutoPtr<mozilla::net::nsHttpResponseHead> mResponseHead;
> +    // It is necessary to know if the content is coming from the cache
> +    // for packaged apps, in the case that only metadata is saved in the cache
> +    // entry, and OnDataAvailable never gets called.

nit: no "," before and

Comment 17

[Valentin Gosu [:valentin] (he/him)]

> > +        // so we need to call it here.
> > +        (void) mFinalListener->OnStopRequest(request, ctxt, aStatus);
> 
> one question: how should we behave (fix this omission) when a response from
> the network doesn't have any content body?

I think not. The streamable-package-format, as well as the multipart/* format explicitly require a close-delimiter ( "--" boundary "--" ), so missing content is only valid when the response is coming from the cache.

Of course, we still have the problem of hanging if we get no content. I have filed bug 1186737 to deal with the problem.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86b2cd28a91b
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e4460b0194
https://hg.mozilla.org/integration/mozilla-inbound/rev/396674528152

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12042024&repo=mozilla-inbound

Comment 20

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04336a8432db
https://hg.mozilla.org/integration/mozilla-inbound/rev/1439e7ebd397
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d92866d1ce2

Comment 21

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e21284371250

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03fb7b080f7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ab9033e675
https://hg.mozilla.org/integration/mozilla-inbound/rev/84d88b744dee

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03fb7b080f7f
https://hg.mozilla.org/mozilla-central/rev/e7ab9033e675
https://hg.mozilla.org/mozilla-central/rev/84d88b744dee