Closed Bug 1231565 Opened 8 years ago Closed 8 years ago

Allow storing alternate data (ex: JS Bytecode) in the HTTP cache

Categories

(Core :: Networking: Cache, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
platform-rel --- +
firefox52 --- fixed

People

(Reporter: jduell.mcbugs, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Keywords: DevAdvocacy, Whiteboard: DevAdvocacy-Facebook [platform-rel-Facebook])

Attachments

(5 files, 30 obsolete files)

17.22 KB, patch
Details | Diff | Splinter Review
8.00 KB, patch
Details | Diff | Splinter Review
61.75 KB, patch
Details | Diff | Splinter Review
7.62 KB, patch
michal
: review+
Details | Diff | Splinter Review
8.53 KB, patch
Details | Diff | Splinter Review
(this is different than bug 116572, which is for code that will use it's own external cache in sync with the HTTP cache).

After talking with :nbp at Orlando, we've decided the best way to support caching JS Bytecode (for regular pages) is to store the bytecode as "alternate data" in the HTTP cache.  Doing that will save doing a round-trip in e10s, and also additional I/O, compared with having the JS engine keep its own separate cache.

The plan here is to 

1) change the lower-level cache (Michal) to support writing "additional data" in cache files, after the main/regular data, but before the metadata.  I suggest we allow multiple blocks if it's not too hard to do, ideally with some sort of id that the client can provide (ex: "js-bytecode").   (If that's too hard to do, we can probably live with only allowing one alternative data chunk per URI?)

2) Add some APIs to the cache (and/or HTTP channel);

- Something the client calls before AsyncOpen that lets the cache know that it should return the alt data instead of the regular data if it's available (with the id--"js-bytecode" if we support that).

- Some way (probably a custom HTTP header we add? x-moz-cache-altcontenttype: js-bytecode) that lets the client know in OnStart whether they're getting alt data or original data.  We'll also want to pass information about how long the alt-data is (the JS engine will want to malloc a single big buffer with that size).

- Some function that the client can call on the channel to let it know that it may want to store alt-data on the channel (we may want that so we know to keep the cache entry alive and/or the httpChannelParent, etc).  Also an API that lets them pass in a stream (or other mechanism) for asynchronously writing the alt data into the cache.   We need to be aware of possible races and/or buffer overflows here (ex: we haven't finished writing the regular data into the cache, but the JS engine starts already passing us alt data to write too).

I'm sure I'm forgetting more details here, but that's what I remember :)
Nicolas: got a bug # that this should block?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #1)
> Nicolas: got a bug # that this should block?

Yes, there is Bug 900784 for that.
Flags: needinfo?(nicolas.b.pierron)
Whiteboard: [necko-backlog]
Depends on: 1258747
The full plan, v1 (assigning this bug to Valentin):

Writing:

- add nsIOutputStream nsICacheInfoChannel.openAlternativeOutputStream(in string type)

|type| is an arbitrary string that identifies (names) the data type (aka alt data mime type).  for the first implementation we store only one alt stream, still we should be able to say what the data stored actually is.  it can also be used for versioning of sort.  in the future we can easily add more alt streams with different types with this api.

returns an output stream (actually result of nsICacheEntry.openOutputStream(alt_data_offset), where alt_data_offset is known to the cache entry after the real data has been written) that can be directly written to.  this stream is buffered and blocking (tho, never blocks, of course).

only problem I can see is that this has to work on the child process as well.  the stream we return must actually be an IPC forwarding stream.  the actual cache entry is only present on the parent process.

we will also need to prolong life of the parent channel and also life time of the cache entry kept on the parent (by the nsHttpChannel on the parent) ; simply said: while there is a child channel, let the parent channel live as well...  might become pretty suboptimal, but we can have another api added telling the channel there will probably be alt data to write (that in or after onstop we will call openAlternativeOutputStream), so we can release channels that won't consume alt data early, as we do today

the cache entry's implementation of openOutputStream ensures there is just one output stream open at a time.  this very simply ensures we cannot write alt data before real data are written down.

nsICacheInfoChannel.openAlternativeOutputStream should fail when there already is alt data present on the cache entry.



Reading:

- add void nsICacheInfoChannel.preferAlternativeDataType(in string type)

must be called before asyncOpen() and carried to any redirect-to channels

|type| is expected to be the same as what has been passed to openAlternativeOutputStream().  this method will instruct the channel to read the alt stream - when present - instead of the real data from the cache entry, simply passing the alt-data offset to OpenInputStream will do (happens in nsHttpChannel::OpenCacheInputStream)


- add (attribute?) string nsICacheInfoChannel.alternativeDataType

must be called only in (or past) OnStartRequest (qi the |request| to nsICacheInfoChannel), otherwise should probably throw (assuming the alt type is not yet known)

it returns the type of the alternative data the channel is giving the consumer right now or null when the real data is delivered.  I kinda tend to return a string, since one day preferAlternativeDataType may take a list of types


Information about presence, type and offset of alt data will be held in metadata.  Let's have say "alt-data" key with a following value form:  "1;12345,javascript/binary"

- 1 is the version of the alt-data metadata form
- 12345 is the offset (int64)
- javascript/binary is the |type|, probably restrict the delimiter be contained (whatever char we decide to be it)


How this metadata is going to be handled exactly is up to you, you will have to sync with Michal, as he is going to need the info for some decisions in bug 1258747.

I will write an xpcshell test for the basic simple functionality.  that should then be cloned to tests of all corner cases, mainly those outlined in bug 1258747.
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Includes also implementation of nsICacheTesting.flush(observer), actually quit simple.

I've never ran any of this ;)
Currently we CloseCacheEntry() in a number of cases, meaning that after when we call openAlternativeOutputStream after OnStopRequest, we no longer have the entry around to open the output stream right away.
Should we change the behaviour so we keep the entry around after OnStopRequest, or change the API so openAlternativeOutputStream is async? This might be the better choice, as it would make it easier to open the stream on the parent and "pass it" to the child.
Flags: needinfo?(honzab.moz)
Also few questions concerning the cache implementation:
* Who sets the alt-data metadata on the cache entry? Can I use CacheFileChunkListener to set it when we close the stream? Or is there a better way?
* Will we have another method for opening the alt-data input stream? Or do we call openInputStream from nsHttpChannel with the offset we get from the metadata?
(In reply to Valentin Gosu [:valentin] from comment #7)
> Currently we CloseCacheEntry() in a number of cases, meaning that after when
> we call openAlternativeOutputStream after OnStopRequest, we no longer have
> the entry around to open the output stream right away.
> Should we change the behaviour so we keep the entry around after
> OnStopRequest, or change the API so openAlternativeOutputStream is async?
> This might be the better choice, as it would make it easier to open the
> stream on the parent and "pass it" to the child.

Don't go async.  You may not end up with the same entry when opening it again.  Unless we find a serious showstopper, let's keep the entry around in the channel.  I think channels are going away soon enough to free the memory occupied by the entry.

(In reply to Valentin Gosu [:valentin] from comment #8)
> Also few questions concerning the cache implementation:
> * Who sets the alt-data metadata on the cache entry? Can I use
> CacheFileChunkListener to set it when we close the stream? Or is there a
> better way?

You set it when the alt-data output stream closes with success.  No need for sync here.

> * Will we have another method for opening the alt-data input stream? Or do
> we call openInputStream from nsHttpChannel with the offset we get from the
> metadata?

It's up to you.  For the starter we can go with just the offset, but since the metadata is something known to the entry, we could move that logic there (to CacheEntry) and have a new method(s) for handling the alt-data.  We will have to work with the alt-data metadata state anyway.  

Maybe also bring Michal to this discussion, you will anyway probably coordinate on the WOULD_BLOCK/EOF bug 1258747 with him.

Feel free to whip up some mockup patches and submit them here for early feedback.
Flags: needinfo?(honzab.moz)
Just learned about a new requirement (should be easy to add):

- when there already is alt-data (completely written, no output stream open) but someone calls openAlternativeOutputStream again on that entry, we have to truncate the existing data ; that means:
  * truncate the file
  * delete the "alt-data" metadata
  * and then write as usually
Attached patch bug1231565-alt-data-part1.patch (obsolete) — Splinter Review
Basic outline of the patch.
I still need to implement the outputStream wrapper for the child, passing the preferred data type to redirected channels, and some of the logic in cacheEntry.
Attachment #8734384 - Flags: review?(honzab.moz)
Attachment #8734384 - Flags: review?(honzab.moz) → feedback?(honzab.moz)
Comment on attachment 8734384 [details] [diff] [review]
bug1231565-alt-data-part1.patch

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

looks good.  sorry, could give feedback sooner but the Thursday meeting interrupted me.

::: netwerk/base/nsICacheInfoChannel.idl
@@ +56,5 @@
>    attribute boolean allowStaleCacheContent;
> +
> +  /**
> +   * Calling this method instructs the channel to serve the alternative data
> +   * representation that was previously saved in the cache.

not true.  it tells the channel to load it when available, but if not, load the real data.

@param type comment needs to be added

@@ +66,5 @@
> +   * Holds the type of the alternative data representation that the channel
> +   * is returning.
> +   * Is empty string if no alternative data representation was requested, or
> +   * if the requested representation wasn't found in the cache.
> +   * Can only be called during or after OnStartRequest.

is this really true?  isn't it accessible at on-modify-request too?  (not sure)

@@ +73,5 @@
> +
> +  /**
> +   * Opens and returns an output stream that a consumer may use to save an
> +   * alternate representation of the data.
> +   * Must be called after OnStopRequest.

// Must be called after OnStopRequest that delivered the real data.

how does this behave when you are loading alt-data already?

::: netwerk/cache2/CacheEntry.cpp
@@ +1243,5 @@
> +  int64_t altDataOffset = -1;
> +  if (NS_SUCCEEDED(rv)) {
> +    // The format is: "1;12345,javascript/binary"
> +    //         <version>;<offset>,<type>
> +    mozilla::Tokenizer p(altDataMetadata, nullptr, "/");

you may need to add more, like -_+ etc...  or rather search for a delimiter to find an end

(I could imagine a ReadUntil(Token const& t, nsDependentCSubstring& result) method added on Tokenizer - a different bug, please)

@@ +1247,5 @@
> +    mozilla::Tokenizer p(altDataMetadata, nullptr, "/");
> +    uint32_t metatadaVersion;
> +    int64_t offset = -1;
> +    nsAutoCString availableAltData;
> +    if (p.ReadInteger(&metatadaVersion) && p.CheckChar(';') &&

ok, but you also have to work with the version somehow

@@ +1257,5 @@
> +      if (availableAltData == type) {
> +        altDataOffset = offset;
> +      }
> +    }
> +  }

nit: this is a single purpose method, in those I personally prefer doing

if (failed) {
  return error;
}

form of branching. it's easier to read and mainthain

::: netwerk/cache2/nsICacheEntry.idl
@@ +209,5 @@
> +  /**
> +   * Opens and returns an input stream that can be used to read the alternative
> +   * representation previously saved in the cache.
> +   * @throws
> +   *    - NS_ERROR_NOT_AVAILABLE if the alt-data representation doesn't exist.

// if the alt-data representation doesn't exist at all or alt-data of the given type doesn't exist.

@@ +210,5 @@
> +   * Opens and returns an input stream that can be used to read the alternative
> +   * representation previously saved in the cache.
> +   * @throws
> +   *    - NS_ERROR_NOT_AVAILABLE if the alt-data representation doesn't exist.
> +   *    - NS_ERROR_IN_PROGRESS when the write is still in progress.

no, when write (of alt-data) is in progress, you can stream it to this consumer.  the cache backend is fully capable of it.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +482,5 @@
>  
> +    // Holds the name of the alternative data type the channel returned.
> +    nsCString mPreferredAltDataType;
> +    // Holds the name of the alternative data type the channel returned.
> +    nsCString mAltDataType;

needs some comments cleanup
also, these both could be moved to the base channel?
Attachment #8734384 - Flags: feedback?(honzab.moz) → feedback+
(In reply to Honza Bambas (:mayhemer) from comment #12)
> > +   * Can only be called during or after OnStartRequest.
> 
> is this really true?  isn't it accessible at on-modify-request too?  (not
> sure)

Not enforced yet, but this attribute will not be available in the child until OnStartRequest.

> // Must be called after OnStopRequest that delivered the real data.
> 
> how does this behave when you are loading alt-data already?

For the channel, you should be able to request the alt-data, then overwrite with another alt-data afterwards if you want to.
However, it might be a problem at the cache entry level, if there's another channel reading while we're trying to write.
I don't know how we can handle that one yet.

> ::: netwerk/protocol/http/nsHttpChannel.h
> @@ +482,5 @@
> >  
> > +    // Holds the name of the alternative data type the channel returned.
> > +    nsCString mPreferredAltDataType;
> > +    // Holds the name of the alternative data type the channel returned.
> > +    nsCString mAltDataType;
> 
> needs some comments cleanup
> also, these both could be moved to the base channel?

We could. Should I try to make HttpBaseChannel extend nsICacheInfoChannel?
(In reply to Valentin Gosu [:valentin] from comment #13)
> (In reply to Honza Bambas (:mayhemer) from comment #12)
> > > +   * Can only be called during or after OnStartRequest.
> > 
> > is this really true?  isn't it accessible at on-modify-request too?  (not
> > sure)
> 
> Not enforced yet, but this attribute will not be available in the child
> until OnStartRequest.

OK, just wanted to know if there was some certainty when this was available.

> 
> > // Must be called after OnStopRequest that delivered the real data.
> > 
> > how does this behave when you are loading alt-data already?
> 
> For the channel, you should be able to request the alt-data, then overwrite
> with another alt-data afterwards if you want to.
> However, it might be a problem at the cache entry level, if there's another
> channel reading while we're trying to write.
> I don't know how we can handle that one yet.

OK.  Good point.  With normal content entries (w/o this patch), when there is a consumer still reading and another one that wants to rewrite at the same time, the new writer must use a new entry.  The existing one is doomed, which makes any potential writer to it to finish its job and all readers finish reading w/o errors.  The entry is then deleted on the last reference release.

Here, the situation is different.  We could for now go with one of the options:

1) (preferred)
- don't allow writing alt-data when alt-data are just being read (an input stream _that has already been read_ exists)
- we are just a cache, the concurrent cache will be rare and no readers will break with this

2) (not good IMO)
- kill readers and allow writing a new content
- this will cause random (very hard to detect and diagnose) breakages of cached alt-data reading
- will give some small (rarely useful) performance gain


So, I believe (1) is OK to go, still, we are just a cache which goal is to consistently deliver a cached content or rather recoverably fail very early.

> 
> > ::: netwerk/protocol/http/nsHttpChannel.h
> > @@ +482,5 @@
> > >  
> > > +    // Holds the name of the alternative data type the channel returned.
> > > +    nsCString mPreferredAltDataType;
> > > +    // Holds the name of the alternative data type the channel returned.
> > > +    nsCString mAltDataType;
> > 
> > needs some comments cleanup
> > also, these both could be moved to the base channel?
> 
> We could. Should I try to make HttpBaseChannel extend nsICacheInfoChannel?

No, just carry down the members, don't change inheritance.
I renamed this member, because I want to use mOnStartRequestCalled in a slightly different way
Attachment #8737386 - Flags: review?(honzab.moz)
* Add PAltDataOutputStream.ipdl to be able to open an OutputStream to the cache entry in the child process
* Rename HttpBaseChannel::mOnStartRequestCalled to mOnStartRequestListenerCalled to better illustrate its usage
* Add HttpBaseChannel::mOnStartRequestCalled to be able to enforce method called during or after OnStartRequest()


MozReview-Commit-ID: IfIEdZKT1tk
Attachment #8737388 - Flags: feedback?(jduell.mcbugs)
Attachment #8737388 - Flags: feedback?(honzab.moz)
Attachment #8734384 - Attachment is obsolete: true
(In reply to Valentin Gosu [:valentin] from comment #16)
> Created attachment 8737388 [details] [diff] [review]
> (Part 2) Allow storing alternate data in the HTTP cache

Jason, if you could look at the IPDL and AltDataOutputStream* implementations I would really appreciate it. I tried to do more or less the same things as HttpChannelChild.

One of the problems I found though is that at the end of HttpChannelChild::OnStopRequest we call to PHttpChannelChild::Send__delete__ which tears down the IPDL connection. We probably want to change that in order for this API to work.
Comment on attachment 8737388 [details] [diff] [review]
(Part 2) Allow storing alternate data in the HTTP cache

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

::: netwerk/base/nsICacheInfoChannel.idl
@@ +74,5 @@
> +
> +  /**
> +   * Opens and returns an output stream that a consumer may use to save an
> +   * alternate representation of the data.
> +   * Must be called after the OnStopRequest that delivered the real data.

more rich info about this method behavior has to be provided.  we need to discuss how this behaves/affects consumers currently reading any existing alt-data, how this behaves when write is already in progress etc.

feel free to make TODO or any conceptual notes here, this is WIP anyway.

::: netwerk/cache2/CacheEntry.cpp
@@ +1235,5 @@
> +}
> +
> +NS_IMETHODIMP CacheEntry::OpenAlternativeInputStream(const nsACString & type, nsIInputStream * *_retval)
> +{
> +  // TODO: throw error if the output stream hasn't been closed.

you mean the real-data output stream?

@@ +1253,5 @@
> +  int64_t altDataOffset = -1;
> +  if (!p.ReadInteger(&metadataVersion) ||
> +      !p.CheckChar(';') ||
> +      !p.ReadInteger(&altDataOffset) ||
> +      !p.CheckChar(',')) {

I'd slightly more prefer to first check the version and only then continue reading.  it will be easier to debug why this method happen to fail.

@@ +1258,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  // The requested alt-data representation is not available
> +  // or the metadata has the wrong version.

has a wrong version

::: netwerk/cache2/nsICacheEntry.idl
@@ +197,5 @@
>  
> +  /**
> +   * Is used to designate the version of "alt-data" metadata format.
> +   */
> +  const unsigned long ALT_DATA_META_VERSION = 1;

why exactly in IDL?  this is something pretty internal IMO

::: netwerk/ipc/NeckoParent.cpp
@@ +274,5 @@
> +{
> +  HttpChannelParent* chan = static_cast<HttpChannelParent*>(channel);
> +  nsCOMPtr<nsIOutputStream> stream;
> +  nsresult rv = chan->OpenAlternativeOutputStream(type, getter_AddRefs(stream));
> +  AltDataOutputStreamParent* parent = new AltDataOutputStreamParent(stream);

don't we have some PBackgroud for this?  I really think you don't need to reimplement this again...

::: netwerk/protocol/http/AltDataOutputStreamChild.cpp
@@ +63,5 @@
> +
> +NS_IMETHODIMP
> +AltDataOutputStreamChild::Close()
> +{
> +  if (!mIPCOpen || mError != NS_OK) {

you should return mError when it's a failure

@@ +100,5 @@
> +  }
> +  nsAutoCString buffer;
> +  buffer.SetCapacity(aCount);
> +  aFromStream->Read(buffer.BeginWriting(), aCount, _retval);
> +  Unused << SendWriteData(buffer);

you must set correct length on the string!  _retval may be < aCount

@@ +112,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +  nsAutoCString buffer;
> +  buffer.SetCapacity(aCount);
> +  nsresult rv = aReader(this, aClosure, buffer.BeginWriting(), 0, aCount, _retval);

please see http://hg.mozilla.org/mozilla-central/annotate/9bd900888753/netwerk/base/nsBufferedStreams.cpp#l691 how to write this correctly.

@@ +123,5 @@
> +
> +NS_IMETHODIMP
> +AltDataOutputStreamChild::IsNonBlocking(bool *_retval)
> +{
> +  *_retval = true;

really is?

::: netwerk/protocol/http/AltDataOutputStreamParent.cpp
@@ +55,5 @@
> +AltDataOutputStreamParent::RecvFlush()
> +{
> +  nsresult rv;
> +  if (mOutputStream) {
> +    rv = mOutputStream->Flush();

unimplemented (no-op)

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +494,5 @@
>    // This parameter is used to ensure that we do not call the OnStartRequest
>    // listener more than once.
>    bool mOnStartRequestListenerCalled;
> +  // This is used to ensure methods can only be called during or after OnStartRequest
> +  bool mOnStartRequestCalled;

Aha, so mOnStartRequestListenerCalled is set after we return from mListener->OnStartRequest() but mOnStartRequestCalled is set before it?

I don't think it's a good naming at all then.  More like mAfterOnStartRequest and mBeforeOnStartRequest ?  Not sure..  but your names are confusing.

@@ +514,5 @@
>  
> +  // Holds the name of the preferred alt-data type.
> +  nsCString mPreferredAltDataType;
> +  // Holds the name of the alternative data type the channel returned.
> +  nsCString mAltDataType;

I think you need to include "Cached" in both these names.  And mAltDataType is also a bit confusing, can you find a more illustrating name?

or, originally you used only a single mAltDataType member to store preference and result of alt-data type you needed in ChildChannel (the previous patch version).  Maybe we could do it here as well?  Also because http channel is a heavily used class, so that we can save few bits of memory and maybe also confusion.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5855,5 @@
>      MOZ_ASSERT(!(mTransactionPump && mCachePump) || mCachedContentIsPartial,
>                 "If we have both pumps, the cache content must be partial");
>  
> +    MOZ_ASSERT(!mOnStartRequestCalled, "OnStartRequest should be called only once");
> +    mOnStartRequestCalled = true;

so, I need some more data on how this is eventually used.
Attachment #8737388 - Flags: feedback?(honzab.moz) → feedback+
Comment on attachment 8737386 [details] [diff] [review]
(Part 1) Rename mOnStartRequestCalled to mOnStartRequestListenerCalled to better illustrate its use

I raised some question around this flag, dropping r? until cleared.
Attachment #8737386 - Flags: review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #19)
> Comment on attachment 8737386 [details] [diff] [review]
> (Part 1) Rename mOnStartRequestCalled to mOnStartRequestListenerCalled to
> better illustrate its use
> 
> I raised some question around this flag, dropping r? until cleared.

Note also bug 1261632.
Whiteboard: [necko-backlog] → [necko-active]
* Add PAltDataOutputStream.ipdl to be able to open an OutputStream to the cache entry in the child process
* Rename HttpBaseChannel::mOnStartRequestCalled to mOnStartRequestListenerCalled to better illustrate its usage
* Add HttpBaseChannel::mOnStartRequestCalled to be able to enforce method called during or after OnStartRequest()


MozReview-Commit-ID: IfIEdZKT1tk
Attachment #8737388 - Attachment is obsolete: true
Attachment #8737388 - Flags: feedback?(jduell.mcbugs)
(In reply to Honza Bambas (:mayhemer) from comment #18)
> ::: netwerk/cache2/CacheEntry.cpp
> @@ +1235,5 @@
> > +}
> > +
> > +NS_IMETHODIMP CacheEntry::OpenAlternativeInputStream(const nsACString & type, nsIInputStream * *_retval)
> > +{
> > +  // TODO: throw error if the output stream hasn't been closed.
> 
> you mean the real-data output stream?

Actually it's not needed. The cache entry wouldn't have an "alt-data" metadata element if the real or alt-data wasn't properly saved.

> > +  const unsigned long ALT_DATA_META_VERSION = 1;
> 
> why exactly in IDL?  this is something pretty internal IMO

Moved it to CacheEntry.cpp

> ::: netwerk/ipc/NeckoParent.cpp
> @@ +274,5 @@
> > +  AltDataOutputStreamParent* parent = new AltDataOutputStreamParent(stream);
> 
> don't we have some PBackgroud for this?  I really think you don't need to
> reimplement this again...

I didn't find anything I could use in PBackground, but maybe I'm looking at the wrong thing?
Jason asked if we could just get a filedescriptor in the child that we could write to? I'm not sure if that would work, since the metadata is at the end of the cacheFile - or could we make this work?

> > +NS_IMETHODIMP
> > +AltDataOutputStreamChild::IsNonBlocking(bool *_retval)
> > +{
> > +  *_retval = true;
> 
> really is?

Well, the IPC message sending is async, and shouldn't block.
If I missed something let me know.

> > +    rv = mOutputStream->Flush();
> 
> unimplemented (no-op)

Removed.

> > +  // Holds the name of the preferred alt-data type.
> > +  nsCString mPreferredAltDataType;
> > +  // Holds the name of the alternative data type the channel returned.
> > +  nsCString mAltDataType;
> 
> I think you need to include "Cached" in both these names.  And mAltDataType
> is also a bit confusing, can you find a more illustrating name?

Renamed to mPreferredCachedAltDataType and mAvailableCachedAltDataType.

> 
> or, originally you used only a single mAltDataType member to store
> preference and result of alt-data type you needed in ChildChannel (the
> previous patch version).  Maybe we could do it here as well?  Also because
> http channel is a heavily used class, so that we can save few bits of memory
> and maybe also confusion.

I think both are needed to handle redirects. If I only had one, the preferred type would be cleared when opening the cache entry, and the new channel wouldn't get it.
(In reply to Valentin Gosu [:valentin] from comment #22)
> (In reply to Honza Bambas (:mayhemer) from comment #18)
> > don't we have some PBackgroud for this?  I really think you don't need to
> > reimplement this again...
> 
> I didn't find anything I could use in PBackground, but maybe I'm looking at
> the wrong thing?

Ah, PBackground is something else, sorry :)  I was somehow persuaded we had already a generic way to send data among processes.  Anyway, you've already wrote this, let's go, we can exchange at any time later.

> Jason asked if we could just get a filedescriptor in the child 

What?

> that we could
> write to? I'm not sure if that would work, since the metadata is at the end
> of the cacheFile - or could we make this work?

Nonsense.  Cache fully works only on the parent, we will never do IO even only when via a passed-down descriptor.  What we need is either a shared memory among processes (my big wish!) or some kind of a custom pipe (if already talking about file descs) that will be more efficient than our IPC engine.  Which I don't think it ever will.

Just go with what you have then.

> 
> > > +NS_IMETHODIMP
> > > +AltDataOutputStreamChild::IsNonBlocking(bool *_retval)
> > > +{
> > > +  *_retval = true;
> > 
> > really is?
> 
> Well, the IPC message sending is async, and shouldn't block.
> If I missed something let me know.

non-blocking means that the stream can at any time return NS_ERROR_WOULD_BLOCK and then needs to be AsyncWait'ed for to work further.  So, if you have a buffered stream that always consumes the data, which this apparently is - right?, this has to return false.  Because it's actually a blocking stream.

> 
> > > +    rv = mOutputStream->Flush();
> > 
> > unimplemented (no-op)
> 
> Removed.
> 
> > > +  // Holds the name of the preferred alt-data type.
> > > +  nsCString mPreferredAltDataType;
> > > +  // Holds the name of the alternative data type the channel returned.
> > > +  nsCString mAltDataType;
> > 
> > I think you need to include "Cached" in both these names.  And mAltDataType
> > is also a bit confusing, can you find a more illustrating name?
> 
> Renamed to mPreferredCachedAltDataType and mAvailableCachedAltDataType.

Like it!

> 
> > 
> > or, originally you used only a single mAltDataType member to store
> > preference and result of alt-data type you needed in ChildChannel (the
> > previous patch version).  Maybe we could do it here as well?  Also because
> > http channel is a heavily used class, so that we can save few bits of memory
> > and maybe also confusion.
> 
> I think both are needed to handle redirects. If I only had one, the
> preferred type would be cleared when opening the cache entry, and the new
> channel wouldn't get it.

OK.

Thanks!
Keywords: DevAdvocacy
Whiteboard: [necko-active] → [necko-active], DevAdvocacy-Facebook
Attachment #8737386 - Attachment is obsolete: true
Attached patch Basic test, no r (obsolete) — Splinter Review
MozReview-Commit-ID: KZejqRda2BO
Attachment #8733481 - Attachment is obsolete: true
Comment on attachment 8753536 [details] [diff] [review]
Basic test, no r

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

::: netwerk/test/unit/test_alt-data_simple.js
@@ +89,5 @@
> +
> +  // we may need some gc() calls first here...
> +  // check one of test_cache2-26-no-outputstream-open.js or test_cache2-30c-pinning-deferred-doom.js
> +  // but the entry is inside the channel, so probably no need to...
> +  do_execute_soon(() => Services.cache2.QueryInterface(Ci.nsICacheTesting).flush(cacheFlushObserver));

Honza, I was trying to get the test to actually pass, by stubbing the altInput/OutputStream to use the regular openInput/OutputStream on CacheEntry.
However, a weird thing about this test is that if I call flush, then the second request doesn't send the "If-None-Match" header anymore.
Any idea on how to fix that?
Attachment #8753536 - Flags: feedback?(honzab.moz)
Attachment #8739829 - Attachment is obsolete: true
(In reply to Valentin Gosu [:valentin] from comment #25)
> Comment on attachment 8753536 [details] [diff] [review]
> Basic test, no r
> 
> Review of attachment 8753536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/test/unit/test_alt-data_simple.js
> @@ +89,5 @@
> > +
> > +  // we may need some gc() calls first here...
> > +  // check one of test_cache2-26-no-outputstream-open.js or test_cache2-30c-pinning-deferred-doom.js
> > +  // but the entry is inside the channel, so probably no need to...
> > +  do_execute_soon(() => Services.cache2.QueryInterface(Ci.nsICacheTesting).flush(cacheFlushObserver));
> 
> Honza, I was trying to get the test to actually pass, by stubbing the
> altInput/OutputStream to use the regular openInput/OutputStream on
> CacheEntry.
> However, a weird thing about this test is that if I call flush, then the
> second request doesn't send the "If-None-Match" header anymore.
> Any idea on how to fix that?

Can you give me a NSPR log?  cache2+nsHttp.  As you know, I've never ran this patch, so it might be pretty wrong.  I also think we need to push metadata writing, which happens only once in 5 seconds.  When pending, the entries actually are not purged.

Tho, still don't follow why the entry is not reused, log will show.
Attachment #8753536 - Flags: feedback?(honzab.moz)
* Add PAltDataOutputStream.ipdl to be able to open an OutputStream to the cache entry in the child process

Review commit: https://reviewboard.mozilla.org/r/54080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54080/
Attached file log.txt (obsolete) —
Attaching NSPR logs. comment 25, comment 27

[mental note: mozReview is a bad place to post unfinished patches. Sorry for the extra bug mail.
Attachment #8754601 - Attachment is obsolete: true
Attachment #8754602 - Attachment is obsolete: true
Attachment #8754603 - Attachment is obsolete: true
Attachment #8754604 - Attachment is obsolete: true
Attachment #8754605 - Attachment is obsolete: true
Hmm... try adding do_get_profile() to the test ;)
Flags: platform-rel?
platform-rel: --- → ?
Whiteboard: [necko-active], DevAdvocacy-Facebook → [necko-active], DevAdvocacy-Facebook [platform-rel-Facebook]
What is the status of this bug, can I already use the current set of patches to prototype the bytecode cache?
Flags: needinfo?(valentin.gosu)
(In reply to Nicolas B. Pierron [:nbp] from comment #37)
> What is the status of this bug, can I already use the current set of patches
> to prototype the bytecode cache?

I think the API is mostly stable, just the implementation is young.  I personally would give you a green light to start prototyping.
Flags: needinfo?(valentin.gosu)
This passes the simple use case in both e10s and non-e10s. There are still a few bugs - such as error handling when allocating the AltDataOutputStreamParent.
But this seems ok to experiment with.
Attachment #8753540 - Attachment is obsolete: true
MozReview-Commit-ID: 7uBCMfJ7kT5
* Add PAltDataOutputStream.ipdl to be able to open an OutputStream to the cache entry in the child process

MozReview-Commit-ID: GeeUQY3LtJj
Attachment #8768744 - Attachment is obsolete: true
MozReview-Commit-ID: 7uBCMfJ7kT5
Attachment #8768745 - Attachment is obsolete: true
Comment on attachment 8753536 [details] [diff] [review]
Basic test, no r

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

::: netwerk/test/unit/test_alt-data_simple.js
@@ +73,5 @@
> +  do_check_eq(buffer, responseContent);
> +  do_check_eq(cc.alternativeDataType, "");
> +
> +  var os = cc.openAlternativeOutputStream(altContentType);
> +  os.write(altContent, altContent.length);

Question, can we add another test case which would check if this still works if the os.write is made later in a setTimeout?

I guess this should not be an issue, but I think this would resemble a bit more to the actual use case of the JavaScript bytecode cache.
* Add PAltDataOutputStream.ipdl to be able to open an OutputStream to the cache entry in the child process

MozReview-Commit-ID: GeeUQY3LtJj
Attachment #8774984 - Flags: feedback?(honzab.moz)
Attachment #8772503 - Attachment is obsolete: true
MozReview-Commit-ID: P6SoZiBykr
Attachment #8774986 - Flags: feedback?(honzab.moz)
Using do_execute_soon to open the altOutputStream after OnStopRequest as required in comment 43
Attachment #8772504 - Attachment is obsolete: true
platform-rel: ? → +
Comment on attachment 8774984 [details] [diff] [review]
(Part 1) Allow storing alternate data in the HTTP cache

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

all the changes look reasonable.  please even if you are asking just for feedback, add comments (at least some).

(I overlooked this was an f?, so this is more a full review)

::: netwerk/cache2/CacheEntry.cpp
@@ +1245,5 @@
> +  RefPtr<CacheOutputCloseListener> listener;
> +  if (mCloseListener) {
> +    listener = mCloseListener;
> +  } else {
> +    listener = new CacheOutputCloseListener(this);

is this a leftover?

::: netwerk/cache2/CacheEntry.h
@@ +281,5 @@
>    ::mozilla::ThreadSafeAutoRefCnt mHandlesCount;
>  
>    nsTArray<Callback> mCallbacks;
>    nsCOMPtr<nsICacheEntryDoomCallback> mDoomCallback;
> +  RefPtr<CacheOutputCloseListener> mCloseListener;

no use for this (next time comment on it)

@@ -406,5 @@
>  
>    NS_DECL_NSIRUNNABLE
>    explicit CacheOutputCloseListener(CacheEntry* aEntry);
>  
> -private:

why these changes?  I don't see you'd be deriving from this class

::: netwerk/ipc/NeckoParent.cpp
@@ +257,5 @@
> +  nsCOMPtr<nsIOutputStream> stream;
> +  nsresult rv = chan->OpenAlternativeOutputStream(type, getter_AddRefs(stream));
> +  AltDataOutputStreamParent* parent = new AltDataOutputStreamParent(stream);
> +  parent->AddRef();
> +  parent->SetError(rv);

add a comment that the error here will be sent asynchronously back only after a 'write' event is handled on the parent.  wish there were a way to send it sooner or in more sync way, so that child doesn't waste bytes on wire

::: netwerk/protocol/http/AltDataOutputStreamChild.cpp
@@ +9,5 @@
> +
> +NS_IMETHODIMP_(MozExternalRefCountType) AltDataOutputStreamChild::Release()
> +{
> +  NS_PRECONDITION(0 != mRefCnt, "dup release");
> +  NS_ASSERT_OWNINGTHREAD(AltDataOutputStreamChild);

so, is this MT or ST?

@@ +37,5 @@
> +AltDataOutputStreamChild::AltDataOutputStreamChild()
> +  : mIPCOpen(false)
> +  , mError(NS_OK)
> +{
> +  MOZ_COUNT_CTOR(AltDataOutputStreamChild);

see https://dxr.mozilla.org/mozilla-central/rev/ddeb0295df692695b36295177d6790e5393e1f9a/xpcom/glue/nsISupportsImpl.h#143

this way if you leak one object, the stats will say you leak two

@@ +97,5 @@
> +  }
> +  if (NS_FAILED(mError)) {
> +    return mError;
> +  }
> +  Unused << SendWriteData(nsCString(aBuf, aCount));

we had bugs that sending too much data in one IPC message may OOM (or close us to OOM), could you please send in loops by some 256kB?  in all the methods.

@@ +113,5 @@
> +    return mError;
> +  }
> +  nsAutoCString buffer;
> +  buffer.SetCapacity(aCount);
> +  aFromStream->Read(buffer.BeginWriting(), aCount, _retval);

any error checks?

@@ +114,5 @@
> +  }
> +  nsAutoCString buffer;
> +  buffer.SetCapacity(aCount);
> +  aFromStream->Read(buffer.BeginWriting(), aCount, _retval);
> +  buffer.SetCapacity(*_retval);

not sure, but you may also need to do SetLength?  rather check.

@@ +140,5 @@
> +    uint32_t read = 0;
> +    rv = aReader(this, aClosure, buffer.BeginWriting() + cursor, *_retval, left, &read);
> +
> +    if (NS_FAILED(rv)) {
> +      buffer.SetCapacity(cursor);

you should SendWriteData inside the loop (also to have the above mentioned granularity) when reader succeeds.  when reader fails, break immediately with an error.

::: netwerk/protocol/http/AltDataOutputStreamChild.h
@@ +18,5 @@
> +  : public PAltDataOutputStreamChild
> +  , public nsIOutputStream
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

so, this class has thread safe refcnt, is it actually going to be used on multiple threads?  then why e.g. the mError member is not atomic?  and why there are no comments how this class is used?

::: netwerk/protocol/http/AltDataOutputStreamParent.cpp
@@ +34,5 @@
> +  }
> +  nsresult rv;
> +  uint32_t n;
> +  if (mOutputStream) {
> +    rv = mOutputStream->Write(data.BeginReading(), data.Length(), &n);

maybe assert n == data.Length()?

::: netwerk/protocol/http/AltDataOutputStreamParent.h
@@ +18,5 @@
> +  : public PAltDataOutputStreamParent
> +  , public nsISupports
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

probably similar MT/ST uncertainly here?

::: netwerk/protocol/http/PAltDataOutputStream.ipdl
@@ +20,5 @@
> +  async __delete__();
> +
> +child:
> +  async Error(nsresult err);
> +};

please add comments (even before f? only)

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4383,5 @@
> +        rv = cacheEntry->OpenAlternativeInputStream(mPreferredCachedAltDataType, getter_AddRefs(stream));
> +        if (NS_SUCCEEDED(rv)) {
> +            // We have succeeded.
> +            mAvailableCachedAltDataType = mPreferredCachedAltDataType;
> +            mCachedResponseHead->SetContentLength(-1);

comment!  why?

@@ +6569,4 @@
>      CloseCacheEntry(!contentComplete);
>  
> +    // If a preferred alt-data type was set, restore mCacheEntry in case the
> +    // listener wants to open the altData output stream.

explain why you just don't bypass CloseCacheEntry (want to doom the entry on failure? or why?)

@@ +6570,5 @@
>  
> +    // If a preferred alt-data type was set, restore mCacheEntry in case the
> +    // listener wants to open the altData output stream.
> +    if (!mPreferredCachedAltDataType.IsEmpty()) {
> +        mCacheEntry = cacheEntry;

swap?

@@ +6961,5 @@
> +        return NS_ERROR_NOT_AVAILABLE;
> +    }
> +    nsresult rv = mCacheEntry->OpenAlternativeOutputStream(type, _retval);
> +    if (NS_FAILED(rv)) {
> +        return NS_ERROR_NOT_AVAILABLE;

return rv?
Attachment #8774984 - Flags: feedback?(honzab.moz) → feedback+
Comment on attachment 8774986 [details] [diff] [review]
Make alt-data responses report the correct content length

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

::: netwerk/cache2/nsICacheEntry.idl
@@ +201,5 @@
>  
>    /**
> +   * Returns the length of data this entry holds.
> +   * @throws
> +   *    - NS_ERROR_IN_PROGRESS when a write is still in progress.

write of what?

@@ +204,5 @@
> +   * @throws
> +   *    - NS_ERROR_IN_PROGRESS when a write is still in progress.
> +   *    - NS_ERROR_NOT_AVAILABLE if alt data does not exist.
> +   */
> +  readonly attribute long long altDataSize;

in the future we may need to pass the type parameter here.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4385,5 @@
>              // We have succeeded.
>              mAvailableCachedAltDataType = mPreferredCachedAltDataType;
> +            int64_t altDataSize;
> +            if (NS_SUCCEEDED(cacheEntry->GetAltDataSize(&altDataSize))) {
> +                mCachedResponseHead->SetContentLength(altDataSize);

OTOH.. and what if the alt data are still being written?  you don't know the size.  or you can't get here in that case?  if not, then why would you not just take the length of the stream you just get?  no need for complicated apis then (even in the concurrent case I think it's not needed...)
Attachment #8774986 - Flags: feedback?(honzab.moz) → feedback-
(In reply to Honza Bambas (:mayhemer) from comment #48)
> ::: netwerk/cache2/nsICacheEntry.idl
> > +   * @throws
> > +   *    - NS_ERROR_IN_PROGRESS when a write is still in progress.
> write of what?

Regular data or alt-data. If any writing to the cache entry is in progress, the size of the alt-data is invalid.

> > +   */
> > +  readonly attribute long long altDataSize;
> 
> in the future we may need to pass the type parameter here.

> > +            if (NS_SUCCEEDED(cacheEntry->GetAltDataSize(&altDataSize))) {
> > +                mCachedResponseHead->SetContentLength(altDataSize);
> 
> OTOH.. and what if the alt data are still being written?  you don't know the
> size.  or you can't get here in that case?  if not, then why would you not
> just take the length of the stream you just get?  no need for complicated
> apis then (even in the concurrent case I think it's not needed...)

I tried using nsIInputStream::Available() but it doesn't work for non-blocking streams (it returns 0)
CacheFile::GetAltDataSize() does check if mOutput is non-null, meaning we are still writing data, and returns an error code.
MozReview-Commit-ID: 7uBCMfJ7kT5
* * *
[mq]: execute-soon.patch

MozReview-Commit-ID: GFJ3AXj1J1l
Attachment #8779451 - Flags: review?(honzab.moz)
Attachment #8774988 - Attachment is obsolete: true
MozReview-Commit-ID: DvQP7NB4SqW
Attachment #8779454 - Flags: review?(honzab.moz)
Attachment #8774986 - Attachment is obsolete: true
MozReview-Commit-ID: 7uBCMfJ7kT5
Attachment #8779455 - Flags: review?(honzab.moz)
Attachment #8779451 - Attachment is obsolete: true
Attachment #8779451 - Flags: review?(honzab.moz)
Attachment #8774984 - Attachment is obsolete: true
Attachment #8754606 - Attachment is obsolete: true
Attachment #8753536 - Attachment is obsolete: true
* Add PAltDataOutputStream.ipdl to be able to open an OutputStream to the cache entry in the child process
* AltDataOutputStreamChild/Parent are Main Thread only for now.
* Adds methods for reading and writing alt-data to nsICacheInfoChannel.idl
* Keep a ref of the cache entry after OnStopRequest in case the consumer tries to open the alt-data output stream

MozReview-Commit-ID: jlraDI97Hg
Attachment #8779456 - Flags: review?(honzab.moz)
(In reply to Valentin Gosu [:valentin] from comment #49)
> I tried using nsIInputStream::Available() but it doesn't work for
> non-blocking streams (it returns 0)

CacheFileInputStream::Available() returns number of bytes that are available in memory and are guaranteed to not be released before CacheFileInputStream::Read() is called. I.e. even if it returns non-zero value, it isn't necessarily the length of the stream.
Comment on attachment 8779455 [details] [diff] [review]
(Part 1) Basic test for alt-data representation in cache

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

::: netwerk/test/unit/test_alt-data_simple.js
@@ +89,5 @@
> +    }};
> +
> +    // we may need some gc() calls first here...
> +    // check one of test_cache2-26-no-outputstream-open.js or test_cache2-30c-pinning-deferred-doom.js
> +    // but the entry is inside the channel, so probably no need to...

there should be some conclusion made here.  either remove the comment (confirming that no gc() is needed and the entry is freed before we get the flushed notification) or do what it says.

::: netwerk/test/unit_ipc/xpcshell.ini
@@ +54,5 @@
>    !/netwerk/test/unit/data/test_readline6.txt
>    !/netwerk/test/unit/data/test_readline7.txt
>    !/netwerk/test/unit/data/test_readline8.txt
>    !/netwerk/test/unit/data/signed_win.exe
> +  !/netwerk/test/unit/test_alt-data_simple.js

is this really needed?
Attachment #8779455 - Flags: review?(honzab.moz) → review+
Comment on attachment 8779456 [details] [diff] [review]
(Part 2) Allow storing alternate data in the HTTP cache

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

::: netwerk/cache2/CacheEntry.cpp
@@ +1202,5 @@
> +
> +  mozilla::MutexAutoLock lock(mLock);
> +
> +  if (!mHasData || mState < READY || mOutputStream || mIsDoomed) {
> +    return NS_ERROR_NOT_AVAILABLE;

log this? like:

LOG(("  entry not in state to write alt-data"));

::: netwerk/cache2/nsICacheEntry.idl
@@ +203,5 @@
> +   * Opens and returns an output stream that a consumer may use to save an
> +   * alternate representation of the data.
> +   * @throws
> +   *    - NS_ERROR_NOT_AVAILABLE if the real data hasn't been written.
> +   *    - NS_ERROR_IN_PROGRESS when the write is still in progress.

WRITE OF WHAT?  I don't want you to write comments to bugzilla, I want you to write it here :)

@@ +214,5 @@
> +   * Opens and returns an input stream that can be used to read the alternative
> +   * representation previously saved in the cache.
> +   * @throws
> +   *    - NS_ERROR_NOT_AVAILABLE if the alt-data representation doesn't exist at
> +   *      all or if alt-data of the given type doesn't exist.

maybe add a note about concurrent write and read of alt data (if possible or not and how the input then behaves)

::: netwerk/protocol/http/AltDataOutputStreamChild.cpp
@@ +67,5 @@
> +  const uint32_t kChunkSize = 256*1024;
> +  uint32_t next = std::min(data.Length(), kChunkSize);
> +  for (uint32_t i = 0; i < data.Length();
> +       i = next, next = std::min(data.Length(), next + kChunkSize)) {
> +    nsCString chunk(Substring(data, i, next));

would be nice to have a test for this.

@@ +112,5 @@
> +  }
> +  if (NS_FAILED(mError)) {
> +    return mError;
> +  }
> +  Unused << WriteDataInChunks(nsCString(aBuf, aCount));

nsDependentCSubstring please...

@@ +138,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +AltDataOutputStreamChild::WriteSegments(nsReadSegmentFun aReader, void *aClosure, uint32_t aCount, uint32_t *_retval)

write segments makes sense only on buffered streams, which this is not, and it's still just optional.  CacheFileOutputStream doesn't implement it either (despite it is buffered) as well as it doesn't even impl WriteFrom.  did you find a reason to impl this method?

::: netwerk/protocol/http/AltDataOutputStreamChild.h
@@ +13,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class AltDataOutputStreamChild

pretty insufficient comments on this class.

::: netwerk/protocol/http/AltDataOutputStreamParent.h
@@ +33,5 @@
> +  nsresult mStatus;
> +};
> +
> +} // namespace net
> +} // namespace mozilla

add comments to this class, its methods and members.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4384,5 @@
> +                                                    getter_AddRefs(stream));
> +        if (NS_SUCCEEDED(rv)) {
> +            // We have succeeded.
> +            mAvailableCachedAltDataType = mPreferredCachedAltDataType;
> +            // The alternative data may have a different length that the original

than the original

@@ +6567,5 @@
>          mListener->OnStopRequest(this, mListenerContext, status);
>          mOnStopRequestCalled = true;
>      }
>  
> +    // If a preferred alt-data type was set, restore mCacheEntry in case the

'restore' ?  and again, I miss in the comment _WHY_ you are doing this.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +450,5 @@
>      // cache specific data
>      nsCOMPtr<nsICacheEntry>           mCacheEntry;
> +    // if the consumer may try to write alt data to the cache - signaled by
> +    // HttpBaseChannel::mPreferredCachedAltDataType being not empty - this will
> +    // be set during OnStopRequest before clearing mCacheEntry.

say also why
Attachment #8779456 - Flags: review?(honzab.moz) → review+
Comment on attachment 8779454 [details] [diff] [review]
(Part 3) Make alt-data responses report the correct content length

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

Michal, please check the CacheFile::GetAltDataSize piece.

::: netwerk/cache2/CacheEntry.cpp
@@ +1495,5 @@
> +
> +NS_IMETHODIMP CacheEntry::GetAltDataSize(int64_t *aDataSize)
> +{
> +  LOG(("CacheEntry::GetAltDataSize [this=%p]", this));
> +  return mFile->GetAltDataSize(aDataSize);

you may access mFile only when mFileStatus is a success code!

::: netwerk/cache2/CacheFile.cpp
@@ +2108,5 @@
> +  if (mAltDataOffset == -1) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  *aSize = mDataSize - mAltDataOffset;

this should be reviewed by michal, I'm not sure I follow how mDataSize actually works.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4386,5 @@
>              // We have succeeded.
>              mAvailableCachedAltDataType = mPreferredCachedAltDataType;
> +            // Set the correct data size on the channel.
> +            int64_t altDataSize;
> +            if (NS_SUCCEEDED(cacheEntry->GetAltDataSize(&altDataSize))) {

hmm... so when this fails (there is an alt output stream) you leave the bad value in the cached response head?  

I may missed any comment in bugzilla or in any of these patches, but it's unclear how the concurrency here exactly works.

assuming there is concurrency allowed (also according to what you do in this patch) what happens when the writer fails (the alt data generation is interrupted)?  if the output stream is closed with an error, the input stream should fail on read and the consumer should obtain that error to be able to handle it.  I think this how the cache code already works, but should be double checked and that expectation documented - maybe here.  


This is exactly the reason why I want people to write comments - to make them understand what they do themselves.  This piece is underdocumented.  And here is the result - it's unclear what it has to do.
Attachment #8779454 - Flags: review?(honzab.moz)
Attachment #8779454 - Flags: review-
Attachment #8779454 - Flags: feedback?(michal.novotny)
Testing the current set of patches with the work-in-progress from Bug 900784 [1]
I see that some times, I can successfully open the output stream, and write a bunch of bytes in it.

Strangely, after I reload the page I either have the exact number of bytes saved, or I have an empty bytecode stream.  I would expect to either get the same bytes, or not get any alternative input stream.

I collected the logs of 2 runs of Gecko, one where the bytecode cache is produced[2] for https://normandy.cdn.mozilla.net/static/bundles/selfrepair-068962304d04a2173e88.94ed0f93a4f3.js , and a second log where the same resource is being loaded[3] from the bytecode cache.

In these logs, you can look for the log the following lines, which are respectively reporting the size of the written content, reported by the output stream

> 2016-08-24 16:28:30.906409 UTC - [Main Thread]: D/ScriptLoader ScriptLoadRequest (3a21b20): Write bytecode cache (length = 1327478, written = 1327478)

And the size reported by the alternative input stream:

> 2016-08-24 16:54:49.534920 UTC - [Main Thread]: D/ScriptLoader ScriptLoadRequest (1e2b160): Stream complete (url = https://normandy.cdn.mozilla.net/static/bundles/selfrepair-068962304d04a2173e88.94ed0f93a4f3.js)
> 2016-08-24 16:54:49.535087 UTC - [Main Thread]: D/ScriptLoader ScriptLoadRequest (1e2b160): Bytecode length = 0

[1] https://github.com/nbp/gecko-dev/compare/bugzil.la/js-startup-cache/base...nbp:bugzil.la/js-startup-cache/wip
[2] http://people.mozilla.org/~npierron/bug900784/register-normandy-cache.log
[3] http://people.mozilla.org/~npierron/bug900784/load-normandy-cache.log

PS: Don't worry if the bytecode size is way larger than the numbers that the order of magnitude mentioned at Orlando, because the source got converted to UTF-16, but we did not got time for compressing it yet (in the current patch set)
Nicolas, are you correctly closing the output stream after you've written the alt-data?  Could you provide a wip patch so we can take a look/try reproducing?

Honestly, our patch for that support is still a wip, but according my last check it should work for most common cases.  Corner cases to catch/assess/overcome are probably still there :)
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(nicolas.b.pierron)
(In reply to Honza Bambas (:mayhemer) from comment #60)
> Nicolas, are you correctly closing the output stream after you've written
> the alt-data?  Could you provide a wip patch so we can take a look/try
> reproducing?

Yes, I did [1].  I changed the code to unconditionally close the output stream local, and I can still reproduce the same issue.

I have no idea, if this might be any hint for you but I get this issue more frequently with content from other domains than the one of the page which is requesting the external resource.

[1] https://github.com/nbp/gecko-dev/compare/bugzil.la/js-startup-cache/base...nbp:bugzil.la/js-startup-cache/wip#diff-8a17b57edcc05fedea9b5ac209fc0bf2R2108
Flags: needinfo?(nicolas.b.pierron)
(In reply to Honza Bambas (:mayhemer) from comment #60)
> Nicolas, […] Could you provide a wip patch so we can take a look/try
> reproducing?

I made the following diff [2], which aggregates all the patches that I added on top of Bug 1258747 and Bug 1231565 patches.

To reproduce this issue I used a debug build of firefox, and follow these steps:
 - Start firefox.
 - Go to about:preferences to clear the cache.
 - Close firefox

 - Start firefox. (MOZ_LOG="ScriptLoader:6,timestamp,sync" …)
 - Wait until the bytecode for https://….cdn.mozilla.net/… is written.
     D/ScriptLoader ScriptLoadRequest (…): Write bytecode cache (…)
 - Close firefox

 - Start firefox. (MOZ_LOG="ScriptLoader:6,timestamp,sync")
 - Wait until the bytecode for https://….cdn.mozilla.net/… is loaded.
     D/ScriptLoader ScriptLoadRequest (…): Stream complete (url = https://….cdn.mozilla.net/….js)
     D/ScriptLoader ScriptLoadRequest (…): Bytecode length = …

Expected:
 - The length reported by “Bytecode length” should be identical to the length reported in the previous run with “Write bytecode cache”.

[2] https://bug900784.bmoattachments.org/attachment.cgi?id=8784805
Nick, I'll try to find out what's going on here.
Flags: needinfo?(valentin.gosu) → needinfo?(honzab.moz)
(In reply to Nicolas B. Pierron [:nbp] from comment #62)
> [2] https://bug900784.bmoattachments.org/attachment.cgi?id=8784805

This patch [2] doesn't apply to m-c with rebased part1-3 patches for this bug...
Attached file patches rebased to m-c@b7f7ae14590a (obsolete) —
Flags: needinfo?(honzab.moz)
Flags: needinfo?(honzab.moz)
Attachment #8779454 - Attachment is obsolete: true
Attachment #8779454 - Flags: feedback?(michal.novotny)
Attachment #8779456 - Attachment is obsolete: true
(In reply to Honza Bambas (:mayhemer) from comment #56)
> Comment on attachment 8779455 [details] [diff] [review]
> (Part 1) Basic test for alt-data representation in cache
> 
> Review of attachment 8779455 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/test/unit/test_alt-data_simple.js
> @@ +89,5 @@
> > +    }};
> > +
> > +    // we may need some gc() calls first here...
> > +    // check one of test_cache2-26-no-outputstream-open.js or test_cache2-30c-pinning-deferred-doom.js
> > +    // but the entry is inside the channel, so probably no need to...
> 
> there should be some conclusion made here.  either remove the comment
> (confirming that no gc() is needed and the entry is freed before we get the
> flushed notification) or do what it says.

So, if the the CacheEntry is supposed to be freed at this point, I don't think this is happening, regardless of how many gc() calls I make.

# D/cache2 CacheStorageService::RemoveEntry [entry=7f07686826c0]
# D/cache2   still referenced, not removing not purging, still referenced

Also, in CacheStorageService::PurgeFromMemoryRunnable::Run() shouldn't the purging be done before we notify the observers?

> > +  !/netwerk/test/unit/test_alt-data_simple.js
> is this really needed?
Yes, the test throws an error without it.
Attachment #8787169 - Attachment is obsolete: true
Flags: needinfo?(honzab.moz)
(In reply to Nicolas B. Pierron [:nbp] from comment #62)
> (In reply to Honza Bambas (:mayhemer) from comment #60)
> > Nicolas, […] Could you provide a wip patch so we can take a look/try
> > reproducing?
> 
> I made the following diff [2], which aggregates all the patches that I added
> on top of Bug 1258747 and Bug 1231565 patches.
> 
> To reproduce this issue I used a debug build of firefox, and follow these
> steps:
>  - Start firefox.
>  - Go to about:preferences to clear the cache.
>  - Close firefox
> 
>  - Start firefox. (MOZ_LOG="ScriptLoader:6,timestamp,sync" …)
>  - Wait until the bytecode for https://….cdn.mozilla.net/… is written.
>      D/ScriptLoader ScriptLoadRequest (…): Write bytecode cache (…)
>  - Close firefox
> 
>  - Start firefox. (MOZ_LOG="ScriptLoader:6,timestamp,sync")
>  - Wait until the bytecode for https://….cdn.mozilla.net/… is loaded.
>      D/ScriptLoader ScriptLoadRequest (…): Stream complete (url =
> https://….cdn.mozilla.net/….js)
>      D/ScriptLoader ScriptLoadRequest (…): Bytecode length = …
> 
> Expected:
>  - The length reported by “Bytecode length” should be identical to the
> length reported in the previous run with “Write bytecode cache”.
> 
> [2] https://bug900784.bmoattachments.org/attachment.cgi?id=8784805

We are applying content conversion in the http channel even when loading the alt-data.  That is of course bad.  Patch is a two-liner, will submit shortly.
Valentin, this needs to be incorporated to the part1 patch.

When a script is served with e.g. gzip encoding by the server, we store it gzip'ed (as coming from the server) in the cache.  The http channel is responsible to inject a correct decoder and deliver the _decoded_ data to the consumer.  This is alright.  But of course, the decoder cannot be applied to the alt-data content which is stored raw and expected begin delivered back raw.  

The problem was that we were feeding the alt-data, as stored previously, to the gzip decoder (and then to the consumer - script loader).  The load ended up with NS_ERROR_INVALID_CONTENT_ENCODING and you ended up with no content.

Solution: bypass content encoding when we are loading alt data (super simple).

@nbp: suggestion - maybe check for errors in OnStopRequest? ;)
Attachment #8788566 - Flags: review?(valentin.gosu)
(In reply to Honza Bambas (:mayhemer) from comment #57)
> Comment on attachment 8779456 [details] [diff] [review]
> (Part 2) Allow storing alternate data in the HTTP cache
> ::: netwerk/protocol/http/AltDataOutputStreamChild.cpp
> > +    nsCString chunk(Substring(data, i, next));
> 
> would be nice to have a test for this.

Adding test for this. The test actually did catch a bug here.

> > +  Unused << WriteDataInChunks(nsCString(aBuf, aCount));
> 
> nsDependentCSubstring please...

Unfortunately, I couldn't find a way to pass nsDependentCSubstring here.
The IPDL methods only accept nsCString, which does cause unnecessary copying.

> write segments makes sense only on buffered streams, which this is not, and
> it's still just optional.  CacheFileOutputStream doesn't implement it either
> (despite it is buffered) as well as it doesn't even impl WriteFrom.  did you
> find a reason to impl this method?

I had missed that. I'll remove the implementation.
(In reply to Valentin Gosu [:valentin] from comment #68)
> > there should be some conclusion made here.  either remove the comment
> > (confirming that no gc() is needed and the entry is freed before we get the
> > flushed notification) or do what it says.
> 
> So, if the the CacheEntry is supposed to be freed at this point, I don't
> think this is happening, regardless of how many gc() calls I make.
> 
> # D/cache2 CacheStorageService::RemoveEntry [entry=7f07686826c0]
> # D/cache2   still referenced, not removing not purging, still referenced
> 
> Also, in CacheStorageService::PurgeFromMemoryRunnable::Run() shouldn't the
> purging be done before we notify the observers?

The flow is:
- expected all entries are freed by external referrers
- CacheStorageService::Flush()
  - adds aObserver as an observer for "cacheservice:purge-memory-pools"
  - posts to the IO thread on the very last priority level (executed as late as possible) to perform the purge
- CacheStorageService::PurgeFromMemoryRunnable::Run() (on the IO thread)
  - performs the purge, from there you are probably seeing the logs about RemoveEntry
  - after that's done, it posts itself back to the main thread
- CacheStorageService::PurgeFromMemoryRunnable::Run() (on the main thread)
  - notify "cacheservice:purge-memory-pools"
  - the observer gets it and does its job (test continues)

Only thing that could happen is that purge is scheduled sometimes between or just before CacheStorageService::Flush() and this first purge triggers the global notification, for our purpose too soon.  But that seems very unlikely.  If that's confirmed (and only if), we could extend the CacheStorageService::PurgeFromMemoryRunnable class to take the observer and notify it directly along with notifying the global notification.  But I more think it's just JS not doing gc as expected.

Have you tried exactly this?  https://dxr.mozilla.org/mozilla-central/rev/91c2b9d5c1354ca79e5b174591dbb03b32b15bbf/netwerk/test/unit/test_cache2-30c-pinning-deferred-doom.js#45
Flags: needinfo?(valentin.gosu)
(In reply to Honza Bambas (:mayhemer) from comment #72) 
> Have you tried exactly this? 
Yes, but from what I could tell from the logs, the destructor for the CacheEntry never got called. It seems odd for this to happen, unless I'm leaking it somehow.
Flags: needinfo?(valentin.gosu)
(In reply to Valentin Gosu [:valentin] from comment #73)
> (In reply to Honza Bambas (:mayhemer) from comment #72) 
> > Have you tried exactly this? 
> Yes, but from what I could tell from the logs, the destructor for the
> CacheEntry never got called. It seems odd for this to happen, unless I'm
> leaking it somehow.

You can use XPCOM_MEM_BLOAT_LOG=/path/to/log during the test run and you'll see if you are leaking anything.

But I think I know the cause.  The http channel is probably held alive by JS (as a closure).  And the channel's not closing its cache entry because the alt-data has been requested, [1].  Could that be it?


[1] https://bugzilla.mozilla.org/attachment.cgi?id=8787612&action=diff#a/netwerk/protocol/http/nsHttpChannel.cpp_sec5
Flags: needinfo?(valentin.gosu)
Rank: 3
(In reply to Honza Bambas (:mayhemer) from comment #74)
> (In reply to Valentin Gosu [:valentin] from comment #73)
> > (In reply to Honza Bambas (:mayhemer) from comment #72) 
> > > Have you tried exactly this? 
> > Yes, but from what I could tell from the logs, the destructor for the
> > CacheEntry never got called. It seems odd for this to happen, unless I'm
> > leaking it somehow.
> 
> You can use XPCOM_MEM_BLOAT_LOG=/path/to/log during the test run and you'll
> see if you are leaking anything.
> 
> But I think I know the cause.  The http channel is probably held alive by JS
> (as a closure).  And the channel's not closing its cache entry because the
> alt-data has been requested, [1].  Could that be it?

Confirmed! The entry gets released now that I moved the flush to an out-of-scope function and do a gc() just before I run it.
Thanks for your help!
Flags: needinfo?(valentin.gosu)
Fixed the issue with GC. However, in e10s we have a different behaviour, as the GC and flushing happen in the content process, while the cache entry lives in the parent process.
Attachment #8779455 - Attachment is obsolete: true
Attachment #8787612 - Attachment is obsolete: true
MozReview-Commit-ID: DvQP7NB4SqW
Attachment #8788967 - Flags: review?(michal.novotny)
Attachment #8787611 - Attachment is obsolete: true
Comment on attachment 8788566 [details] [diff] [review]
(no content encoding when loading alt-data)

Folded into part 2 of the patches.
Attachment #8788566 - Attachment is obsolete: true
Attachment #8788566 - Flags: review?(valentin.gosu)
(In reply to Honza Bambas (:mayhemer) from comment #58)
> I may missed any comment in bugzilla or in any of these patches, but it's
> unclear how the concurrency here exactly works.
> 
> assuming there is concurrency allowed (also according to what you do in this
> patch) what happens when the writer fails (the alt data generation is
> interrupted)?  if the output stream is closed with an error, the input
> stream should fail on read and the consumer should obtain that error to be
> able to handle it.  I think this how the cache code already works, but
> should be double checked and that expectation documented - maybe here.  

I added another test to verify that large content and streaming works.
However, the error reporting could be a bit tricky, especially in e10s. The writer may have closed the stream completely before an error gets reported back. And the writer has no way of calling CloseWithStatus as it doesn't implement nsIAsyncOutputStream (should it?).
That still leaves async e10s as a problem in reporting errors from writer to reader, and backwards.
Comment on attachment 8788966 [details] [diff] [review]
Test that large alt-data content can be saved, and that streaming works

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

I don't know why you so insist on releasing the entry while it's still being written.

except that, the test makes sense.

::: netwerk/test/unit/test_alt-data_stream.js
@@ +101,5 @@
>  
> +function openAltChannel()
> +{
> +  // We need to do a GC pass to ensure the cache entry has been freed.
> +  gc();

if you keep the output stream, which refers the entry, then how can you make the entry release?  That doesn't make much sense to me.  There cannot be two entries for the same URL at the same time.  If there are, then it's a bug!

if you want to do a concurrent read test, then you don't have to (actually must not) release the existing entry.  I also don't see anything that would purge the entry.

This simply doesn't make sense at all to me.

@@ +115,5 @@
> +  onStartRequest: function(request, context) { },
> +  onDataAvailable: function(request, context, stream, offset, count) {
> +    let string = NetUtil.readInputStreamToString(stream, count);
> +    this.buffer += string;
> +    if (this.buffer.length == firstChunkSize) {

this condition seems a bit volatile to me...

@@ +119,5 @@
> +    if (this.buffer.length == firstChunkSize) {
> +      // write the rest of the content
> +      os.write(altContent.substring(firstChunkSize, altContent.length), altContent.length - firstChunkSize);
> +      os.close();
> +      os = null;

no effect until gc() is made
Attachment #8788966 - Flags: review?(honzab.moz) → feedback+
Comment on attachment 8788967 [details] [diff] [review]
(Part 3) Make alt-data responses report the correct content length

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

::: netwerk/cache2/nsICacheEntry.idl
@@ +202,5 @@
> +  *    - NS_ERROR_IN_PROGRESS when a write is still in progress (either real
> +                              content or alt data).
> +  *    - NS_ERROR_NOT_AVAILABLE if alt data does not exist.
> +  */
> +  readonly attribute long long altDataSize;

You need to change UUID

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4500,4 @@
>              mCachedResponseHead->SetContentLength(-1);
> +            // Set the correct data size on the channel.
> +            int64_t altDataSize;
> +            if (NS_SUCCEEDED(cacheEntry->GetAltDataSize(&altDataSize))) {

Do we have a test for concurrent read/write access for the alternative data?
Attachment #8788967 - Flags: review?(michal.novotny) → review+
> ::: netwerk/cache2/nsICacheEntry.idl
> 
> You need to change UUID

We stopped having to change UUIDs a while ago.
I removed the gc() part, which was just left over from the previous patch. I couldn't think of a better condition for the test, but I added a comment saying so. Do you think this test is enough to check concurrency? If not, could you let me know what would be better? Thanks!
Attachment #8794664 - Flags: review?(honzab.moz)
Attachment #8788966 - Attachment is obsolete: true
Comment on attachment 8794664 [details] [diff] [review]
(Part 4) Test that large alt-data content can be saved, and that streaming works

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

The patch series don't apply to the current m-c, so I cannot check this test does what it is expected to myself.  If you want to check it, just set nsHttp:5 logging and check for "write is in progress" log line under "nsHttpChannel::OnCacheEntryCheck" for the second channel instance, and then "nsHTTPChannel::OnCacheEntryCheck exit" with result=0 (=ENTRY_WANTED).  If you find it, the second channel reads concurrently.  If not, something is wrong.

::: netwerk/test/unit/test_alt-data_stream.js
@@ +117,5 @@
>  
> +    // XXX: this condition might be a bit volatile. If this test times out,
> +    // it probably means that for some reason, the listener didn't get all the
> +    // data in the first chunk.
> +    if (this.buffer.length == firstChunkSize) {

I was more thinking like having a flag that tells you "I'm in ondataavailable the first time, write the rest".  But this may work well too, if not, we know what to try to change as the first thing ;)
Attachment #8794664 - Flags: review?(honzab.moz) → review+
Addressed IRC comments - removed Etag and no-cache
Attachment #8794664 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/40417bdcaedecc7f7430bc45ac1b703eb7859d34
Bug 1231565 - (Part 1) Basic test for alt-data representation in cache r=honzab

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c8123885905804ac06f6dc01d0f7f969f0ceb8
Bug 1231565 - (Part 2) Allow storing alternate data in the HTTP cache r=honzab

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a0e25cebb306ee52992d15cffbc47248309f91
Bug 1231565 - (Part 3) Make alt-data responses report the correct content length r=michal

https://hg.mozilla.org/integration/mozilla-inbound/rev/cde7a9badcab194e25adf5fc48a991f6fa41fb87
Bug 1231565 - (Part 4) Test that large alt-data content can be saved, and that streaming works r=honzab
Depends on: 1306033
Whiteboard: [necko-active], DevAdvocacy-Facebook [platform-rel-Facebook] → DevAdvocacy-Facebook [platform-rel-Facebook]
Depends on: 1341343
Depends on: 1347470
Depends on: 1487100
You need to log in before you can comment on or make changes to this bug.