Closed Bug 1170837 Opened 5 years ago Closed 5 years ago

Provide way to update packaged apps

Categories

(Core :: Networking, defect)

41 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Provide a way to check for ETag/Size/LastModified and update the app/cache entries if anything has changed.
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?
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.
Depends on: 1182487
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?
Flags: needinfo?(honzab.moz)
Resolved on IRC
Flags: needinfo?(honzab.moz)
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
Attachment #8632753 - Flags: review?(honzab.moz)
Make the stream converter aware of the "application/package" mimetype.
Attachment #8635639 - Flags: review?(honzab.moz)
Attachment #8632753 - Attachment is obsolete: true
Attachment #8635641 - Flags: review?(honzab.moz)
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
Attachment #8635639 - Flags: review?(honzab.moz) → feedback+
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.
Attachment #8635640 - Flags: review?(honzab.moz) → review+
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.
Attachment #8635641 - Flags: review?(honzab.moz) → review+
> > +/* 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.
Make the stream converter aware of the "application/package" mimetype.
Attachment #8636894 - Flags: review?(honzab.moz)
Attachment #8635639 - Attachment is obsolete: true
Attachment #8636894 - Attachment is obsolete: true
Attachment #8636894 - Flags: review?(honzab.moz)
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
Attachment #8636897 - Flags: review?(honzab.moz) → review+
Blocks: 1186737
> > +        // 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.
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12042024&repo=mozilla-inbound
Flags: needinfo?(valentin.gosu)
https://hg.mozilla.org/mozilla-central/rev/03fb7b080f7f
https://hg.mozilla.org/mozilla-central/rev/e7ab9033e675
https://hg.mozilla.org/mozilla-central/rev/84d88b744dee
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.