Closed Bug 1036275 Opened 10 years ago Closed 9 years ago

Support http-served packaged HTML pages/apps

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: fabrice, Assigned: valentin)

References

Details

Attachments

(3 files, 18 obsolete files)

Add Packaged App Service
47.62 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
Change nsHttpChannel to call into PackagedAppService for urls containing !//
15.76 KB, patch
Details | Diff | Splinter Review
package.pak
490 bytes, application/x-pak
Details 
We need that to install packaged apps from real web urls as:
https://my-domain.com/foo/package.zip!//images/logo.png
Attached patch wip (obsolete) — — Splinter Review 
Still need to add tests. I'm also not sure if the change in the jar channel to flag these loads as safe is good enough, or if we should explicitly set a flag on the jar channel when we Init() it.
Assignee: nobody → fabrice
Attachment #8452891 - Flags: feedback?(jduell.mcbugs)
Just as a pointer, the security implications of this were discussed in bug 369814.
TLDR: We should expect an explicit non-standard mime-type as opt-in. Otherwise, XSS entails.
Comment on attachment 8452891 [details] [diff] [review]
wip

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

Ran out of time to get to this before I go on PTO for a week... Passing to Honza.
Attachment #8452891 - Flags: feedback?(jduell.mcbugs) → feedback?(honzab.moz)
(In reply to Fabrice Desré [:fabrice] from comment #0)
> We need that to install packaged apps from real web urls as:
> https://my-domain.com/foo/package.zip!//images/logo.png

Can you describe more in detail what you are trying to achieve?  Where will this URL be present?  Just in web content?  Why doesn't it have jar: schema directly?  Why it cannot be relative?  Is this '!/' URL parametrization specified somewhere?

The example URL is not a packaged app, it's a resource from the app.

Honestly, looking at the patch -- it's most crazy stuff I've seen in many months :)  Mostly I'm concerned about exploitability and false-positives on the '!//' found somewhere in the URL.  When this would behave bad, we could well end up with a bad security bug.  Also, replacing a channel this way doesn't simply sound right to me, but...  I understand the purpose.

Is there a specific mime type the zip file is served with?  I assume it's just x-app/zip or so?  If it were something specific, we could filter the response with a content decoder or something, but that is opt-in for the servers (.htaccess not available on e.g. free/shared hosting..)

I assume the server response to unpatched request example is 404?

This way I assume we have to download (and hopefully cache?) the zip and then use to server the single resource content, right?  Good we have cache entry concurrent sharing now...

I think the URL form here is the problem.  I assume resource loads will only come from content that is being loaded in a context.  Hence I more tend to have some better support for relative URLs here then absolute URLs with path in a zip file.

Does anyone else (chrome,ie,whoever) do this?  By means of supporting such '!inner/path' URLs.

The '!' modifier should actually be treated a bit like '#'.  But I'm worried about web compat.
See comment 4
Flags: needinfo?(fabrice)
Comment on attachment 8452891 [details] [diff] [review]
wip

Dropping f? until the questions get answered.
Attachment #8452891 - Flags: feedback?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #4)
> (In reply to Fabrice Desré [:fabrice] from comment #0)
> > We need that to install packaged apps from real web urls as:
> > https://my-domain.com/foo/package.zip!//images/logo.png
> 
> Can you describe more in detail what you are trying to achieve?  Where will
> this URL be present?  Just in web content?  Why doesn't it have jar: schema
> directly?  Why it cannot be relative?  Is this '!/' URL parametrization
> specified somewhere?
> 
> The example URL is not a packaged app, it's a resource from the app.

Currently packaged apps live in their own silo, with app:// based origins that are assigned by the platform when you install the app (you can ask for a specific origin only if you are a privileged app). That means that it's impossible to link to a resource inside an app://, first because you can't guess the url, and secondly because app:// rely on the app being installed.

We want packaged apps to be more webby, and providing real linkability through stable urls is a key point to achieve that. So what we try to do is let people host packages on the web, and build http(s):// uri to access resources in them. We also need uri resolution to "just work", so that if https://my-domain.com/foo/package.zip!//index.html has a <script src="main.js"> tag this resolves to https://my-domain.com/foo/package.zip!//main.js

> Honestly, looking at the patch -- it's most crazy stuff I've seen in many
> months :)  Mostly I'm concerned about exploitability and false-positives on
> the '!//' found somewhere in the URL.  When this would behave bad, we could
> well end up with a bad security bug.  Also, replacing a channel this way
> doesn't simply sound right to me, but...  I understand the purpose.

Yes, I'm also not super happy with the fact that we actually return a JARChannel and not the HTTP channel itself. If you know how to do that better, please tell!

> Is there a specific mime type the zip file is served with?  I assume it's
> just x-app/zip or so?  If it were something specific, we could filter the
> response with a content decoder or something, but that is opt-in for the
> servers (.htaccess not available on e.g. free/shared hosting..)

We will enforce a mime type, as Frederik noted in comment 2.

> I assume the server response to unpatched request example is 404?

That's what we want, yes.

> This way I assume we have to download (and hopefully cache?) the zip and
> then use to server the single resource content, right?  Good we have cache
> entry concurrent sharing now...

Yep. I also filed bug 1032254 about cache behavior.

> I think the URL form here is the problem.  I assume resource loads will only
> come from content that is being loaded in a context.  Hence I more tend to
> have some better support for relative URLs here then absolute URLs with path
> in a zip file.

But how do you bootstrap your context? How do you load the first resource form the zip?

> Does anyone else (chrome,ie,whoever) do this?  By means of supporting such
> '!inner/path' URLs.

Not that I know, but I don't think this is a big issue.
Flags: needinfo?(fabrice)
Flags: needinfo?(honzab.moz)
Jason, transfering the ni? on you since Honza is not available.
Flags: needinfo?(honzab.moz) → needinfo?(jduell.mcbugs)
Comment on attachment 8452891 [details] [diff] [review]
wip

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

So for a gross hack the implementation here is actually fairly simple.  We just return a JAR file on the child (instead of an HTTP file) with a "jar:http" URI, and then everything mostly just works.

Some questions about the "mostly":
1) will clients be OK with getting a nsIJARChannel back instead of an nsIHttpChannel?
2) there's currently no way AFAICT to get HTTP headers, etc back from a jar:http channel. OK?

Fabrice is going to look into those questions.  If we need to return something that looks more like an HTTP channel, I propose the following:

- we could teach nsIJARChannel to provide access to the HttpChannel that does the load (on the child that would be an HttpChannelChild).  That could provide savvy clients with a way to get the content-length, etc. info

- if we need the client to get an actual HTTP channel back, we'll need to return something that implements nsIHttpChannel (and ideally, something that if you SetHeader() on it, etc, will affect how the actual HTTP request happens).  Not sure if we need that level of versimilitude, but if we do, I'd propose we return an HttpChannelChild that's been trained to create a JAR file that it AsyncOpen()s during its own AsyncOpen().  
  
- If we need the ability for the HttpChannel to have things like SetHeader affect the HTTP request, we could also add an API to nsJARChannel (doesn't need to be in an IDL) that allows the HTTP channel that we want to use for the download to be passed in.  I.e. instead of just calling NS_OpenURI() here we'd hand roll something that lets the channel for the load:

    http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.cpp#837

that channel could either be 1) the initial HttpChannelChild (we'd need to let it be AsynOpen'd twice, with the first creating/opening the JARChannel and the 2nd doing the actual download of the zip file), or 2) a new HttpChannelChild that we forward any SetHeader calls to.  Both are somewhat gross...

- The JarChannel could probably be trained to have a special mode where when it calls OnStart/Data/Stop it passed the underlying Httpchannel for the nsIRequest arg instead of itself.  That would be the last piece toward making it all seem like an HTTP load to the client.

As Honza mentioned this is all a little bit gross...  I'd still prefer an "app:http://" link, which would 1) eliminate the "I asked for an HTTP channel, but got a JAR channel instead" issue, and 2) skip the whole magic "look for '!//' in the URI" issue.   That does seem like it might have false-positive or security issues, though I haven't thought them through enough.  The implementation would also be simple.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1693,5 @@
>  
> +    // Check if we are loading a hosted package, and if so create the
> +    // appropriate jar channel.
> +    // We turn http://foo.com/path/app.zip!//js/file.js into:
> +    //         jar:http://foo.com/path/app.zip!/js/file.js

I assume the reason for using '!//' in the http link, but '!/' in the jar one is that you're trying to make false positives less likely in http uris?

@@ +1702,5 @@
> +        nsAutoCString jarSpec("jar:");
> +        jarSpec.Append(Substring(spec, 0, pos));
> +        jarSpec.Append("!");
> +        jarSpec.Append(Substring(spec, pos + 2));
> +        nsRefPtr<nsJARChannel> channel = new nsJARChannel();

I'd allocate channel via nsIIOService.newChannelFromURI().  Slightly less code and the kosher way.  Not a big deal.
(In reply to Fabrice Desré [:fabrice] from comment #0)
> We need that to install packaged apps from real web urls as:
> https://my-domain.com/foo/package.zip!//images/logo.png

if you connect to port 443, establish a ssl session, and say
GET /foo/package.zip!//images/logo.png HTTP/1
host: my-domain.com

what is the expected response? (i.e. is it the same as the jar code would yield - a valid png).
> GET /foo/package.zip!//images/logo.png HTTP/1

JAR channels never ask for that URI from a server--they only ask for the zip file, i.e.

  GET /foo/package.zip HTTP/1

The !// stuff gets shaved off and used to access the contents of the ZIP once it's downloaded.
Flags: needinfo?(jduell.mcbugs)
And to be clear--we'd wind up using a JAR channel here (though it might be hidden behind an HTTP channel so everything looks more normal), so we'll still always be asking the HTTP server for the zip file only.
(In reply to Jason Duell (:jduell) from comment #11)
> > GET /foo/package.zip!//images/logo.png HTTP/1
> 
> JAR channels never ask for that URI from a server--they only ask for the zip

that's not what I asked :)

if http://blah in firefox means a different resource than http://blah in another http compliant client then its going to need a pretty high bar to justify.
(In reply to Jason Duell (:jduell) from comment #9)
> Comment on attachment 8452891 [details] [diff] [review]
> wip
> 
> Review of attachment 8452891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So for a gross hack the implementation here is actually fairly simple.  We
> just return a JAR file on the child (instead of an HTTP file) with a
> "jar:http" URI, and then everything mostly just works.
> 
> Some questions about the "mostly":
> 1) will clients be OK with getting a nsIJARChannel back instead of an
> nsIHttpChannel?

It's not ideal because we set loadFlags and notificationCallbacks on the channel (eg to switch to the right cookie jar).

> 2) there's currently no way AFAICT to get HTTP headers, etc back from a
> jar:http channel. OK?

Not great either. We need at least content type, size and etag from the resource we actually load.
 
> Fabrice is going to look into those questions.  If we need to return
> something that looks more like an HTTP channel, I propose the following:
> 
> - we could teach nsIJARChannel to provide access to the HttpChannel that
> does the load (on the child that would be an HttpChannelChild).  That could
> provide savvy clients with a way to get the content-length, etc. info

That would work for the apps code that I checked for the previous use cases.

> 
> As Honza mentioned this is all a little bit gross...  I'd still prefer an
> "app:http://" link, which would 1) eliminate the "I asked for an HTTP
> channel, but got a JAR channel instead" issue, and 2) skip the whole magic
> "look for '!//' in the URI" issue.   That does seem like it might have
> false-positive or security issues, though I haven't thought them through
> enough.  The implementation would also be simple.

The reason for not using app:http:// is mostly to make things really be webby, including origin checks, csp, etc.

> ::: netwerk/protocol/http/nsHttpHandler.cpp
> @@ +1693,5 @@
> >  
> > +    // Check if we are loading a hosted package, and if so create the
> > +    // appropriate jar channel.
> > +    // We turn http://foo.com/path/app.zip!//js/file.js into:
> > +    //         jar:http://foo.com/path/app.zip!/js/file.js
> 
> I assume the reason for using '!//' in the http link, but '!/' in the jar
> one is that you're trying to make false positives less likely in http uris?

Yes. Jonas wants to check web compatibility there also.

> @@ +1702,5 @@
> > +        nsAutoCString jarSpec("jar:");
> > +        jarSpec.Append(Substring(spec, 0, pos));
> > +        jarSpec.Append("!");
> > +        jarSpec.Append(Substring(spec, pos + 2));
> > +        nsRefPtr<nsJARChannel> channel = new nsJARChannel();
> 
> I'd allocate channel via nsIIOService.newChannelFromURI().  Slightly less
> code and the kosher way.  Not a big deal.

Ha yes thanks, I forgot about this one!
(In reply to Patrick McManus [:mcmanus] from comment #13)
 
> if http://blah in firefox means a different resource than http://blah in
> another http compliant client then its going to need a pretty high bar to
> justify.

http://blah.net/a.zip!//foo would get you a different resource, yes. How big a web compat issue this is, I'm not sure. Jonas?
Flags: needinfo?(jonas)
(In reply to Fabrice Desré [:fabrice] from comment #15)
> (In reply to Patrick McManus [:mcmanus] from comment #13)
>  
> > if http://blah in firefox means a different resource than http://blah in
> > another http compliant client then its going to need a pretty high bar to
> > justify.
> 
> http://blah.net/a.zip!//foo would get you a different resource, yes.

(In reply to Fabrice Desré [:fabrice] from comment #7)
> 
> We want packaged apps to be more webby, and providing real linkability
> through stable urls is a key point to achieve that. So what we try to do is
> let people host packages on the web, and build http(s):// uri to access
> resources in them. We also need uri resolution to "just work", so that if
> https://my-domain.com/foo/package.zip!//index.html has a <script
> src="main.js"> tag this resolves to
> https://my-domain.com/foo/package.zip!//main.js
> 

Would a new scheme work? I don't mean jar or app.. I mean a new one defined with your semantics above - which involve a superset of https and some new uri rules like you're describing in the patch. Websockets is sort of a kindred spirit - wss://example.com is actually defined to mean a particular https:// bootstrapping transaction followed by a formatted message exchange. (OK, its not an exact match :))

You don't have to worry about backward compatibility - anybody that can't interepret newscheme:// wasn't going to interpret the !// syntax the way you wanted either. And infrastructure and servers can still run vanilla https (please don't allow plaintext) servers.

I don't think you've really made it more webby by making the http:// scheme inconsistent.
Fair enough, I don't have a super strong opinion but a new scheme looks better to me than app:http://...
We definitely want to use "real" http URLs here. I.e. neither a new scheme or a nested schema.

The intent here is to experiment with how well a "!//" separator solution works. The ultimate goal is for this to become an agreed upon web standard. There's discussions within the W3C TAG about how to do packaging on the web. My impression is that something like a "!//" separator is the strongest contender as a solution. Possibly in combination with other features which make fallback on existing browsers easier.

But it's definitely a big undertaking to add something like this. This needs buyin from at least W3C and IETF. And possibly other orgs that I don't know about.

Until we have that the goal is not to ship this in release builds for Firefox desktop.

In FirefoxOS (and maybe in Fennec and WebRT, not sure) we already have a packaging solution which is based on app:// URLs which have all sorts of problems. So either way I think this is a step in the right direction for packaging on the web.

But I definitely don't think that we should enable this in Firefox desktop release builds. And probably also not in nightly/aurora builds for desktop either.

So we should make sure this feature is controlled by a pref and then only enable this pref in FirefoxOS for now.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #18)
> We definitely want to use "real" http URLs here. I.e. neither a new scheme
> or a nested schema.

The disadvantage to re-using https:// is an inconsistent web. What is the advantage?
The advantage is that we'll get experience with how well the "!//" solution works. The goal is that the final solution for packages will enable pages loaded from a package to still use OAuth which requires normal http(s) origins and that can make connections back to their home server using normal same-origin policies (i.e. without relying on CORS).

Whatever we do here fractures the web. Including if we do nothing. All the same content that we're hoping to migrate to using "!//" is currently using app://. I'm just as worried that they will come to depend on that. Likewise if we use app:http:// or somenewscheme: I'd be worried about content becoming reliant on that.

But again, I don't think we're going to fracture things very much since there's very little content being authored specifically for FirefoxOS, which is the only place where I'm proposing we turn this on for now.
Fabrice:  have you figured out if you need to get an nsIHttpChannel back, or if a JARChannel is fine here?
Flags: needinfo?(fabrice)
Fabrice tells me we do need nsIHttpChannel.
Flags: needinfo?(fabrice)
Fabrice,

I need to know where you want to store the downloaded application.zip file.  Will there be some external mechanism to delete them at some point?  By default jar:http://foo.com/bar.jar URIs result in bar.jar getting downloaded and then deleted from the filesystem by the channel's destructor (so one .zip load per URI!).  I'm guessing you want a longer lifespan :)   Will these apps be registered with Gonk as installed apps in some standard location, with deletion handled by some code that's not in the JarChannel?
Flags: needinfo?(fabrice)
My current patches are reusing most of the current packaged apps infrastructure, because we need to do the same signature checks etc. That means that we save the application.zip in /data/local/webapps/$APPORIGIN/application.zip. Deletion is done when you uninstall the app. It seems that for installed apps we could use this as a poor man cache, but for resources from uninstalled apps we need necko to cache the zip resource no (and pin it as we already discussed)?
Flags: needinfo?(fabrice)
High level architecture (from chat with Honza and Fabrice):

We will create a new HttpAppChannel class to handle this magical new http URI type:
  
  http://foo.com/app.zip!//subresource.jpg

Fabrice says it's important that these act exactly like HTTP channel responses (i.e. have HTTP status, cookies, response headers), even when they come from a cached app.zip file.  I.e. they need to work exactly as if they were hosted apps.  This means we need to store all the HTTP meta-data from the HTTP channel that initially loads the app.zip file and return it for each subrequest (with Content-Type and Content-Length adjusted for the subresource). We also want to maintain HTTP caching semantics (expiration, etags, etc) for the app.zip (so we re-fetch it if needed), with the important exception that Fabrice says we can assume that we can cache it for at least a day (this makes caching info on the child, which is important for performance, much easier).

However we can't just store the app.zip in the HTTP cache and load things normally.  HttpAppChannel needs to use JARChannel to extract the subresource from the app.zip.  JAR channels need a file descriptor (so they can mmap the app.zip file), but the cache and HttpChannel provide only the OnDataAvailable stream interface.  

We considered an architecture that changed the HTTP cache to return an fd instead of a stream, but it has several problems. 1) the fd would be to a cache entry, which contains both the app.zip and the cache meta-data (at the end), so we'd need to train the ZipReader code to handle these files; 2) the HTTP cache de-prioritizes writes (and prefers reads), so it doesn't provide a good guarantee of when the fd would actually be ready to read.  It seemed easier to instead use an nsIDownloader to store the app.zip as a separate file (much like nsJARChannel already does, but on the parent), and use the HTTP cache to store *only* the meta-data for the request, using a new nsICachingChannel.metadataOnly flag. (we will also read only meta-data from the cache, by specifying LOAD_ONLY_IF_MODIFIED, which returns NS_OK but doesn't call OnDataAvailable).

So, the first time a URI for a new app.zip is loaded, we will use IDPL to go to the parent, launch an nsIDownloader to get the file, and tell the HTTP cache to store only meta-data for the request.  We'll then add a new IPDL function that sends the fd for the zip and the usual HTTP info back to the child.  The HttpAppChannel will have a member mJarChannel that it will feed the fd to (this will require changes to the JAR code to let us specify an fd to read from), and then we can AsyncOpen it and have everything we need--HttpAppChannel looks like the HTTP channel that loaded the zip, and it forwards along the OnStart/OnData/OnStop calls from the JARChannel (possibly inserting an nsUnknownDecoder into the listener chain so we can return the right Content-Type?  Or maybe the code in nsJARChannel:GetContentType() is good enough--but it seems to use only file extensions).

The zip file will be stored in a special location by the nsIDownloader:

  /data/local/webapps/$APPORIGIN/application.zip

In addition, we want the nsIDownloader to be 'special' in that it will check for this location and use an existing zip file there if 1) the HTTP cache tells us (via LOAD_ONLY_IF_MODIFIED) that the file hasn't changed, or 2) we're offline.  In that case it will just open the file on disk and trigger OnDownloadComplete() as if it had downloaded the zip.

We also need to handle lifetime: we need cache pinning to guarantee that the app's cache entries aren't evicted (unless of course they have expired: maybe not even then, for offline support until we're online again?) until/unless the app is uninstalled.  Also we'll need to delete the app.zip file when an app is uninstalled (some code needs to monitor for nsIObserverService app uninstall events).

To make this code fast, we also want the common case to not need to do IPDL at all. Just like for the app:// protocol we want to just hit the JAR cache and be done. But since we need to also provide HTTP request/response info, we will need to store that in the JARCache somehow (probably by adding the ability to store opaque meta-data with a JAR cache entry) and provide a way for HttpAppChannel to access it so it can pretend to be the original HTTP request even for a JAR cache hit.  We'll also need to provide an expiration time for the JAR cache entry (the one-day minimum expiration mentioned above) so that we go to the parent and do regular HTTP validation and re-fetching as needed after that time passes.

Honza has a pretty good idea of the cache changes needed, and I don't think the HttpAppChannel/nsIDownloader stuff will be too hard.

The least understood parts of this bug right now are 1) how hard it will be to change the JAR code, and 2) whether we can feasibly re-use the existing IPDL ParamTraits code to serialize the HTTP {responseHead, requestHeaders, etc} data we need into a string that we store in the JAR cache as "metadata".  

Honza, does this all sound correct to you?
Flags: needinfo?(honzab.moz)
Depends on: 1064258
(In reply to Jason Duell (:jduell) from comment #25)
> High level architecture (from chat with Honza and Fabrice):
> 
> We will create a new HttpAppChannel class to handle this magical new http
> URI type:
>   
>   http://foo.com/app.zip!//subresource.jpg
> 
> Fabrice says it's important that these act exactly like HTTP channel
> responses (i.e. have HTTP status, cookies, response headers), even when they
> come from a cached app.zip file.  I.e. they need to work exactly as if they
> were hosted apps.  This means we need to store all the HTTP meta-data from
> the HTTP channel that initially loads the app.zip file and return it for
> each subrequest (with Content-Type and Content-Length adjusted for the
> subresource). We also want to maintain HTTP caching semantics (expiration,
> etags, etc) for the app.zip (so we re-fetch it if needed), with the
> important exception that Fabrice says we can assume that we can cache it for
> at least a day (this makes caching info on the child, which is important for
> performance, much easier).
> 
> However we can't just store the app.zip in the HTTP cache and load things
> normally.  HttpAppChannel needs to use JARChannel to extract the subresource
> from the app.zip.  JAR channels need a file descriptor (so they can mmap the
> app.zip file), but the cache and HttpChannel provide only the
> OnDataAvailable stream interface.  
> 
> We considered an architecture that changed the HTTP cache to return an fd
> instead of a stream, but it has several problems. 1) the fd would be to a
> cache entry, which contains both the app.zip and the cache meta-data (at the
> end), so we'd need to train the ZipReader code to handle these files; 2) the
> HTTP cache de-prioritizes writes (and prefers reads), so it doesn't provide
> a good guarantee of when the fd would actually be ready to read.  It seemed
> easier to instead use an nsIDownloader to store the app.zip as a separate
> file (much like nsJARChannel already does, but on the parent), and use the
> HTTP cache to store *only* the meta-data for the request, using a new
> nsICachingChannel.metadataOnly flag. (we will also read only meta-data from
> the cache, by specifying LOAD_ONLY_IF_MODIFIED, which returns NS_OK but
> doesn't call OnDataAvailable).

Nit: you will always just specify nsICachingChannel.metadataOnly flag, for both first fetch (a.k.a write) and follow on check/read channels.  LOAD_ONLY_IF_MODIFIED flag will be used only internally (transparently for you).

> 
> So, the first time a URI for a new app.zip is loaded, we will use IDPL to go
> to the parent, launch an nsIDownloader to get the file, and tell the HTTP
> cache to store only meta-data for the request.  We'll then add a new IPDL
> function that sends the fd for the zip and the usual HTTP info back to the
> child.  The HttpAppChannel will have a member mJarChannel that it will feed
> the fd to (this will require changes to the JAR code to let us specify an fd
> to read from), and then we can AsyncOpen it and have everything we
> need--HttpAppChannel looks like the HTTP channel that loaded the zip, and it
> forwards along the OnStart/OnData/OnStop calls from the JARChannel (possibly
> inserting an nsUnknownDecoder into the listener chain so we can return the
> right Content-Type?  Or maybe the code in nsJARChannel:GetContentType() is
> good enough--but it seems to use only file extensions).
> 
> The zip file will be stored in a special location by the nsIDownloader:
> 
>   /data/local/webapps/$APPORIGIN/application.zip

Not sure about the local (if that is similar to how our local and roaming part of the profile works) in the path.  I was told to store the pinned entries (when I was implementing the first attempt for support of !// URLs directly in the cache) in the roaming part of the profile as something very persistent, like extensions.  Local part is volatile, it can go away at any time (at least, I understand it's definition that way).  Is that what we want?

> 
> In addition, we want the nsIDownloader to be 'special' in that it will check
> for this location and use an existing zip file there if 1) the HTTP cache
> tells us (via LOAD_ONLY_IF_MODIFIED) that the file hasn't changed, or 2)
> we're offline.  In that case it will just open the file on disk and trigger
> OnDownloadComplete() as if it had downloaded the zip.

Note, that when a content of an entry is "NOT MODIFIED" and the metadataOnly flag (thus also automatically the LOAD_ONLY_IF_MODIFIED flag) is set, you will only get OnStart/OnStop(NS_OK) notifications and no OnData.  It's up to you to check IsFromCache on the channel so that you know your previously stored zip file is still valid.

> 
> We also need to handle lifetime: we need cache pinning to guarantee that the
> app's cache entries aren't evicted (unless of course they have expired:
> maybe not even then, for offline support until we're online again?)
> until/unless the app is uninstalled.  Also we'll need to delete the app.zip
> file when an app is uninstalled (some code needs to monitor for
> nsIObserverService app uninstall events).
> 
> To make this code fast, we also want the common case to not need to do IPDL
> at all. Just like for the app:// protocol we want to just hit the JAR cache
> and be done. But since we need to also provide HTTP request/response info,
> we will need to store that in the JARCache somehow (probably by adding the
> ability to store opaque meta-data with a JAR cache entry) and provide a way
> for HttpAppChannel to access it so it can pretend to be the original HTTP
> request even for a JAR cache hit.  We'll also need to provide an expiration
> time for the JAR cache entry (the one-day minimum expiration mentioned
> above) so that we go to the parent and do regular HTTP validation and
> re-fetching as needed after that time passes.
> 
> Honza has a pretty good idea of the cache changes needed, and I don't think
> the HttpAppChannel/nsIDownloader stuff will be too hard.
> 
> The least understood parts of this bug right now are 1) how hard it will be
> to change the JAR code, and 2) whether we can feasibly re-use the existing
> IPDL ParamTraits code to serialize the HTTP {responseHead, requestHeaders,
> etc} data we need into a string that we store in the JAR cache as
> "metadata".  
> 
> Honza, does this all sound correct to you?

Makes sense!  Perfect outline, you didn't miss a bit :)
Flags: needinfo?(honzab.moz)
>> /data/local/webapps/$APPORIGIN/application.zip
>
> Not sure about the local 

I'm just basing this path on what Fabrice said in comment 24.
I will finalize the architecture here and then we'll see who's gonna code it.
Assignee: fabrice → honzab.moz
Fabrice--do we need to support redirects for http://....!// URIs?  It may make the architecture a fair amount more complicated, so if we can forbid it...
Flags: needinfo?(fabrice)
So, here's the architecture Honza and I discussed. It's along the same lines as comment 25.

nsHttpHandler decides based on the !// in the path that we need to create a HttpAppChannel(Child?).
The HttpAppChannelChild calls a method on the parent that uses nsDownloader to download the zip file.
We set the loadOnlyIfModified flag on the channel, so it doesn't redownload the file if it didn't change. We also need cacheOnlyMetadata, so we only save the metadata in the cache.
We send back to the child the filedesc for the file, and the metadata.
On the child, we use a new caching service, which caches the filedesc for the zip file, and the metadata.
We then open an nsJARChannel to the filedescriptor and serve content from that.
Next requests that deal with the same zip file, would go through the caching service on the child, and won't do any IPDL.

The zip location would be: <roaming profile>/<jar-cache>/<app-id>/<file-url-hash>
There's also going to be a service which listens to uninstall events, and deletes the zip files for that app.

Regarding the redirects, if we don't need to notify any listeners on the child, that wouldn't be too much of a burden, probably, as the downloader will just get the resource we need. Otherwise there might be some serious effort needed to support it, or we could have it as a followup feature.

Also, I don't know if we need to support inner zips. I think it may be supported by the JarChannel so we might get this feature for free :)

I initially intended to have the HttpAppChannel extend HttpChannelChild, but what I've done so far feels very hacky, so I'm probably going to create a new IPDL.

Honza and Jason, let me know if I've missed anything.
Assignee: honzab.moz → valentin.gosu
Valentin, your description seems not to be 100% accurate.  To make this a bit more clear, here is a purely multiprocess architecture as I understand we agreed on:

- nsHttpHandler decides what channel class to instantiate to handle the URI
- on '!//' URI (or different conditions) it instantiates a new class HttpAppChannel (name of this new class is tricky and should yet be discussed, also, it probably doesn't need to have "Child" in the name, there is no IPC in it!)
- HttpAppChannel asks a JARCacheChild *service* via an *asynchrnous* API for FD of the file and also the metadata (response headers etc.) of the network response for the zip file 
- JARCacheChild has a hashtable 
  - the table is keyed by a hash (SHA256) of the pre-exclamation-mark part of the ASCII URI spec of the original URI (redirects not concerned right now)
  - the values in the table are: FD + metadata + time stamp
  - the cache entries are kept only for a limited amount of time (12 - 24 hours), hence the time stamp
- when the JARCacheChild doesn't find the FD+metadata value under the hash already, it sends IPC to the parent, JARCacheParent
- JARCacheParent instantiates nsDownloader with the pre-exclamation-mark part of the URI as URI to download and file path according to comment 30 as the file to download ; we don't care whether the file is already there or not
- nsDownloader needs to be extended to optionally instruct the channel to set nsICachingChannel.cacheOnlyMetadata = true on the channel ; *nothing else needs to be done with the channel, the channel will then automatically behave as if the LOAD_ONLY_IF_MODIFIED flag has been set on it*
- nsDownloader must then handle the case when we go from the cache (only OnStart/OnStop is called with NS_OK) and just return with "OK, your file is there"
- (we may also want to remember the cached file is valid and assume so for say 12 or 24 ongoing hours to not bother with conditional requests to the server on and on with every request to the JARCache[P/Ch] ; this may be done in a followup and should first be discussed)
- after nsDownloader is done, JARCacheParent opens the downloaded (or just revalidated) file and hands the FD and metadata of the channel nsDownloader used for fetching to JARCacheChild via IPC ; nsDownloader may have to be modified to hand the metadata out
- JARCacheChild saves the FD+metadata+<now> to its hashtable and calls back to the HttpAppChannel with that FD and metadata 
- HttpAppChannel then gives the FD to JARChannel to extract the particular file from the zip
- HttpAppChannel then forwards OnData to the final consumer and exposes the given metadata on itself
(In reply to Jason Duell [:jduell] (needinfo? me for lower latency) from comment #29)
> Fabrice--do we need to support redirects for http://....!// URIs?  It may
> make the architecture a fair amount more complicated, so if we can forbid
> it...

Redirecting http://...app.zip!//foo.css to http://...app.zip!//bar.css doesn't need to be supported.
But can we support redirecting http://...app.zip!//foo.css to http://...app2.zip!//foo.css ?
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #32)
> (In reply to Jason Duell [:jduell] (needinfo? me for lower latency) from
> comment #29)
> > Fabrice--do we need to support redirects for http://....!// URIs?  It may
> > make the architecture a fair amount more complicated, so if we can forbid
> > it...
> 
> Redirecting http://...app.zip!//foo.css to http://...app.zip!//bar.css
> doesn't need to be supported.
> But can we support redirecting http://...app.zip!//foo.css to
> http://...app2.zip!//foo.css ?

This will be supported automatically.  Only thing with the architecture from comment 31 (30) is that the content process will not be notified about the redirect - i.e. won't have a chance to veto as well as monitor any SSL state changes.  We could somehow enforce CSP or something on the parent for the downloading channel tho.  nsDownloader will be the most changed class here I think :)
The current TAG direction on packaging has changed.  Is http://w3ctag.github.io/packaging-on-the-web/ not cool for some reason?
So the architecture in comment 31 is nice and (relatively speaking) clean, but it doesn't do one thing:  if the channel client (docshell or whatever) on the child sets custom HTTP headers on the HttpAppChannel, those don't get sent in the actual network request that we use to fetch the app.zip (nor are they stored in the jar cache for future hits).

For instance, DocShell seems on some occasions to set an array of HTTP headers on its channels, which it's getting as a stream from a DocShellLoadInfo struct:

  http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10604 http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1377

Do we know what those headers are and how bad it would be to not be sending them?

If we need to support SetRequestHeader, I can think of two options:

1) We further fudge the JARChannelChild/Parent connection to also take a list of custom HTTP headers that need to get sent (HttpChannelChild intercepts SetRequestHeader and keeps a list of modified headers--mClientSetRequestHeaders.  We could have HttpAppChannel do the same) and then teach nsDownloader to accept custom headers (or a specified HTTP channel that we create and set the headers on).

2) My original idea for the architecture here was to have HttpAppChannel inherit from HttpChannelChild (so it would have a parent/child side), and on the parent, we'd teach nsDownloader to accept whatever channel we tell it to (which would be the nsHttpChannel that HttpChannelParent creates, so it would have the SetRequestHeader changes reflected there).  In this architecture most/all of the IPDL would get done by HttpAppChannel{Parent/Child|, i.e the nsDownloader would be on the parent, and HttpAppChannel would be responsible for moving the fd across IPDL and populating the jar cache on the child.  But it might make supporting redirects more complicated...
   
Honza, do you know what kinds of headers the docshell is setting here?  What do you think of option #2?
Flags: needinfo?(honzab.moz)
I think we can teach the JAR cache service (which also does IPDL in my architecture and may also easily notify about the redirects in some followup work) to obtain request headers that the HttpAppChannel has collected, and send them up.  I don't see a point in storing/caching these headers anywhere.  Only problem might be when change to these request headers would cause the resource (the zip) to change, but I don't expect it.

I'm no expert to docshell, bz says he isn't too, but he definitely knows more than me :)
Flags: needinfo?(honzab.moz)
My guess is we could probably set the headers on the channel used by the downloader quite easily (right now I don't set any headers), when we first get the resource.
If the docshell/whatever sets the same headers each time, there should not be a problem. If the headers change in a way that could make the zip change (as Honza said) we could later introduce some logic to update the zip whenever that happens.
This kinda works for this little unit test. I still have to implement a bunch of things, but things are looking good
Considering that it seems nsZipReaderCache::GetFd/nsJARChannel::LookupFile already have a caching mechanism for the FDs, I'd say building another one in JarCacheChild wouldn't be necessary anymore, right? So probably the name (JarCacheChild/Parent) isn't exactly appropriate, since we'd only be using it to cache the headers and timestamp.

I don't think this changes the architecture too much. Honza, is there anything you think we should change given this fact?
Flags: needinfo?(honzab.moz)
Hm...  nsJARChannel::LookupFile will do a remote open/cache lookup when you give it "remoteopenfile://" file URL.  Not in other cases.  But we could somehow use that to not duplicate the code, right.  Only thing is that you don't have control over the already implemented JAR cache, so any timestamps I was talking about earlier are moot now..

My architecture give us a good atomicity.  My JarCache IPC service you ask the FD for hides the file name from you and gives you an already open FD to the file we just verified with the server it's OK - the FD is open on the parent during a single event loop inside nsHttpChannel::OnStopRequset.  Hence, there is no window before someone else deletes/rewrites the file.

With reusing the cache already implemented we loose this atomicity since you have to ask the JarCache service (let's rename it to HttpJarService then) for the remote file URL to open, not the FD.  So before the ping-back via RemoteOpenFile the file is "unprotected".  Before HttpJarService gives the URL to you, it must download/reval the file via the procedure as I described in my architecture, so it remains unchanged.

I don't know if that atomicity is in anyway critical.  For me it's some kind of a sign of reliability, but depends, we should reuse existing code rather.  Still you need to keep the headers cached on the child, so there will be a new cache - two caches, yours and the existing one, you need to sync.  Up to you which way seems better to you.
Flags: needinfo?(honzab.moz)
Question: could 2 apps try to use a zip from the same URL? Because that would pose some issues:
For example, if app1 gets foo.com/app.zip!//a.txt, and app.zip gets downloaded to jarcache/[appID]/app.zip for example, then when app2 tries to get foo.com/app.zip!//a.txt the cache tells it that it has already been downloaded, but the zip isn't in the app's own folder.
Or does the cross-origin policy not allow two apps to get files from the same domain?
(In reply to Valentin Gosu [:valentin] from comment #41)
> Question: could 2 apps try to use a zip from the same URL? Because that
> would pose some issues:
> For example, if app1 gets foo.com/app.zip!//a.txt, and app.zip gets
> downloaded to jarcache/[appID]/app.zip for example, then when app2 tries to
> get foo.com/app.zip!//a.txt the cache tells it that it has already been
> downloaded, but the zip isn't in the app's own folder.
> Or does the cross-origin policy not allow two apps to get files from the
> same domain?

Te cache must isolate anything two distinct apps load.  Period.  

foo.com/app.zip loaded by app 1001 and 1002 will have a respective files and any cache entris (http cache already does it that way, when you correctly loadcontext the channel.)
(In reply to Honza Bambas (:mayhemer) from comment #42)
> 
> foo.com/app.zip loaded by app 1001 and 1002 will have a respective files and
> any cache entris (http cache already does it that way, when you correctly
> loadcontext the channel.)

That makes sense. Thanks for the clarification.
TODO:
* pass status to child in OnDownloadCompleted
* send appId and request headers to parent process
* create service to delete jars when app is uninstalled
* use actual _remote profile_
* fix possible leaks :)
Attachment #8524263 - Flags: feedback?(honzab.moz)
Attachment #8521682 - Attachment is obsolete: true
Comment on attachment 8524263 [details] [diff] [review]
Support loading inner resources from http served zips (WIP)

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

Doesn't look bad, but..

I'm playing with an idea to hack/reuse on jar:remoteopenfile to even download the file for you (not just open an already existing (?) file...  It would be easy to modify nsJARChannel to work with some new 'remote' schema (if needed - maybe "jar:removedownloadfile" ?) and the parent of remoteopenfile (or any derived class) would do the download job (mostly what you have already written).

What do you think?

::: netwerk/protocol/http/HttpAppChannel.h
@@ +9,5 @@
> +namespace mozilla {
> +namespace net {
> +
> +class HttpAppChannel
> +  : public HttpBaseChannel

not a bad idea

::: netwerk/protocol/http/HttpJarServiceChild.cpp
@@ +32,5 @@
> +
> +  nsTArray<HttpJarRequest>::index_type index = mRequests.IndexOf(spec);
> +  if (index != nsTArray<HttpJarRequest>::NoIndex) {
> +    // A request for this url is already in progress
> +    mRequests[index].mListeners.AppendElement(listener);

a hashtable please, nsClassHashtable or nsRefPtrHashtable probably.

assert thread when working with this array/hashtable

@@ +58,5 @@
> +  HttpJarCacheEntry *req = new HttpJarCacheEntry();
> +  req->mURI = uri;
> +  req->mFilepath = filepath;
> +  req->mHead = head;
> +  req->mTimestamp = time;

why is this going over IPC?  also, I've only seen 0 sent down.  the meaning is how long to keep the entry in our cache, so here you just use "TimeStamp::NowLoRes()"

@@ +65,5 @@
> +
> +  // We need to notify all channels that requested for this zip file
> +  nsTArray<HttpJarRequest>::index_type index = mRequests.IndexOf(uri);
> +  if (index != nsTArray<HttpJarRequest>::NoIndex) {
> +    for (uint32_t i = 0; i < mRequests[index].mListeners.Length(); ++i) {

good practice is to swap to a local array first

@@ +68,5 @@
> +  if (index != nsTArray<HttpJarRequest>::NoIndex) {
> +    for (uint32_t i = 0; i < mRequests[index].mListeners.Length(); ++i) {
> +      mRequests[index].mListeners[i]->FileAvailable(filepath, (nsHttpResponseHead *)&head);
> +    }
> +    mRequests[index].mListeners.Clear();

no need to do this

@@ +70,5 @@
> +      mRequests[index].mListeners[i]->FileAvailable(filepath, (nsHttpResponseHead *)&head);
> +    }
> +    mRequests[index].mListeners.Clear();
> +    mRequests.RemoveElementAt(index);
> +  }

else assert(false) ?

::: netwerk/protocol/http/HttpJarServiceParent.cpp
@@ +24,5 @@
> +
> +NS_IMPL_ISUPPORTS(JarRequestContext, nsISupports)
> +
> +static nsAutoCString
> +computeSHA256(nsCString input) {

... (...)
{
}

all over the file.

@@ +33,5 @@
> +  hasher->Finish(true /* ascii */, hash);
> +
> +  char* data = hash.BeginWriting();
> +  for (uint32_t i = 0; i < hash.Length(); ++i) {
> +    if (data[i] == '/') { // Filename must not contain slashes

Please make sure this is all you need to escape.

@@ +75,5 @@
> +  nsresult rv = NS_NewURI(getter_AddRefs(uri), spec);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv), "Bad URI");
> +
> +  nsCOMPtr<nsILoadInfo> loadInfo =
> +    new mozilla::LoadInfo(nsContentUtils::GetSystemPrincipal(),

this should be an app principal and later you should be able to get this info (the appid) from the channel back - to determine the file location.

@@ +90,5 @@
> +                                      nullptr, // callbacks
> +                                      nsIRequest::LOAD_NORMAL,
> +                                      nullptr); // ioservice
> +
> +  // Set the cacheOnlyMetadata flag on the CachingChannel

I think it's pretty obvious :)  rather say why you do it..

@@ +108,5 @@
> +NS_IMETHODIMP
> +HttpJarServiceParent::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
> +{
> +  JarRequestContext *context = static_cast<JarRequestContext*>(ctxt);
> +  nsHttpChannel *chan = static_cast<nsHttpChannel *>(request);

I don't like to see this..  could we rather expose on nsIHttpChannelInternal all that you need?

@@ +143,5 @@
> +  // Create a downloader with this as a DownloadListener
> +  // and the file as a target
> +  rv = NS_NewDownloader(getter_AddRefs(context->mDownloader), this, file);
> +
> +  context->mDownloader->OnStartRequest(request, ctxt);

interesting use... does this really work?  are you sure this use is not in any way fragile?

@@ +152,5 @@
> +HttpJarServiceParent::OnDataAvailable(nsIRequest *request, nsISupports *ctxt,
> +                              nsIInputStream *inStr,
> +                              uint64_t sourceOffset, uint32_t count)
> +{
> +  JarRequestContext *context = static_cast<JarRequestContext*>(ctxt);

unused

::: netwerk/protocol/http/PHttpJarService.ipdl
@@ +32,5 @@
> +  GetJar(nsCString uri);
> +  __delete__();
> +
> +child:
> +  Downloaded(nsCString uri, nsCString filepath, nsHttpResponseHead head, uint32_t time); // TODO: filedesc? and status

comments!

::: netwerk/protocol/http/moz.build
@@ +28,5 @@
>      'nsHttpResponseHead.h',
>  ]
>  
>  EXPORTS.mozilla.net += [
> +    'HttpAppChannel.h',

really needed to export (just confirming)?

::: netwerk/protocol/http/nsIHttpJarService.idl
@@ +20,5 @@
> +[scriptable, uuid(906bea9a-4ab8-4abc-a358-0cdb1f46e269)]
> +interface nsIHttpJarService : nsISupports
> +{
> +  void getJarFile(in nsIHttpJarServiceListener l, in nsIURI uri);
> +};

comments comments comments...
Attachment #8524263 - Flags: feedback?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #45)
> Comment on attachment 8524263 [details] [diff] [review]
> Support loading inner resources from http served zips (WIP)
> 
> Review of attachment 8524263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Doesn't look bad, but..
> 
> I'm playing with an idea to hack/reuse on jar:remoteopenfile to even
> download the file for you (not just open an already existing (?) file...  It
> would be easy to modify nsJARChannel to work with some new 'remote' schema
> (if needed - maybe "jar:removedownloadfile" ?) and the parent of
> remoteopenfile (or any derived class) would do the download job (mostly what
> you have already written).
> 
> What do you think?
> 

I think that's possible, but I don't see a big advantage in doing that.
There could be, but nsJARChannel already feels pretty complex.
For the moment I'm relying on remoteopenfile, but I'm contemplating passing the FD to the JarChannel in a followup bug.

> @@ +108,5 @@
> > +NS_IMETHODIMP
> > +HttpJarServiceParent::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
> > +{
> > +  JarRequestContext *context = static_cast<JarRequestContext*>(ctxt);
> > +  nsHttpChannel *chan = static_cast<nsHttpChannel *>(request);
> 
> I don't like to see this..  could we rather expose on nsIHttpChannelInternal
> all that you need?

I did the same thing as in HttpChannelParent.cpp:642 which also needs the responseHead.
We could probably add this to nsIHttpChannelInternal

> 
> @@ +143,5 @@
> > +  // Create a downloader with this as a DownloadListener
> > +  // and the file as a target
> > +  rv = NS_NewDownloader(getter_AddRefs(context->mDownloader), this, file);
> > +
> > +  context->mDownloader->OnStartRequest(request, ctxt);
> 
> interesting use... does this really work?  are you sure this use is not in
> any way fragile?
> 

Seems to work. I wrote it like this because while I could get the headers for the downloaded file in OnDownloadComplete, I also needed the headers from the cache, and it seemed cleaner to do it like this.
I don't think it would be fragile in any way, and the extra function call shouldn't be that much of a burden performance-wise.

> 
> ::: netwerk/protocol/http/moz.build
> @@ +28,5 @@
> >      'nsHttpResponseHead.h',
> >  ]
> >  
> >  EXPORTS.mozilla.net += [
> > +    'HttpAppChannel.h',
> 
> really needed to export (just confirming)?
> 

Right. No need.
I don't like this zip-file approach to apps. It forces you to download the entire zip if someone deep-links you to an image. It also means you re-download the entire zip if you want to update from a previous version, unless you're lucky and "transfer-encoding: rsync" works well with the zip file.

The use of zip files seems unnecessary. Pre-downloading is already supported through HTTP/2 push. Atomic updates could be supported by letting servers say "all your existing cache entries under https://app.example.com/app/ must be revalidated". The server and client could then negotiate updates to each file as appropriate, possibly using push and/or transfer-encoding.
(In reply to Valentin Gosu [:valentin] from comment #46)
> (In reply to Honza Bambas (:mayhemer) from comment #45)
> > Comment on attachment 8524263 [details] [diff] [review]
> > Support loading inner resources from http served zips (WIP)
> > 
> > Review of attachment 8524263 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Doesn't look bad, but..
> > 
> > I'm playing with an idea to hack/reuse on jar:remoteopenfile to even
> > download the file for you (not just open an already existing (?) file...  It
> > would be easy to modify nsJARChannel to work with some new 'remote' schema
> > (if needed - maybe "jar:removedownloadfile" ?) and the parent of
> > remoteopenfile (or any derived class) would do the download job (mostly what
> > you have already written).
> > 
> > What do you think?
> > 
> 
> I think that's possible, but I don't see a big advantage in doing that.
> There could be, but nsJARChannel already feels pretty complex.
> For the moment I'm relying on remoteopenfile, but I'm contemplating passing
> the FD to the JarChannel in a followup bug.

I think I'm OK with a followup on this.  The advantage (to make this reliable and "doubtless" code) is the atomicity factor.  You download the file and instantly open a handle to it that is actually left open until we evict from the child's cache.  This prevents manipulation with the file in the window between download and request for the FD via remoteopenfile.  But a priority followup is OK.

> 
> > @@ +108,5 @@
> > > +NS_IMETHODIMP
> > > +HttpJarServiceParent::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
> > > +{
> > > +  JarRequestContext *context = static_cast<JarRequestContext*>(ctxt);
> > > +  nsHttpChannel *chan = static_cast<nsHttpChannel *>(request);
> > 
> > I don't like to see this..  could we rather expose on nsIHttpChannelInternal
> > all that you need?
> 
> I did the same thing as in HttpChannelParent.cpp:642 which also needs the
> responseHead.
> We could probably add this to nsIHttpChannelInternal

Please do it in this bug.

> 
> > 
> > @@ +143,5 @@
> > > +  // Create a downloader with this as a DownloadListener
> > > +  // and the file as a target
> > > +  rv = NS_NewDownloader(getter_AddRefs(context->mDownloader), this, file);
> > > +
> > > +  context->mDownloader->OnStartRequest(request, ctxt);
> > 
> > interesting use... does this really work?  are you sure this use is not in
> > any way fragile?
> > 
> 
> Seems to work. I wrote it like this because while I could get the headers
> for the downloaded file in OnDownloadComplete, I also needed the headers
> from the cache, and it seemed cleaner to do it like this.
> I don't think it would be fragile in any way, and the extra function call
> shouldn't be that much of a burden performance-wise.

I would more expect to use the downloader so that is calls on us and not that we forward to the downloader (actually I would expect the downloader's implementation of nsIStreamListener/nsIRequsetObserver be private, or considered private.)
After some discussion at our Portland meeting, this bug is taking a different, broader turn: we're now trying to support the general case of packaging HTML pages/apps. I.e not just B2G apps, but also cases like a way to email a web page (and all of its subresources) easily.  We want a general package format for the web.

Zip is out for various reasons (mostly because it doesn't support streaming well: you have to download the whole zip before you get the index to access subresources, and we want to be able to serve resources from the package as it's being downloaded, for faster response times). The current suggestion from Sicking is to use multipart/MIME as the format: it could optionally contain a part that has an index with offsets for other resources.

We'll need to use a parser to chop the multipart message into separate files.  It's an open question as to whether we 1) keep the package file around, build an index for it, and use that to serve subresource channel requests, or if 2) we parse the file once (while it downloads) and store each subresource into the regular HTTP cache, and then we can discard the package itself.  I haven't thought through all aspects of this, but load time and ease of eviction both come to mind:

1) Keep package file around:
- When request for first resource is made, it's likely that we'll want most/all of the other resources, so we could aggressively preload them (and that preload can have nice I/O linear scan).
- When we evict we have only one file to evict. For B2G apps (which will have some special identifier so we can recognize them) this is easy: it happens when we uninstall. Otherwise we should probably evict things as "normal", depending on the usual max-age/expires/etc headers.  This would presumably require either storing the package file itself in the HTTP cache, or a meta-data-only entry for it (that gets updated with most recently accessed timestamps, like a regular cache entry), and evict based on that.

2) Split package into separate cache files (plus a special entry for the package's URI that contains only meta-data and a list of the files in the package)
- Serving requests is just like any other HTTP request.
- We could still do a preload of all resources when the 1st resource is requested (using the list of resources)
- Eviction: for non-B2G apps we probably still want to evict all resources in a package all at once, and also based on the most recently used timestamp of any of those resources.  One way that could work is that any use of a subresource would also update the last-access time for the package meta-data entry, and the eviction algorithm could learn to check the meta-data entry as well when it would normally check a subresource.

For storing things in memory we could possibly use the HTTP RAM cache, or Patrick volunteered the HTTP/2 push cache, or possibly the internals of the HTTP disk cache already have some buffer area that could be used. (We'll need to have some limit of how much memory a package can store in RAM before we give up and just store on disk only).

This changes the architecture of this bug a fair amount--there's no longer any need to use JAR at all, nor will we ship an fd to the child***.  Instead child requests will work mostly as regular HTTP channels do (i.e stream the bytes across IPDL), except that we'll need to have some notion of "package download in progress" and queue requests for subresources in the package until the subresource they want is available.

*** If we keep the package file around I guess we could use an fd and have the child seek and read from it.  But I'm guessing we don't want to take this route unless we see that streaming data over IPDL is too slow.
Summary: Support loading inner resources from http served zips → Support http-served packaged HTML pages/apps
This sure sounds quite like https://en.wikipedia.org/wiki/MHTML to me.
:freddyb is right; after looking at RFC 2557, there is a lot of overlap.  All the basic functions we need are there with one fairly major exception (maybe two).  The identity of resources is different.  Content in MHTML is identified using cid: URIs.

Here, the intent is to provide an identifier for each resource that can be merged into the encompassing space (which is likely an https: or http: URI, but could also be file: or anything that has a path component).  Even without merging, resources can be identified within the package in a way that can be transparently backward-compatible with the web.  The only major risk is that we haven't identified a separator that is sufficiently distinguished.

But format is a secondary concern.  If we find a better format that accomplishes those ends, that's great.  Of course, we don't want to chop and change any more than necessary.

An important task is to find a sequence of character that has a very low probability of appearing on the end of a path component on any existing server.  I fear that '!/' (with a trailing '/', of course) is not a sufficiently distinguished string, but testing should help with that.

A less important task is finding enough momentum in the standards bodies to make a change to the URI spec to support this idea of subordinate resources.  We intend to at least prove viability first though.

Another key aspect is getting the Vary semantics right for caches.  That's going to make content negotiation one of the things that performs worse as a result of packaging.  As envisaged, you won't be able to have different Accept/Accept-Language/etc... header fields acting on each of the subresources.

Finally, there are some risks around authority, but we are pretty sure that those can be safely contained.
The other thing we could do right away is to gather telemetry about how often URLs with "!//" or "!/" in them appear.

A simple way to do that is to count how many nsIURIs we create, and how many of them contain the string "!//". Then send those two numbers back. A possibly slightly more meaningful thing to measure is how many pages that use "!//" URLs, out of the total number of pages we load. That more clearly indicates how much of the web would break if we made this change.
> The other thing we could do right away is to gather telemetry about how often URLs with "!//" or "!/" in them appear.

Valentin: open a new bug for this telemetry and let's get that part of the work here moving again.
Flags: needinfo?(valentin.gosu)
Depends on: 1120216
Flags: needinfo?(valentin.gosu)
Depends on: 1120985
Also makes nsMultiMixedConv/nsPartChannel save and return individual headers for each part of the resource file.
Attachment #8549216 - Flags: review?(honzab.moz)
Comment on attachment 8549216 [details] [diff] [review]
Allow nsMultiMixedConv to compute its boundary if content-type=application/package

This patch belongs in bug 1120985
Attachment #8549216 - Attachment is obsolete: true
Attachment #8549216 - Flags: review?(honzab.moz)
Attached patch Add Packaged App Service (obsolete) — — Splinter Review 

        Attachment #8553154 -
        Flags: feedback?(honzab.moz)

    
    

    
  
Needs more work, and more comments, but it kind of works :)

A summary of how it works is this:

nsHttpChannel::BeginConnect() sees that there's a !// in the URL and calls nsIPackagedAppService::requestURI(nsIURI, nsICacheEntryOpenCallback, nsILoadContextInfo)

PackagedAppService::RequestURI creates CacheEntryChecker that checks if we already have that entry in the cache. If yes, it just calls the callbacks, else, it calls to PackagedAppService::OpenNewPackageInternal

PackagedAppService::OpenNewPackageInternal creates a new channel to start downloading the package, and PackagedAppDownloader is in charge of gettin OnStart/StopRequest and OnDataAvailable for all the resources in the package (they will be called multiple times)

For each resource, a CacheEntryWriter object is created, which creates a new cache entry, opens up the outputStream, and writes things into the cache entry.

After OnStopRequest is recevied, it calls the callbacks, which would make nsHttpChannel start using the cache entry.

After the package is downloaded, the remaining callbacks are called with NS_ERROR_FILE_NOT_FOUND.


There are a bunch of things left to do:
* figure out if the entries need to be pinned in the cache
* presumably, the package would also contain a list of files it contains. We should create a cache entry for the package, that contains the list and other relevant data, so we don't need to redownload a package in order to figure out the file is not there.
* resolve all of the TODOs left in the code.
* figure out when the entries are expired, and we need to get an update version of the package. (includes deleting entries that have been removed from the package)
* ???

    
    

    
  
Comment on attachment 8553154 [details] [diff] [review]
Add Packaged App Service

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

In general, the approach looks pretty good!

::: netwerk/base/public/nsIPackagedAppService.idl
@@ +20,5 @@
> +interface nsIPackagedAppService : nsISupports
> +{
> +  /**
> +   * @uri is a URL that looks like http://example.com/path/to/package!//resource.html
> +   * @principal is the original principal that requested the resource

update the comment

@@ +22,5 @@
> +  /**
> +   * @uri is a URL that looks like http://example.com/path/to/package!//resource.html
> +   * @principal is the original principal that requested the resource
> +   */
> +  void requestURI(in nsIURI uri, in nsICacheEntryOpenCallback callback, [optional] in nsILoadContextInfo info);

if you want to work with the cache API in this method implementation nsILoadContextInfo must not be optional.

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +16,5 @@
> +CacheEntryWriter::ConsumeData(nsIInputStream *in, void *closure,
> +                         const char *fromRawSegment, uint32_t toOffset,
> +                         uint32_t count, uint32_t *writeCount)
> +{
> +  CacheEntryWriter *self = (CacheEntryWriter *)closure;

static cast

@@ +18,5 @@
> +                         uint32_t count, uint32_t *writeCount)
> +{
> +  CacheEntryWriter *self = (CacheEntryWriter *)closure;
> +  if (self->mOutputStream)
> +    return self->mOutputStream->Write(fromRawSegment, count, writeCount);

I a don't much see a reason (code path) mOutputStream would ever be null.  Based on that I don't see a reason why not to just go directly, but if there's rational, just put a comment ;)

@@ +40,5 @@
> +CacheEntryWriter::OnCacheEntryAvailable(nsICacheEntry *aEntry, bool aNew,
> +                                   nsIApplicationCache *aApplicationCache,
> +                                   nsresult aResult)
> +{
> +  MOZ_ASSERT(aNew, "This should be a new entry");

use nsICacheStorage.openTruncate that gives you a new entry synchronously (at no cost), then no callback, no assertions :)

@@ +58,5 @@
> +
> +  nsCOMPtr<nsIResponseHeadProvider> provider(do_QueryInterface(aRequest));
> +
> +  nsHttpResponseHead *responseHead = provider->GetResponseHead();
> +  mEntry->SetPredictedDataSize(responseHead->TotalEntitySize()); // TODO: is this needed

We have some rules to not cache entries that are larger then some limit (preferable).  So, yes, unless you want to enforce caching of large content (which I don't suggest), this is needed.

@@ +70,5 @@
> +
> +  rv = mEntry->SetMetaDataElement("request-method", "GET");
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }

here some expiration time play may need to happen as well.  I'll respond later on this, you may also check nsHttpChannel::UpdateExpirationTime.

@@ +117,5 @@
> +nsresult
> +PackagedAppDownloader::CreateCacheEntry(nsIURI *uri, nsICacheEntryOpenCallback* callback)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsICacheStorageService> cacheStorageService =

maybe cache the service?

@@ +125,5 @@
> +  }
> +
> +  nsCOMPtr<nsICacheStorage> cacheStorage;
> +  rv = cacheStorageService->DiskCacheStorage(mLoadContextInfo, false,
> +                                             getter_AddRefs(cacheStorage));

check result!

@@ +128,5 @@
> +  rv = cacheStorageService->DiskCacheStorage(mLoadContextInfo, false,
> +                                             getter_AddRefs(cacheStorage));
> +
> +  cacheStorage->AsyncOpenURI(uri, EmptyCString(),
> +                             nsICacheStorage::OPEN_TRUNCATE, callback);

openTruncate, for our regular HTTP disk cache it's guarantied to work.  this code should be on the CacheEntryWriter class.

@@ +209,5 @@
> +{
> +  nsAutoCString spec;
> +  uri->GetSpec(spec);
> +
> +  nsCOMArray<nsICacheEntryOpenCallback>* array = mCallbacks.Get(spec);

assert thread (main?) when manipulating with mCallbacks or have a lock over it

@@ +212,5 @@
> +
> +  nsCOMArray<nsICacheEntryOpenCallback>* array = mCallbacks.Get(spec);
> +  if (array) {
> +    // Call all the callbacks for this URI
> +    for (uint32_t i=0; i<array->Length(); ++i) {

i = 0; ...

@@ +214,5 @@
> +  if (array) {
> +    // Call all the callbacks for this URI
> +    for (uint32_t i=0; i<array->Length(); ++i) {
> +      uint32_t result;
> +      array->ObjectAt(i)->OnCacheEntryCheck(mWriter->mEntry, nullptr, &result);

please pick the object to a local nsRefPtr.

@@ +219,5 @@
> +      array->ObjectAt(i)->OnCacheEntryAvailable(mWriter->mEntry, false, nullptr, aResult);
> +    }
> +    // Clear the array and remove it from the hashtable
> +    array->Clear();
> +    mCallbacks.Remove(spec);

do this as the first

@@ +225,5 @@
> +  return NS_OK;
> +}
> +
> +PLDHashOperator
> +Enumerator(const nsACString& key,

better name, anon namespace or static as a class member (preferred)

@@ +230,5 @@
> +           nsAutoPtr<nsCOMArray<nsICacheEntryOpenCallback> >& callbackArray,
> +           void* arg)
> +{
> +  for (uint32_t i=0; i<callbackArray->Length(); ++i) {
> +    // TODO: use result

via arg?

@@ +241,5 @@
> +nsresult
> +PackagedAppDownloader::ClearCallbacks(nsresult aResult)
> +{
> +  // TODO: pass aResult
> +  mCallbacks.Enumerate(Enumerator, nullptr);

&aResult? :)

@@ +280,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PackagedAppService::RequestURI(nsIURI *uri, nsICacheEntryOpenCallback *callback,
> +                               nsILoadContextInfo *loadInfo)

use aArg form

@@ +296,5 @@
> +  }
> +
> +  nsCOMPtr<nsILoadContextInfo> info = loadInfo;
> +  if (!info)
> +    info = GetLoadContextInfo(false, 0, false, false);

{}

and.. hmm... assuming default is bad.  you should require loadInfo be passed.

@@ +301,5 @@
> +
> +  nsresult rv;
> +  nsCOMPtr<nsICacheStorageService> cacheStorageService =
> +    do_GetService("@mozilla.org/netwerk/cache-storage-service;1", &rv);
> +

no blank line (confuses)

@@ +309,5 @@
> +
> +  nsCOMPtr<nsICacheStorage> cacheStorage;
> +
> +  rv = cacheStorageService->DiskCacheStorage(info, false,
> +                                             getter_AddRefs(cacheStorage));

rv check

@@ +328,5 @@
> +  uri->GetSpec(spec);
> +
> +  nsPrintfCString key("{%d,%u,%d,%d}{%s}", info->IsPrivate(), info->AppId(),
> +                      info->IsInBrowserElement(), info->IsAnonymous(),
> +                      spec.get());

please use AppendKeyPrefix from cache2/CacheFileUtils.h

@@ +345,5 @@
> +             "This should never be called if the token is missing");
> +
> +  nsCOMPtr<nsIURI> packageURI;
> +  uri->CloneIgnoringRef(getter_AddRefs(packageURI));
> +  packageURI->SetPath(Substring(path, 0, pos));

hm.. and what if !// is part of # string?

@@ +350,5 @@
> +
> +  nsCString key = computeKey(packageURI, info);
> +
> +  nsRefPtr<PackagedAppDownloader> listener;
> +  if (mDownloadingPackages.Get(key, getter_AddRefs(listener))) {

again, assert thread or you have to lock

::: netwerk/protocol/http/PackagedAppService.h
@@ +1,1 @@
> +#ifndef mozilla_net_PackagedAppService_h

license

@@ +10,5 @@
> +// Creates a new cache entry
> +// Receives OnStartRequest/OnStopRequest/OnDataAvailable
> +// and writes them to the cache entry's stream
> +// Calls the callback giving the entry once it receives OnStopRequest
> +class CacheEntryWriter MOZ_FINAL

nit, up to you to decide: these helpers may be in some mozilla::net::detail namespace or just nested in the service class.

@@ +55,5 @@
> +  nsresult CallCallbacks(nsIURI *uri, nsICacheEntry *aEntry, nsresult aResult);
> +  nsresult CreateCacheEntry(nsIURI *uri, nsICacheEntryOpenCallback* callback);
> +  nsresult ClearCallbacks(nsresult aResult);
> +
> +  nsClassHashtable<nsCStringHashKey, nsCOMArray<nsICacheEntryOpenCallback>> mCallbacks;

comments on all in this 'private' part will be needed

@@ +90,5 @@
> +  nsCOMPtr<nsILoadContextInfo> mLoadContextInfo;
> +
> +private:
> +  ~CacheEntryChecker() { }
> +};

please write comments about what is the lifetime of each class, how are invoked, how are used, how are chained (e.g. whole is CacheEntryChecker::mCallback etc..)

::: netwerk/protocol/http/moz.build
@@ +33,5 @@
>      'HttpChannelChild.h',
>      'HttpChannelParent.h',
>      'HttpInfo.h',
>      'NullHttpChannel.h',
> +    'PackagedAppService.h',

really needs to be exported?  you can include ../protocol/http/ as well.

        Attachment #8553154 -
        Flags: feedback?(honzab.moz) → feedback+

    
    

    
  
Comment on attachment 8553159 [details] [diff] [review]
Change nsHttpChannel to call into PackagedAppService for urls containing !//

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

Let's see what break :))

        Attachment #8553159 -
        Flags: feedback?(honzab.moz) → feedback+

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #59)
> > +
> > +  nsCOMPtr<nsIURI> packageURI;
> > +  uri->CloneIgnoringRef(getter_AddRefs(packageURI));
> > +  packageURI->SetPath(Substring(path, 0, pos));
> 
> hm.. and what if !// is part of # string?

That should not affect this, as GetPath does not return the #part at all.
The only problem at the moment is that packages containing other packages doesn't work, but I think we can probably fix that use case too.
> > +// Calls the callback giving the entry once it receives OnStopRequest
> > +class CacheEntryWriter MOZ_FINAL
> 
> nit, up to you to decide: these helpers may be in some mozilla::net::detail
> namespace or just nested in the service class.

I chose mozilla::net::packaged.
Let me know if you think there might be a better name.

> ::: netwerk/protocol/http/moz.build
> @@ +33,5 @@
> >      'NullHttpChannel.h',
> > +    'PackagedAppService.h',
> 
> really needs to be exported?  you can include ../protocol/http/ as well.

It's needed for the service constructor in nsNetModule.cpp...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Add Packaged App Service (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8562195 -
        Flags: review?(honzab.moz)

    
  

        Attachment #8553154 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Valentin Gosu [:valentin] from comment #61)
> > really needs to be exported?  you can include ../protocol/http/ as well.
> 
> It's needed for the service constructor in nsNetModule.cpp...

Yep, that's what I mean, you can include via a relative path.

    
    

    
  
Comment on attachment 8562195 [details] [diff] [review]
Add Packaged App Service

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

::: netwerk/base/nsIPackagedAppService.idl
@@ +19,5 @@
> +[scriptable, builtinclass, uuid(77f9a34d-d082-43f1-9f83-e852d0173cd5)]
> +interface nsIPackagedAppService : nsISupports
> +{
> +  /**
> +   * @aURI is a URL that looks like http://example.com/path/to/package!//resource.html

"looks like" is a little bit non-descriptive ;)

@@ +20,5 @@
> +interface nsIPackagedAppService : nsISupports
> +{
> +  /**
> +   * @aURI is a URL that looks like http://example.com/path/to/package!//resource.html
> +   * @aCallback is an object implementing nsICacheEntryOpenCallback

and what is it expect to get? when? what is the lifetime? and why this is even here?  so many important things to tell here...

@@ +21,5 @@
> +{
> +  /**
> +   * @aURI is a URL that looks like http://example.com/path/to/package!//resource.html
> +   * @aCallback is an object implementing nsICacheEntryOpenCallback
> +   * @aInfo is an object used to determine the cache jar this resource goes in.

where from is aInfo usually taken?

::: netwerk/build/nsNetCID.h
@@ -855,5 @@
>   */
>  #define NS_STARTTLSSOCKETPROVIDER_CONTRACTID \
>      NS_NETWORK_SOCKET_CONTRACTID_PREFIX "starttls"
>  
> -

no ws only changes please

::: netwerk/build/nsNetModule.cpp
@@ +248,5 @@
>  }
>  }
>  #endif // !NECKO_PROTOCOL_http
>  
> +

no ws only

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +15,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +using namespace mozilla::net::packaged;

rather typedef this in the service class and to define the ::packaged class members just open the namespace?

@@ +25,5 @@
> +
> +CacheEntryWriter::CacheEntryWriter(nsIURI* aURI, nsICacheStorage* aStorage)
> +{
> +  aStorage->OpenTruncate(aURI, EmptyCString(), getter_AddRefs(mEntry));
> +  mEntry->OpenOutputStream(0, getter_AddRefs(mOutputStream));

oh, please check for results.  probably not good idea to do this in a ctor, anyway.

@@ +31,5 @@
> +
> +NS_METHOD
> +CacheEntryWriter::ConsumeData(nsIInputStream *in, void *closure,
> +                         const char *fromRawSegment, uint32_t toOffset,
> +                         uint32_t count, uint32_t *writeCount)

nit: indent

@@ +35,5 @@
> +                         uint32_t count, uint32_t *writeCount)
> +{
> +  CacheEntryWriter *self = static_cast<CacheEntryWriter*>(closure);
> +  MOZ_ASSERT(self->mOutputStream, "The stream should not be null");
> +  return self->mOutputStream->Write(fromRawSegment, count, writeCount);

rather check non-null as well?

@@ +43,5 @@
> +CacheEntryWriter::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext)
> +{
> +  nsCOMPtr<nsIResponseHeadProvider> provider(do_QueryInterface(aRequest));
> +  nsHttpResponseHead *responseHead = provider->GetResponseHead();
> +  mEntry->SetPredictedDataSize(responseHead->TotalEntitySize());

you don't like checking return results?

@@ +65,5 @@
> +CacheEntryWriter::OnStopRequest(nsIRequest *aRequest, nsISupports *aContext,
> +                           nsresult aStatusCode)
> +{
> +  mOutputStream->Close();
> +  mOutputStream = nullptr;

null check

@@ +76,5 @@
> +                             uint32_t aCount)
> +{
> +  // Calls ConsumeData to read the data into the cache entry
> +  uint32_t n;
> +  return aInputStream->ReadSegments(ConsumeData, this, aCount, &n);

null check

@@ +92,5 @@
> +    return rv;
> +  }
> +
> +  rv = cacheStorageService->DiskCacheStorage(aInfo, false,
> +                                             getter_AddRefs(mCacheStorage));

hmm... what if the package is served with Cache-control: no-store?  And how do we handle no-store when present in the part responses?

@@ +126,5 @@
> +  if (!provider || !chan) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  nsHttpResponseHead *responseHead = provider->GetResponseHead();

check result

@@ +154,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PackagedAppDownloader::OnStopRequest(nsIRequest *aRequest, nsISupports *aContext,
> +                                nsresult aStatusCode)

nit: indent

@@ +162,5 @@
> +  // The request is normally a multiPartChannel. If it isn't, it generally means
> +  // an error has occured (such as a 404). Just pass the error to all listeners
> +  // and end the package load.
> +  if (!multiChannel) {
> +    ClearCallbacks(aStatusCode);

what if the status is not a failure here?  404 doesn't imply it, actually quite otherwise

@@ +165,5 @@
> +  if (!multiChannel) {
> +    ClearCallbacks(aStatusCode);
> +    if (gPackagedAppService) {
> +      gPackagedAppService->NotifyPackageDownloaded(mPackageKey);
> +    }

can this be done before calling callbacks?

@@ +183,5 @@
> +    return rv;
> +  }
> +
> +  CallCallbacks(uri, mWriter->mEntry, aStatusCode);
> +  mWriter = nullptr;

swap mWriter to local comptr before CallCallbacks (just for reentrancy case)

@@ +195,5 @@
> +  // This is the last part.
> +  ClearCallbacks(NS_ERROR_FILE_NOT_FOUND);
> +  if (gPackagedAppService) {
> +    gPackagedAppService->NotifyPackageDownloaded(mPackageKey);
> +  }

here as well (swap order)

@@ +262,5 @@
> +{
> +  MOZ_ASSERT(arg, "The void* parameter should be a pointer to nsresult");
> +  nsresult *result = static_cast<nsresult*>(arg);
> +  for (uint32_t i = 0; i < callbackArray->Length(); ++i) {
> +    nsCOMPtr<nsICacheEntryOpenCallback> callback = callbackArray->ObjectAt(i);

no need for nsCOMPtr here (and elsewhere)

@@ +284,5 @@
> +CacheEntryChecker::OnCacheEntryCheck(
> +  nsICacheEntry *aEntry, nsIApplicationCache *aApplicationCache,
> +  uint32_t *_retval)
> +{
> +  return mCallback->OnCacheEntryCheck(aEntry, aApplicationCache, _retval);

if mCallback rejects the entry then you get NS_ERROR_CACHE_KEY_NOT_FOUND in OCEA bellow.  just a note, maybe that's what you want and it is correct.

@@ +364,5 @@
> +PackagedAppService::OpenNewPackageInternal(nsIURI *uri,
> +                                           nsICacheEntryOpenCallback *callback,
> +                                           nsILoadContextInfo *info)
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "mDownloadingPackages hashtable is not thread safe");

thinking of making these RELEASE_ASSERTs...

@@ +374,5 @@
> +             "This should never be called if the token is missing");
> +
> +  nsCOMPtr<nsIURI> packageURI;
> +  uri->CloneIgnoringRef(getter_AddRefs(packageURI));
> +  packageURI->SetPath(Substring(path, 0, pos));

please... check results

@@ +378,5 @@
> +  packageURI->SetPath(Substring(path, 0, pos));
> +
> +  nsAutoCString key;
> +  packageURI->GetSpec(key);
> +  CacheFileUtils::AppendKeyPrefix(info, key);

first, please use GetAsciiSpec, second, CacheFileUtils::AppendKeyPrefix should be called as the first on the resulting string.

@@ +380,5 @@
> +  nsAutoCString key;
> +  packageURI->GetSpec(key);
> +  CacheFileUtils::AppendKeyPrefix(info, key);
> +
> +  nsRefPtr<PackagedAppDownloader> listener;

nit: maybe call this "downloader" ?

@@ +395,5 @@
> +  nsCOMPtr<nsIChannel> channel;
> +  nsresult rv = NS_NewChannel(
> +    getter_AddRefs(channel), packageURI, nsContentUtils::GetSystemPrincipal(),
> +    nsILoadInfo::SEC_NORMAL, nsIContentPolicy::TYPE_OTHER, nullptr, nullptr,
> +    nsIRequest::LOAD_NORMAL);

jonas or somebody should review this from the security point of view (is HSTS enforced?  should we follow redirects? (https->http),  what about mixed content detection and blocking?  what about CORS?

@@ +405,5 @@
> +  nsCOMPtr<nsIStreamConverterService> streamconv =
> +    do_GetService("@mozilla.org/streamConverters;1", &rv);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }

declare before use

@@ +417,5 @@
> +  listener->AddCallback(uri, callback);
> +
> +  nsCOMPtr<nsIStreamListener> mimeConverter;
> +  rv = streamconv->AsyncConvertData("multipart/mixed", "*/*", listener, nullptr,
> +                               getter_AddRefs(mimeConverter));

nit indent

::: netwerk/protocol/http/PackagedAppService.h
@@ +15,5 @@
> +namespace net {
> +
> +// This namespace contains all the helper classes we use to implement
> +// the packaged app service.
> +namespace packaged {

hmm... ::detail or nesting still seem better to me
do you see specific sub-namespaces for impl details widely used in the code around?

@@ +44,5 @@
> +  nsCOMPtr<nsIOutputStream> mOutputStream;
> +};
> +
> +class PackagedAppDownloader MOZ_FINAL
> +  : public nsIStreamListener

comments: what is lifetime of this class?  who references it?  what is the scope of this class?

@@ +53,5 @@
> +  NS_DECL_NSIREQUESTOBSERVER
> +
> +  // Initializes mCacheStorage and saves aKey as mPackageKey which is later
> +  // used to remove this object from PackagedAppService::mDownloadingPackages
> +  nsresult Init(nsILoadContextInfo* aInfo, nsCString aKey);

s/nsCString aKey/nsCString const &aKey/
what is the typical caller?

@@ +55,5 @@
> +  // Initializes mCacheStorage and saves aKey as mPackageKey which is later
> +  // used to remove this object from PackagedAppService::mDownloadingPackages
> +  nsresult Init(nsILoadContextInfo* aInfo, nsCString aKey);
> +  // Registers a callback which gets called when the given nsIURI is downloaded
> +  nsresult AddCallback(nsIURI *uri, nsICacheEntryOpenCallback *callback);

what is |uri| exactly? (also, use aURI)

@@ +60,5 @@
> +
> +private:
> +  ~PackagedAppDownloader() { }
> +
> +  // Calls all the callbacks registered for the given URI.

URI here is the package, right?  please express that in the comment

@@ +66,5 @@
> +  nsresult CallCallbacks(nsIURI *uri, nsICacheEntry *aEntry, nsresult aResult);
> +  // Clears all the callbacks for this package
> +  // This would get called at the end of downloading the package and would
> +  // cause us to call OnCacheEntryAvailable with a null entry. This would be
> +  // equivalent to a 404 when loading from the net.

hmm... how is actually the 404 error propagated to the waiting channels?  we should propagate to target consumers I think (not needed to be done in this bug, but land should at least not be ruined for it)

@@ +69,5 @@
> +  // cause us to call OnCacheEntryAvailable with a null entry. This would be
> +  // equivalent to a 404 when loading from the net.
> +  nsresult ClearCallbacks(nsresult aResult);
> +  static PLDHashOperator ClearCallbacksEnumerator(const nsACString& key,
> +    nsAutoPtr<nsCOMArray<nsICacheEntryOpenCallback>>& callbackArray,

nsTArray<nsCOMPtr> please

@@ +71,5 @@
> +  nsresult ClearCallbacks(nsresult aResult);
> +  static PLDHashOperator ClearCallbacksEnumerator(const nsACString& key,
> +    nsAutoPtr<nsCOMArray<nsICacheEntryOpenCallback>>& callbackArray,
> +    void* arg);
> +  // Returns a URI with the PartChannel's full URI

what is "the PartChannel" ?  from this comment I don't know what this method does

@@ +80,5 @@
> +  nsRefPtr<CacheEntryWriter> mWriter;
> +  // Cached value of nsICacheStorage
> +  nsCOMPtr<nsICacheStorage> mCacheStorage;
> +  // A hastable containing all the consumers which requested a URI and need
> +  // to be notified once it is inserted into the cache.

what is the key?

@@ +126,5 @@
> +
> +private:
> +  ~PackagedAppService();
> +
> +  // This method is called if an entry wasn't found in the cache.

what entry? what happens? what is the reason this has to be called in that case?

@@ +132,5 @@
> +  // If so, then it simply adds the callback to that PackageAppDownloader
> +  // Else it begins downloading the new package and adds it to mDownloadingPackages
> +  nsresult OpenNewPackageInternal(nsIURI *uri,
> +                                  nsICacheEntryOpenCallback *callback,
> +                                  nsILoadContextInfo *info);

maybe also describe the args, mainly why info is needed

@@ +136,5 @@
> +                                  nsILoadContextInfo *info);
> +
> +  // Called by PackageAppDownloader once the download has finished
> +  // (or encountered an error) to remove the package from mDownloadingPackages
> +  nsresult NotifyPackageDownloaded(nsCString aKey);

on what threads is the hash table manipulation happening?

::: netwerk/test/unit/test_packaged_app_service_404.js
@@ +1,1 @@
> +Cu.import("resource://testing-common/httpd.js");

again, please write description to your tests...

@@ +45,5 @@
> +  onCacheEntryCheck: function() { return Ci.nsICacheEntryOpenCallback.ENTRY_WANTED; },
> +  onCacheEntryAvailable: function (entry, isnew, appcache, status) {
> +    // TODO: it returns NS_ERROR_FAILURE for a missing package
> +    // and NS_ERROR_FILE_NOT_FOUND for a missing file from the package.
> +    // Maybe make them both return NS_ERROR_FILE_NOT_FOUND?

I'd leave it

@@ +60,5 @@
> +  isAnonymous: false
> +}
> +
> +function test_package_does_not_exist() {
> +  paservice.requestURI(createURI(uri+"/package0!//index.html"), listener404, loadContextInfo);

package-non-existent?

@@ +64,5 @@
> +  paservice.requestURI(createURI(uri+"/package0!//index.html"), listener404, loadContextInfo);
> +}
> +
> +function test_file_does_not_exist() {
> +  paservice.requestURI(createURI(uri+"/package!//fake.html"), listener404, loadContextInfo);

s/fake here as well..

::: netwerk/test/unit/test_packaged_app_service_cached_entry.js
@@ +9,5 @@
> +  isPrivate: false,
> +  appId: 0,
> +  isInBrowserElement: false,
> +  isAnonymous: false
> +}

nit: there is LoadContextInfo.jsm module for this

::: netwerk/test/unit/test_packaged_app_service_simple.js
@@ +84,5 @@
> +}
> +
> +function test_bad_args() {
> +  Assert.throws(() => { paservice.requestURI(createURI("http://test.com"), cacheListener, loadContextInfo); }, "url's with no !// aren't allowed");
> +  Assert.throws(() => { paservice.requestURI(createURI("http://test.com/package!//test"), null, loadContextInfo); }, "should have a callbak");

callbak

::: netwerk/test/unit/xpcshell.ini
@@ +314,5 @@
>  [test_safeoutputstream_append.js]
> +[test_packaged_app_service_simple.js]
> +[test_packaged_app_service_cached_entry.js]
> +[test_packaged_app_service_404.js]
> +

no new line at the end

        Attachment #8562195 -
        Flags: review?(honzab.moz) → feedback-

    
    

    
  
Hi Jonas,

So it's to my understanding that a package should contain a "manifest" with the list of files in that package. The manifest should be streamed first, so that we know if a file is in the package or not.
What format do we want for the manifest? JSON? Or maybe \r\n separated file list?

What I'm pondering is the process by which we get a 404.
So if we look for the cache entry and we don't find it, then we check if the Last Modified tag. If the package hasn't changed, we look to see if the file is in the package file list, and if it isn't, that's a 404.
If the package has changed, that means a new file might have been added to the package. We start downloading the package. After we get the manifest, we check the file list (maybe async'ly). If the file still isn't in the list, we 404, or else we keep streaming until we finally download the file.

Does that sound fine?
Flags: needinfo?(jonas)

    
    

    
  
(In reply to Valentin Gosu [:valentin] from comment #65)
> Hi Jonas,
> 
> So it's to my understanding that a package should contain a "manifest" with
> the list of files in that package. The manifest should be streamed first, so
> that we know if a file is in the package or not.

I don't actually think that we need that from the package format itself.

Here's what I was thinking:

For normal browsing no manifest is involved. You can simply browse to any page inside the package using a "!//" URL and "run the app" that way. We wouldn't verify that the package contains any particular resources.

If you then want to install the app, we do the following. First we look for a "manifest" header in the package headers (name to be bikeshed). If one is found, we pin the app package in our http cache to make sure that it's available offline. We also use the file that the header points to as the app manifest.

The identity of such an app should likely be the URL of the app package rather than the URL of the manifest. But I'm not fully sure about that. Either way this mostly affects the app code, not the packaging code.

Again through, we wouldn't verify that the package contains any particular resources. I.e. the manifest does not contain any list of resources.


However, when an installation happens, we should look at the manifest to see if it's indicating that the app is a privileged app. If so we should trigger the package signing verification code. This code will verify that the package is properly signed and that the list of signatures matches the list of files in the package. If the signing check fails, we abort installation.

But this only affects the installation code. I.e. normally browsing to such a package would still work fine. And it's not a simple list of files but rather something that contains various signatures as well.


But there is a caveats. In the future we'll probably want to change the signature-checks such that we can do them at browsing-time as well. Rather than just at time of installation. But this will involve other gecko changes as well. Such as figuring out what origins these privileged packages will have. And how to get data into the nsIPermissionManager when you browse to them. So not something that we should do for a first pass of packaging support.

But if you think that it's best to write the package code to support something like this from the get-go we should pull together people and figure out what that should look like.
Flags: needinfo?(jonas)

    
    

    
  
Thanks for writing down that explanation Jonas, I've been trying to figure out how this is intended to work for a while now.

From your description (and previous comments), the way I'm now imagining a signed hosted packaged app is as a nested multipart MIME message - a multipart/mixed message containing multiple multipart/signed messages which contain individual resources and their signature, where one of those resources is an app manifest.

I'm wondering how you imagine the signing process will work for these apps? Will they still need to be centrally signed by Mozilla? If so, I imagine that the app author needs to send some kind of packaged unsigned version of their app (e.g. a zip file or a multipart/mixed MIME message) to the Marketplace and then have Mozilla send them back a signed version which contains signatures for each resource?

Or will developers be able to sign their own packages?

    
    

    
  
Lets take the discussion about signing format and signing process elsewhere. It shouldn't affect the package format.

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #66)
> But there is a caveats. In the future we'll probably want to change the
> signature-checks such that we can do them at browsing-time as well. Rather
> than just at time of installation.

It seems that in the first phase, this package app can't access privilieged API at browsing-time.

I have a question when sinature-checks is enabled at browsing-time for accessing privileged APIs.
Q: In this case, maybe I can let web pages not in this package access privileged APIs. 
  a. 
    - To have privileged package app with permissions.
    - This app creates an i-frame to external page and in the same origin.
    - Could this external page access the privileged API directly now?

  b. 
    - To have privileged package app with permissions.
    - This app creates an i-frame to external page and in the same origin.
    - Could this external page use postMessage to ask app for excuting some privileged API?

Thanks.

    
    

    
  
Let's take the discussion about how to do signing elsewhere. I'd rather keep this bug just to the issue of having a streamable package format which allows addressing resources inside the package with real URLs.

Handling of signing should be relatively orthogonal to the package format so we can discuss that independently.

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #64)
> @@ +395,5 @@
> > +  nsCOMPtr<nsIChannel> channel;
> > +  nsresult rv = NS_NewChannel(
> > +    getter_AddRefs(channel), packageURI, nsContentUtils::GetSystemPrincipal(),
> > +    nsILoadInfo::SEC_NORMAL, nsIContentPolicy::TYPE_OTHER, nullptr, nullptr,
> > +    nsIRequest::LOAD_NORMAL);
> 
> jonas or somebody should review this from the security point of view (is
> HSTS enforced?  should we follow redirects? (https->http),  what about mixed
> content detection and blocking?  what about CORS?
> 

Jonas: apart from the above questions, I want to ask about how a package considers its root.
So in the example at https://w3ctag.github.io/packaging-on-the-web/#streamable-package-format , the page tries to load the script located at /scripts/app.js
Normally, this would mean domain/scripts/app.js, but in our context it would refer to domain/package.pak!//scripts/app.js - any clue on how I should approach this?
Flags: needinfo?(jonas)

    
    

    
  
:valentin, have you considered a link relation pointing to the root?  A relation type of "index" might fit: https://tools.ietf.org/html/rfc5988#section-6.2.2

    
    

    
  
(In reply to Valentin Gosu [:valentin] from comment #71)
> So in the example at
> https://w3ctag.github.io/packaging-on-the-web/#streamable-package-format ,
> the page tries to load the script located at /scripts/app.js

I'm not sure I 100% understand the question. But I'll give it a shot.

In the example a file inside the package contains markup like <script src="/scripts/app.js"></script>. This relative URL would be resolved according to normal URL resolution rules used today.

So if the markup appears in page.html inside a package at http://domain.com/folder/package.pak. That means that the page's URL would be http://domain.com/folder/package.pak!//page.html.

Resolving /scripts/app.js against http://domain.com/folder/package.pak!//page.html results in http://domain.com/scripts/app.js.

I.e. it refers to a resource outside of the package.

> Normally, this would mean domain/scripts/app.js, but in our context it would
> refer to domain/package.pak!//scripts/app.js - any clue on how I should
> approach this?

No, we should follow the normal rules. It would not refer to a resource inside the package. The W3C proposal does a lot of magic to URLs which I think is a bad idea. Our goal should not be to be compatible with the W3C URL handling, but to create a better alternative to it.
Flags: needinfo?(jonas)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Add Packaged App Service (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8576615 -
        Flags: review?(honzab.moz)

    
  

        Attachment #8562195 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #64)
> Comment on attachment 8562195 [details] [diff] [review]
> > +  rv = cacheStorageService->DiskCacheStorage(aInfo, false,
> > +                                             getter_AddRefs(mCacheStorage));
> 
> hmm... what if the package is served with Cache-control: no-store?  And how
> do we handle no-store when present in the part responses?

Ignoring them seems fair. These headers don't have much meaning in the context of a packaged app, since they are served from the cache :)
> @@ +165,5 @@
> > +  if (!multiChannel) {
> > +    ClearCallbacks(aStatusCode);
> > +    if (gPackagedAppService) {
> > +      gPackagedAppService->NotifyPackageDownloaded(mPackageKey);
> > +    }
> 
> can this be done before calling callbacks?

Yes. With an added kungFuDeathGrip(this), since NotifyPackageDownloaded removes the package from the hashtable.
> 
> hmm... ::detail or nesting still seem better to me
> do you see specific sub-namespaces for impl details widely used in the code
> around?

You're right. I nested the classes, and it seems a bit cleaner now. :)

> hmm... how is actually the 404 error propagated to the waiting channels?  we
> should propagate to target consumers I think (not needed to be done in this
> bug, but land should at least not be ruined for it)
> 

The error gets passed to the channel in OnCacheEntryAvailable.
NOT_CACHED makes it show the "your browser is offline" screen, but FILE_NOT_FOUND seems to be the one we want here.

> @@ +69,5 @@
> > +  // cause us to call OnCacheEntryAvailable with a null entry. This would be
> > +  // equivalent to a 404 when loading from the net.
> > +  nsresult ClearCallbacks(nsresult aResult);
> > +  static PLDHashOperator ClearCallbacksEnumerator(const nsACString& key,
> > +    nsAutoPtr<nsCOMArray<nsICacheEntryOpenCallback>>& callbackArray,
> 
> nsTArray<nsCOMPtr> please

mCallbacks.Enumerate expects an argument of this type, and nsCOMArray doesn't seem compatible with nsTArray<nsCOMPtr>

    
    

    
  

      
      
      
      

        Attached patch
          
          
        interdiff.patch (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
>> hmm... what if the package is served with Cache-control: no-store?  And how
>> do we handle no-store when present in the part responses?

>Ignoring them seems fair. These headers don't have much meaning in the context of a packaged app, since >they are served from the cache :)

I'd be more comfortable with "cache-control: no-store" on packaged apps being an error. That way a new feature won't be overriding an unrelated spec and flirting with a security boundary.

    
    

    
  
Note that this bug is not about packaged *apps*. This bug is mainly about supporting packaged content on the web.

For packaged content I agree that we should support all the normal cache control headers the normal way.

The only part that is about "apps" is when the user decides to pin a html page that was loaded from a package. I agree that scenario is a bit more complex when it comes to content that was served using "no-store", independent of if the page is packaged or not.

But I think user wishes should override server wishes. Unless we have explicit attack scenarios in mind.

    
    

    
  
Comment on attachment 8576615 [details] [diff] [review]
Add Packaged App Service

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

Rest on the idiff.

::: netwerk/base/nsIPackagedAppService.idl
@@ +27,5 @@
> +   *   - aCallback->OnCacheEntryCheck() is called to verify the entry is valid
> +   *   - aCallback->OnCacheEntryAvailable() is called with a pointer to the
> +   *     the cached entry, if one exists, or an error code otherwise
> +   *   - aCallback is kept alive using an nsCOMPtr until OnCacheEntryAvailable
> +   *     is called

few words this is actually target of the async result of the operation would be good.

@@ +31,5 @@
> +   *     is called
> +   * @aInfo is an object used to determine the cache jar this resource goes in.
> +   *   - usually created by calling GetLoadContextInfo(requestingChannel)
> +   */
> +  void requestURI(in nsIURI aURI, in nsICacheEntryOpenCallback aCallback, in nsILoadContextInfo aInfo);

nit, aInfo should either be first or second, not last.  It's the most significant determinant here.

also, outline of what the method does would be nice..

        Attachment #8576615 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 8576617 [details] [diff] [review]
interdiff.patch

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

Thanks for this idiff, it helps a lot!

I appreciate you are writing comments!  Please, now when you do it, take care of the content too.  It's hard, I know, needs some training, but it's important.  It's not always clear what is what when you refer things/arguments.  You must be very exact on the description and try to see the code as if you never seen it or seen it after a year and half and try to very quickly understand how this works.  It may not be 100% perfect, but 99% would be good :D


Few things left to update I think.

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +48,5 @@
> +                                                  void *closure,
> +                                                  const char *fromRawSegment,
> +                                                  uint32_t toOffset,
> +                                                  uint32_t count,
> +                                                  uint32_t *writeCount)

nit: try to stick with aArgument form please.

@@ +163,5 @@
>  {
>    nsCOMPtr<nsIResponseHeadProvider> provider(do_QueryInterface(aRequest));
>    nsCOMPtr<nsIChannel> chan(do_QueryInterface(aRequest));
>  
>    if (!provider || !chan) {

WARN_IF

@@ +168,5 @@
>      return NS_ERROR_INVALID_ARG;
>    }
>  
>    nsHttpResponseHead *responseHead = provider->GetResponseHead();
> +  if (!responseHead) {

WARN_IF

@@ +175,2 @@
>    nsAutoCString contentLocation;
>    responseHead->GetHeader(nsHttp::ResolveAtom("Content-Location"), contentLocation);

Hmm.. you should check the header is there, right?

I think we can then just ignore the resource when the header is missing and go on to the next one.  Test should be added.

@@ +193,5 @@
>    if (StringBeginsWith(contentLocation, NS_LITERAL_CSTRING("/"))) {
>      contentLocation = Substring(contentLocation, 1);
>    }
>  
>    path += contentLocation;

hmm.. one question.. how is URL encoding determined and handled here?  followup bug would be enough to investigate.

@@ +220,1 @@
>    if (!multiChannel) {

Since there is a cleanup code, I'd a little bit more prefer 

if (mupltiChannel) {
  do_stuff();
}

..
ClearCallbacks.. etc...
return;

@@ +309,5 @@
>      // Call all the callbacks for this URI
>      for (uint32_t i = 0; i < array->Length(); ++i) {
>        uint32_t result;
> +      array->ObjectAt(i)->OnCacheEntryCheck(aEntry, nullptr, &result);
> +      // TODO: do we need to check the result?

oh yes... see how CacheEntry handles OnCacheEntryCheck() call.
and when there is no entry, you must not call Check() at all.

@@ +459,5 @@
>  
> +  {
> +    nsAutoCString spec;
> +    packageURI->GetAsciiSpec(spec);
> +    key += spec;

better append ":" before the URL.

@@ +463,5 @@
> +    key += spec;
> +  }
> +
> +  nsRefPtr<PackagedAppDownloader> downloader;
> +  if (mDownloadingPackages.Get(key, getter_AddRefs(downloader))) {

hmm.. I somehow don't see a place where you are adding the downloader to this hash table.

::: netwerk/protocol/http/PackagedAppService.h
@@ +15,5 @@
>  namespace net {
>  
> +// This service is used to download packages from the web, which are saved in
> +// the cache. The service also looks up entries in the cache and returns them
> +// if they are valid.

some rephrase would be good.

@@ +20,5 @@
> +// The package format is defined at:
> +//     https://w3ctag.github.io/packaging-on-the-web/#streamable-package-format
> +// Downloading the package is triggered by calling requestURI(aURI, aCallback, aInfo)
> +//     aURI is the subresource uri - http://domain.com/path/package!//resource.html
> +//     aCallback is an object implementing nsICacheEntryOpenCallback

which is passed why? what does it get? how do we behave based on results of the callback?

@@ +23,5 @@
> +//     aURI is the subresource uri - http://domain.com/path/package!//resource.html
> +//     aCallback is an object implementing nsICacheEntryOpenCallback
> +//     aInfo is a nsILoadContextInfo used to pick the cache jar the resource goes into
> +// When requestURI is called, a CacheEntryChecker is created to verify if the
> +// entry is already in the cache. If it is, it passes it to the callback.

entry of what?

@@ +24,5 @@
> +//     aCallback is an object implementing nsICacheEntryOpenCallback
> +//     aInfo is a nsILoadContextInfo used to pick the cache jar the resource goes into
> +// When requestURI is called, a CacheEntryChecker is created to verify if the
> +// entry is already in the cache. If it is, it passes it to the callback.
> +// Otherwise, it starts downloading the package. When the entry is downloaded,

s/entry/packaged subresource (or something)/ ?

@@ +25,5 @@
> +//     aInfo is a nsILoadContextInfo used to pick the cache jar the resource goes into
> +// When requestURI is called, a CacheEntryChecker is created to verify if the
> +// entry is already in the cache. If it is, it passes it to the callback.
> +// Otherwise, it starts downloading the package. When the entry is downloaded,
> +// it gets passed to the caller.

caller?  who is the caller?  what??

@@ +43,5 @@
>    // If so, then it simply adds the callback to that PackageAppDownloader
>    // Else it begins downloading the new package and adds it to mDownloadingPackages
> +  // - aInfo is needed because cache entries are located in separate cache jars
> +  //   The caller will only be able to access a cache entry
> +  //   if it has the same nsILoadContextInfo

Describe all arguments.  

"if an entry wasn't found" - which resource the entry belong to?  etc...

@@ +55,3 @@
>    nsresult NotifyPackageDownloaded(nsCString aKey);
>  
> +  // This class is used to write data into the cache entry.

to what resource the entry belongs to?

@@ +67,2 @@
>  
> +    // Static method used to create a CacheEntryWriter.

either say what all the method really does or just omit the comment, it should be clear that static Create creates this object :)

@@ +86,5 @@
> +  // This class is used to download a packaged app. It acts as a listener
> +  // for the nsMultiMixedConv object that parses the package.
> +  // OnStartRequest and OnStopRequest will be called once for each
> +  // resource in the package, while OnDataAvailable may be called multiple times
> +  // for every resource.

"There is an OnStartRequest, OnDataAvailable*, OnStopRequest sequence called for each resource"

@@ +88,5 @@
> +  // OnStartRequest and OnStopRequest will be called once for each
> +  // resource in the package, while OnDataAvailable may be called multiple times
> +  // for every resource.
> +  // The PackagedAppService holds a hashtable of the PackagedAppDownloaders
> +  // that are in progress.

" to coalesce same loads"

@@ +100,5 @@
> +    NS_DECL_NSISTREAMLISTENER
> +    NS_DECL_NSIREQUESTOBSERVER
> +
> +    // Initializes mCacheStorage and saves aKey as mPackageKey which is later
> +    // used to remove this object from PackagedAppService::mDownloadingPackages

aKey is what?

@@ +126,5 @@
> +    // The request must be QIable to nsIResponseHeadProvider since it looks
> +    // at the Content-Location header to compute the full path.
> +    static nsresult GetSubresourceURI(nsIRequest * aRequest, nsIURI **aResult);
> +    // Used to write data into the cache entry
> +    nsRefPtr<CacheEntryWriter> mWriter;

lifetime? the more the better.

@@ +133,5 @@
> +    // A hastable containing all the consumers which requested a resource and need
> +    // to be notified once it is inserted into the cache.
> +    // The key is a subresource URI - http://example.com/package.pak!//res.html
> +    // Should only be used on the main thread.
> +    nsClassHashtable<nsCStringHashKey, nsCOMArray<nsICacheEntryOpenCallback>> mCallbacks;

> >

@@ +140,5 @@
> +    nsCString mPackageKey;
> +  };
> +
> +  // This class is used to look for an entry in the cache.
> +  // Looks for an entry in the cache. Calls aCallback->OnCacheEntryAvailable

entry.. what entry?

::: netwerk/test/unit/test_packaged_app_service_cached_entry.js
@@ +33,5 @@
>    }
>  };
>  
>  function test_first() {
> +  paservice.requestURI(createURI(uri+"/package!//index.html"), cacheListener, LoadContextInfo.default);

spaces around +

        Attachment #8576617 -
        Flags: feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Add Packaged App Service (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8587799 -
        Flags: review?(honzab.moz)

    
  

        Attachment #8576615 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        interdiff.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8576617 -
        Attachment is obsolete: true

    
    

    
  


  
It also accepts any cache entry (sets mCachedContentIsValid to true) and returns FILE_NOT_FOUND when no error is available.

    
    

    
  


  
This patch changes nsHttpChannel in order to call into the PackagedAppService for resources containing !//.
It also accepts any cache entry (sets mCachedContentIsValid to true) and returns FILE_NOT_FOUND when no error is available.

    
  

        Attachment #8553159 -
        Attachment is obsolete: true

    
  

        Attachment #8588242 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8587800 [details] [diff] [review]
interdiff.patch

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

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +145,5 @@
>  {
>    nsCOMPtr<nsIURI> uri;
>    nsresult rv = GetSubresourceURI(aRequest, getter_AddRefs(uri));
>    if (NS_WARN_IF(NS_FAILED(rv))) {
> +    mWriter = nullptr;

should you inform the writer first?  like OnStart(rv); OnStop(rv); mWriter = null; or so?

@@ +156,3 @@
>    }
>  
> +  rv = mWriter->OnStartRequest(aRequest, aContext);

check (or assert) you have the writer.

@@ +176,4 @@
>      return NS_ERROR_FAILURE;
>    }
>    nsAutoCString contentLocation;
> +  nsresult rv = responseHead->GetHeader(nsHttp::ResolveAtom("Content-Location"), contentLocation);

nit: I usually define rv as the first var in a method (breaking the "define on first use" rule).  I've seen bugs because of a redefinition of rv like this.

@@ +253,5 @@
> +    rv = multiChannel->GetIsLastPart(&lastPart);
> +    if (NS_FAILED(rv) || !lastPart) {
> +      // If this isn't the last part, we don't do the cleanup yet
> +      return NS_OK;
> +    }

hmm... won't these returns bypass the ClearCallbacks call at the end?  If it's a problem maybe have an "internal" method you call from here and move this block into it.

@@ +279,5 @@
>                                                             uint64_t aOffset,
>                                                             uint32_t aCount)
>  {
> +  if (!mWriter) {
> +    return NS_OK;

hmm.. you must read all from the input.  Use ReadSegments(NS_DiscardSegment)

@@ +503,5 @@
>  
> +  nsCOMPtr<nsICachingChannel> cacheChan(do_QueryInterface(channel));
> +  if (cacheChan) {
> +    // This is needed so we only download the entire package when it has changed.
> +    cacheChan->SetCacheOnlyMetadata(true);

This means you never cache the content just the response head.  The comment doesn't seem right then.

::: netwerk/protocol/http/PackagedAppService.h
@@ +44,5 @@
>    // It checks to see if the package is currently being downloaded.
>    // If so, then it simply adds the callback to that PackageAppDownloader
>    // Else it begins downloading the new package and adds it to mDownloadingPackages
> +  // - aURI is the packaged resource's URL
> +  // - aCallback is an object implementing nsICacheEntryOpenCallback

don't write the obvious...

@@ +49,3 @@
>    // - aInfo is needed because cache entries are located in separate cache jars
> +  //   If a resource isn't found in the package, aCallback->OnCacheEntryAvailable
> +  //   will be called with a null entry and an error result as a status.

and what about OnCacheEntryCheck? (unaddressed r comment from last time)

::: netwerk/test/unit/test_packaged_app_service.js
@@ +15,5 @@
> +// It sends a 304 Not Modified if the request has an ETag header.
> +var packagedAppRequestsMade = 0;
> +function packagedAppContentHandlerCaching(metadata, response)
> +{
> +  if (metadata.hasHeader("ETag")) {

where are you setting this header?

@@ +228,5 @@
> +        equal(read,"<html>\r\n  <head>\r\n    <script src=\"/scripts/app.js\"></script>\r\n    ...\r\n  </head>\r\n  ...\r\n</html>\r\n"); // not using do_check_eq since logger will fail for the 1/4MB string
> +    });
> +    run_next_test();
> +  }
> +};

organization of this test is pretty ad-hoc, hard to track.  can you do something about it?  also, please write a list of what this test does (maybe refer the methods).  also, why exactly have you joined the separate tests to a single one?  I liked that :)

        Attachment #8587800 -
        Flags: review-

    
    

    
  
Comment on attachment 8587799 [details] [diff] [review]
Add Packaged App Service

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

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +325,5 @@
> +    for (uint32_t i = 0; i < array->Length(); ++i) {
> +      uint32_t result;
> +      array->ObjectAt(i)->OnCacheEntryCheck(aEntry, nullptr, &result);
> +      // TODO: do we need to check the result?
> +      array->ObjectAt(i)->OnCacheEntryAvailable(aEntry, false, nullptr, aResult);

this is left unaddressed and is pretty important.

        Attachment #8587799 -
        Flags: review?(honzab.moz)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #85)
> Comment on attachment 8587800 [details] [diff] [review]
> interdiff.patch
> 
> Review of attachment 8587800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/PackagedAppService.cpp
> @@ +145,5 @@
> >  {
> >    nsCOMPtr<nsIURI> uri;
> >    nsresult rv = GetSubresourceURI(aRequest, getter_AddRefs(uri));
> >    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    mWriter = nullptr;
> 
> should you inform the writer first?  like OnStart(rv); OnStop(rv); mWriter =
> null; or so?

I don't not sure I understand what you mean here. mWriter should be null at the begining of OnStartRequest (unless an error occurred for previous resources).
We set mWriter to null as a flag that we are not writing to any cache entry.

> @@ +253,5 @@
> > +    rv = multiChannel->GetIsLastPart(&lastPart);
> > +    if (NS_FAILED(rv) || !lastPart) {
> > +      // If this isn't the last part, we don't do the cleanup yet
> > +      return NS_OK;
> > +    }
> 
> hmm... won't these returns bypass the ClearCallbacks call at the end?  If
> it's a problem maybe have an "internal" method you call from here and move
> this block into it.
> 

Good point. I moved this part outside the if block.
I also changed how we handle a null mWriter in this method, since a missing Content-Location for the last resource would return early, and requests for non-existing entries would hang instead of returning FILE_NOT_FOUND. I added a test for this case.

> +    return NS_OK;
> 
> hmm.. you must read all from the input.  Use ReadSegments(NS_DiscardSegment)

Thanks for the tip :)
 
> @@ +49,3 @@
> >    // - aInfo is needed because cache entries are located in separate cache jars
> > +  //   If a resource isn't found in the package, aCallback->OnCacheEntryAvailable
> > +  //   will be called with a null entry and an error result as a status.
> 
> and what about OnCacheEntryCheck? (unaddressed r comment from last time)

It was in another patch that I forgot to merge. I did forget to change a few of the comments though.

> 
> ::: netwerk/test/unit/test_packaged_app_service.js
> > +  if (metadata.hasHeader("ETag")) {
> 
> where are you setting this header?

I experimented with updating the package, and forgot this in the test :)

> @@ +228,5 @@
> > +        equal(read,"<html>\r\n  <head>\r\n    <script src=\"/scripts/app.js\"></script>\r\n    ...\r\n  </head>\r\n  ...\r\n</html>\r\n"); // not using do_check_eq since logger will fail for the 1/4MB string
> > +    });
> > +    run_next_test();
> > +  }
> > +};
> 
> organization of this test is pretty ad-hoc, hard to track.  can you do
> something about it?  also, please write a list of what this test does (maybe
> refer the methods).  also, why exactly have you joined the separate tests to
> a single one?  I liked that :)

It's easier to share some code this way, without a lot of boilerplate code.

I reorganized the test, wrote a bunch of comments, and managed to reuse big parts of the code (only 2 listeners and contentHandlers). If you still think it's too crowded, I can split it up into multiple files.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Add Packaged App Service (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8589880 -
        Flags: review?(honzab.moz)

    
  

        Attachment #8587799 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug1036275-interdiff.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8587800 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Valentin Gosu [:valentin] from comment #87)
> > should you inform the writer first?  like OnStart(rv); OnStop(rv); mWriter =
> > null; or so?
> 
> I don't not sure I understand what you mean here. mWriter should be null at
> the begining of OnStartRequest (unless an error occurred for previous
> resources).
> We set mWriter to null as a flag that we are not writing to any cache entry.

OK, then please add a comment.  This is confusing.

> > ::: netwerk/test/unit/test_packaged_app_service.js
> > > +  if (metadata.hasHeader("ETag")) {
> > 
> > where are you setting this header?
> 
> I experimented with updating the package, and forgot this in the test :)

This means the test is not good enough to catch your own mistakes.  You should check that the package comes from the cache, but if that would become too complicated, just leave it (and add a comment about it ;)).

> It's easier to share some code this way, without a lot of boilerplate code.
> 
> I reorganized the test, wrote a bunch of comments, and managed to reuse big
> parts of the code (only 2 listeners and contentHandlers). If you still think
> it's too crowded, I can split it up into multiple files.

Cool, thanks.  I'm more tending to have more then a single test file and freely duplicate some code, if its not super-huge.  It also runs faster - it's parallel then!

    
    

    
  
Comment on attachment 8589881 [details] [diff] [review]
bug1036275-interdiff.patch

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

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +333,5 @@
> +        array->ObjectAt(i)->OnCacheEntryCheck(aEntry, nullptr, &wanted);
> +      }
> +      if (wanted == nsICacheEntryOpenCallback::ENTRY_WANTED) {
> +        array->ObjectAt(i)->OnCacheEntryAvailable(aEntry, false, nullptr, aResult);
> +      }

Actually, you should not call this directly as I think of this more.  (your patch is anyway still wrong!).

Rather, while you are still referencing the entry (and keep it force-valid), open the entry again (using the cache storage and the URI you still have) and pass the ObjectAt(i) this time.


Nit: please move ObjectAt(i) to a local var.

        Attachment #8589881 -
        Flags: review-

    
    

    
  
Comment on attachment 8589880 [details] [diff] [review]
Add Packaged App Service

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

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +34,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  rv = writer->mEntry->OpenOutputStream(0, getter_AddRefs(writer->mOutputStream));

err... didn't realize the last time.  when you open the outputstream of an entry it becomes available to others.  at this point it's tho still empty.  this is not what you want.  you must open the stream only after you copy the headers to the entry (set the "reasponse-head" metadata) + any other metadata needed on the entry.  only then open the stream.

force valid the entry immediately after you open it.  time is up to you.

::: netwerk/test/unit/test_packaged_app_service.js
@@ +10,5 @@
> +//     - checks that calls to nsIPackagedAppService::requestURI do not accept a null argument
> +// test_callback_gets_called
> +//     - checks the regular use case -> requesting a resource should asynchronously return an entry
> +// test_cached
> +//     - makes another request for the same file, and checks that the same content is returned

and how do you check it's coming from the cache?  or it doesn't have to come from the cache? not clear.

@@ +13,5 @@
> +// test_cached
> +//     - makes another request for the same file, and checks that the same content is returned
> +// test_request_number
> +//     - checks that the package has only been requested once.
> +//       The second request should come directly from the cache

what is the difference from test_cached?

@@ +20,5 @@
> +//     - checks that requesting a file from a package that does not exist
> +//       calls the listener with an error code
> +// test_file_does_not_exist
> +//     - checks that requesting a resource that doesn't exist inside a package
> +//       calls the listener with an error code

according the comments I don't see a difference.

@@ +24,5 @@
> +//       calls the listener with an error code
> +//
> +// test_no_change_first
> +//     - resets the request counter, turns on 304 responses on the handler and
> +//       then calls nsIPackagedAppService::requestURI which downloads the package

and what has to happen?

@@ +27,5 @@
> +//     - resets the request counter, turns on 304 responses on the handler and
> +//       then calls nsIPackagedAppService::requestURI which downloads the package
> +// test_no_change_404
> +//     - requests a resource that does not exist in the package, to force
> +//       another request being made - the response will be a 304

I don't follow at all

@@ +29,5 @@
> +// test_no_change_404
> +//     - requests a resource that does not exist in the package, to force
> +//       another request being made - the response will be a 304
> +// test_no_change_cached
> +//     - requests the first resource again to make sure it is still cached.

"first resource"?

@@ +36,5 @@
> +//    - tests that a package with missing headers for some of the files
> +//      will still return files that are correct
> +// test_bad_package_404
> +//    - tests that a request for a resource that doesn't exist in the package
> +//      doesn't hang if the last file in the package is missing some headers

please check the wording

@@ +46,5 @@
> +// The number of times this package has been requested
> +// This number might be reset by tests that use it
> +var packagedAppRequestsMade = 0;
> +// If the handler should respond with a 304 response for anything but the first request
> +var respond304 = false;

if the request happens to not be conditional then I'm not sure what you get.  you don't send any cond headers...

@@ +64,5 @@
> +}
> +
> +// The package content
> +// getData formats it as described at https://w3ctag.github.io/packaging-on-the-web/#streamable-package-format
> +var testData = {

what's the head file then for?

        Attachment #8589880 -
        Flags: review?(honzab.moz)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #92)
> this is not what you want.  you must open the stream only after you copy the
> headers to the entry (set the "reasponse-head" metadata) + any other
> metadata needed on the entry.  only then open the stream.
> 
> force valid the entry immediately after you open it.  time is up to you.

After I open the entry, or the stream?
Flags: needinfo?(honzab.moz)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #92)
> Comment on attachment 8589880 [details] [diff] [review]
> Add Packaged App Service
> 
> Review of attachment 8589880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/PackagedAppService.cpp
> @@ +34,5 @@
> > +  if (NS_FAILED(rv)) {
> > +    return rv;
> > +  }
> > +
> > +  rv = writer->mEntry->OpenOutputStream(0, getter_AddRefs(writer->mOutputStream));
> 
> err... didn't realize the last time.  when you open the outputstream of an
> entry it becomes available to others.  at this point it's tho still empty. 
> this is not what you want.  you must open the stream only after you copy the
> headers to the entry (set the "reasponse-head" metadata) + any other
> metadata needed on the entry.  only then open the stream.
> 
> force valid the entry immediately after you open it.  time is up to you.

After I open the entry, or the stream?

> ::: netwerk/test/unit/test_packaged_app_service.js
> @@ +10,5 @@
> > +//     - checks that calls to nsIPackagedAppService::requestURI do not accept a null argument
> > +// test_callback_gets_called
> > +//     - checks the regular use case -> requesting a resource should asynchronously return an entry
> > +// test_cached
> > +//     - makes another request for the same file, and checks that the same content is returned
> 
> and how do you check it's coming from the cache?  or it doesn't have to come
> from the cache? not clear.

I renamed this to test_same_content.

> > +// test_request_number
> > +//     - checks that the package has only been requested once.
> > +//       The second request should come directly from the cache
> 
> what is the difference from test_cached?

This tests that only one request went over the network, meaning what test_same_content returns is from the cache.
I added comments to make this clear.

> @@ +20,5 @@
> > +//     - checks that requesting a file from a package that does not exist
> > +//       calls the listener with an error code
> > +// test_file_does_not_exist
> > +//     - checks that requesting a resource that doesn't exist inside a package
> > +//       calls the listener with an error code
> 
> according the comments I don't see a difference.
> 

First one is "file from a <package that does not exist>"
Second is " <resource that doesn't exist> inside a package"

> 
> @@ +46,5 @@
> > +// The number of times this package has been requested
> > +// This number might be reset by tests that use it
> > +var packagedAppRequestsMade = 0;
> > +// If the handler should respond with a 304 response for anything but the first request
> > +var respond304 = false;
> 
> if the request happens to not be conditional then I'm not sure what you get.
> you don't send any cond headers...

True. This part would be better handled in a followup.
I removed this from the patch, and saved it for later.

> 
> @@ +64,5 @@
> > +}
> > +
> > +// The package content
> > +// getData formats it as described at https://w3ctag.github.io/packaging-on-the-web/#streamable-package-format
> > +var testData = {
> 
> what's the head file then for?

I removed the head file I added since I put everything to one test. Or do you mean that I should move this to the existing head files?

    
    

    
  
(In reply to Valentin Gosu [:valentin] from comment #94)
> (In reply to Honza Bambas (:mayhemer) from comment #92)
> > Comment on attachment 8589880 [details] [diff] [review]
> > Add Packaged App Service
> > 
> > Review of attachment 8589880 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: netwerk/protocol/http/PackagedAppService.cpp
> > @@ +34,5 @@
> > > +  if (NS_FAILED(rv)) {
> > > +    return rv;
> > > +  }
> > > +
> > > +  rv = writer->mEntry->OpenOutputStream(0, getter_AddRefs(writer->mOutputStream));
> > 
> > err... didn't realize the last time.  when you open the outputstream of an
> > entry it becomes available to others.  at this point it's tho still empty. 
> > this is not what you want.  you must open the stream only after you copy the
> > headers to the entry (set the "reasponse-head" metadata) + any other
> > metadata needed on the entry.  only then open the stream.
> > 
> > force valid the entry immediately after you open it.  time is up to you.
> 
> After I open the entry, or the stream?

Immediately after you open the entry.

> Second is " <resource that doesn't exist> inside a package"

.. the point is to make this clear in the test enough.

> > 
> > @@ +64,5 @@
> > > +}
> > > +
> > > +// The package content
> > > +// getData formats it as described at https://w3ctag.github.io/packaging-on-the-web/#streamable-package-format
> > > +var testData = {
> > 
> > what's the head file then for?
> 
> I removed the head file I added since I put everything to one test. Or do
> you mean that I should move this to the existing head files?

what ever works for you.

sorry for the delay.
Flags: needinfo?(honzab.moz)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Add Packaged App Service (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8602815 -
        Flags: review?(honzab.moz)

    
  

        Attachment #8589880 -
        Attachment is obsolete: true

    
  

        Attachment #8588243 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8602815 [details] [diff] [review]
Add Packaged App Service

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

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +34,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  rv = writer->mEntry->ForceValidFor(120);

could it happen (network lags) that PackagedAppService::PackagedAppDownloader::OnStopRequest is called later than in 120 seconds?  I think it could.  Hence, use PR_UINT32_MAX and drop to 0 after you call all callbacks for this entry.

@@ +338,5 @@
> +        array->ObjectAt(i)->OnCacheEntryCheck(aEntry, nullptr, &wanted);
> +      }
> +      if (wanted == nsICacheEntryOpenCallback::ENTRY_WANTED) {
> +        array->ObjectAt(i)->OnCacheEntryAvailable(aEntry, false, nullptr, aResult);
> +      }

hmm... please see comment 91.

r-

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +248,5 @@
>      , mCacheEntriesToWaitFor(0)
>      , mHasQueryString(0)
>      , mConcurentCacheAccess(0)
>      , mIsPartialRequest(0)
> +    , mHasAutoRedirectVetoNotifier(0)

are you sure?

::: netwerk/test/unit/head_packaged_apps.js
@@ +7,5 @@
> +  var body = testData.getData();
> +  response.bodyOutputStream.write(body, body.length);
> +}
> +
> +var testData = {

what is this for?  I don't get this head file anymore.

        Attachment #8602815 -
        Flags: review?(honzab.moz) → review-

    
    

    
  
Comment on attachment 8602818 [details] [diff] [review]
Change nsHttpChannel to call into PackagedAppService for urls containing !//

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ -3280,5 @@
>                                       nsIApplicationCache* aAppCache,
>                                       nsresult status)
>  {
>      MOZ_ASSERT(NS_IsMainThread());
> -

leave this blank please.

@@ +3293,5 @@
> +    if (NS_SUCCEEDED(status) && mIsPackagedAppResource) {
> +        // Packaged apps don't know how to do revalidation just yet,
> +        // so we accept any cache entry
> +        mCachedContentIsValid = true;
> +    }

I don't follow why you need to set this and why here.  With LOAD_ONLY_FROM_CACHE and VALIDATE_NEVER you should get it at true here.  Maybe better then VALIDATE_NEVER flag is LOAD_FROM_CACHE (together with LOAD_ONLY_FROM_CACHE.)

@@ +4983,5 @@
>      if (!mTimingEnabled)
>          mAsyncOpenTime = TimeStamp();
>  
> +
> +    if (Preferences::GetBool("network.http.enable-packaged-apps", false)) {

have a cache.

@@ +4991,5 @@
> +    }
> +
> +    if (mIsPackagedAppResource) {
> +        mLoadFlags |= LOAD_ONLY_FROM_CACHE;
> +        mLoadFlags |= nsIRequest::VALIDATE_NEVER;

nit: no need for nsIRequest::

::: netwerk/protocol/http/nsHttpChannel.h
@@ +475,5 @@
>      uint32_t                          mIsPartialRequest : 1;
>      // true iff there is AutoRedirectVetoNotifier on the stack
>      uint32_t                          mHasAutoRedirectVetoNotifier : 1;
>  
> +    uint32_t                          mIsPackagedAppResource : 1;

comment (and a good one please)

::: netwerk/test/unit/test_packaged_app_channel.js
@@ +114,5 @@
> +  originalPref = Services.prefs.getBoolPref("network.http.enable-packaged-apps");
> +  Services.prefs.setBoolPref("network.http.enable-packaged-apps", true);
> +
> +  add_test(test_channel);
> +  add_test(reset_pref);

will reset_pref() run when test_channel() fails?

        Attachment #8602818 -
        Flags: review?(honzab.moz) → review-

    
    

    
  


  

        Attachment #8612680 -
        Flags: review?(honzab.moz)

        Attachment #8602815 -
        Attachment is obsolete: true

        Attachment #8602818 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #99)
> > +        mCachedContentIsValid = true;
> > +    }
> 
> I don't follow why you need to set this and why here.  With
> LOAD_ONLY_FROM_CACHE and VALIDATE_NEVER you should get it at true here. 
> Maybe better then VALIDATE_NEVER flag is LOAD_FROM_CACHE (together with
> LOAD_ONLY_FROM_CACHE.)
> 

Turns out that VALIDATE_NEVER and VALIDATE_ALWAYS aren't mutually exclusive, and refreshing a page sets VALIDATE_ALWAYS :) You learn something new every day. Indeed, LOAD_FROM_CACHE seems better for the job.

> > +        array->ObjectAt(i)->OnCacheEntryAvailable(aEntry, false, nullptr, aResult);
> > +      }
> 
> hmm... please see comment 91.

I finnaly got what that meant. Sorry for missing this twice.

> > +var testData = {
> 
> what is this for?  I don't get this head file anymore.

Seems hg import --no-commit didn't delete the file properly. qfold seems to do a better job.

    
    

    
  
Comment on attachment 8612680 [details] [diff] [review]
Add Packaged App Service

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

Thanks

r=honzab

        Attachment #8612680 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Comment on attachment 8612681 [details] [diff] [review]
Change nsHttpChannel to call into PackagedAppService for urls containing !//

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

r=honzab, but the check the last comments please.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4784,5 @@
>      nsresult rv;
>  
> +    if (!gPackagedAppPrefChecked) {
> +        rv = Preferences::AddBoolVarCache(&gPackagedAppEnabled,
> +                                     "network.http.enable-packaged-apps", false);

You should assert the main thread here.

nit: you could do a trick to get rid of gPackagedAppPrefChecked:

MOZ_ASSERT(NS_IsMainThread());
static nsresult rv = Preferences::Add...
if (NS_FAILED(rv)) {
  gPackagedAppEnabled = false;
}

Or - preferred, you should put the pref read to the http handler (I think we already work with prefs there)

        Attachment #8612681 -
        Flags: review?(honzab.moz) → review+

        Attachment #8612681 -
        Attachment is obsolete: true
Blocks: 1170823

        Attachment #8589881 -
        Attachment is obsolete: true

        Attachment #8524263 -
        Attachment is obsolete: true

        Attachment #8452891 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        package.pak
        — 
      

    


  
Upon landing on m-c, packaged apps support will be available behind the "network.http.enable-packaged-apps" pref.

The attached file is a simple example that may be used to test this feature.
It needs to be served from a HTTP server with the following header:
"Content-Type: application/package".

To test I used nginx, with the next configuration:
1. I added the following to /etc/nginx/sites-available/default
>  server {
>    listen 80;
>    listen [::]:80;
>  
>    server_name localhost;
>  
>    root /home/icecold/html/;
>    index index.html;
>  
>    location / {
>      try_files $uri $uri/ =404;
>    }
>  }
2. I added the following to /etc/nginx/mime.types
>  application/package                   pak;
3. Placed the attached file at /home/user/html/package.pak

You can access the package by loading
http://localhost/package.pak!//index.html

Note: 
* You might want to clear the cache before/after flipping the pref and accessing the package, as otherwise the cache entry will stay in the cache.
* At the moment the Package App Service does not update the package if it changes. I have filed bug 1170837 for that. As a workaround, you may access a subresource that does not exist, to force it to be downloaded. For example http://localhost/package.pak!//missing.html

    
    

    
  
Valentin: What happens if the package is served with the wrong mimetype? Does all request abort with an error, or do we still fulfill the request, just serving the response without a mimetype?

I *think* it should be safe to do the latter. It certainly would make adoption easier I think. Definitely ok to do as a followup though.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c2939bd290fa
https://hg.mozilla.org/mozilla-central/rev/b62f0a606418
Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox41:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #108)
> Valentin: What happens if the package is served with the wrong mimetype?
> Does all request abort with an error, or do we still fulfill the request,
> just serving the response without a mimetype?

At the moment we abort with an error. I filed bug 1172233 for that.
See Also:  → 1176213

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: