1010783 - Base CacheFileInputStream::Read on ReadSegments

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

to adopt the multisegment reading logic.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

- reimpls Read by ReadSegments, inspired with http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsBufferedStreams.cpp#312
- fixes a major bug in the ReadSegments impl: the offset in the buffer was not correctly shifted

I will write a test this time (follow on patch), since this is pretty serious.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: test v1

Comment 3

[Michal Novotny [:michal]]

Attachment: patch v2

It turned out that Read()/ReadSegments() must be able to read such amount that was returned by Available(). This patch ensures that Available() uses only preloaded chunks to compute the value and also guarantees that these chunks won't be released.

Comment 4

[Michal Novotny [:michal]]

Comment on attachment 8424349 [details] [diff] [review]
test v1

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

It is good to have such test, so r+. But AFAICS this won't test the problem in bug 1011873 (which is fixed by the patch in this bug) since the data must be gzipped and must be fetched via nsHttpChannel and not directly from the cache.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8424352 [details] [diff] [review]
patch v2

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

::: netwerk/cache2/CacheFile.cpp
@@ +195,5 @@
> +  // preloadChunkCount won't change during CacheFile's lifetime since otherwise
> +  // we could potentially release some cached chunks that was used to calculate
> +  // available bytes but would not be available later during call to
> +  // CacheFileInputStream::Read().
> +  mPreloadChunkCount = CacheObserver::PreloadChunkCount();

note that we should fix this in consumers actually.  I haven't seen anywhere written read must return what avail proclaims.  but for now better to yield.

@@ +1244,3 @@
>  
> +bool
> +CacheFile::CanReleaseCachedChunk(uint32_t aIndex)

CacheFile::ShouldKeepCachedChunk and keep the logic polarity?

@@ -1222,3 @@
>  
> -  if (preloadChunkCount == 0) {
> -    // Preloading of chunks is disabled

this condition should stay

@@ +1400,5 @@
> +  // available bytes, since only those chunks are guaranteed not to be released.
> +  uint32_t maxPreloadedChunk = aIndex + mPreloadChunkCount;
> +  if (lastChunk < maxPreloadedChunk) {
> +    maxPreloadedChunk = lastChunk;
> +  }

don't do this for memory only entries, we would be loosing speed!

std::min might be more readable

Comment 6

[Michal Novotny [:michal]]

Attachment: patch v3

(In reply to Honza Bambas (:mayhemer) from comment #5)
> @@ +1244,3 @@
> >  
> > +bool
> > +CacheFile::CanReleaseCachedChunk(uint32_t aIndex)
> 
> CacheFile::ShouldKeepCachedChunk and keep the logic polarity?

I've renamed both methods. ShouldCacheChunk() is called on places where we decide whether we should store the chunk in mCachedChunks. MustKeepCachedChunk() is called when we decide whether to remove the cached chunk from mCachedChunk.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8424356 [details] [diff] [review]
patch v3

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

thanks!  r=honzab

Comment 8

[Michal Novotny [:michal]]

Comment on attachment 8424356 [details] [diff] [review]
patch v3

https://tbpl.mozilla.org/?tree=Try&rev=bec732a1295b

Comment 9

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91a4b6f09d18

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: test v2

The test fixed to work also with the old cache in case of we have to back out again.  Only difference is added a secondary asyncOpenCacheEntry that reopens the entry for read - the old cache cannot open input stream on a fresh entry (write access only - what a limitation :))).

Re comment 4: this test is intentionally checking the cache API and not the http channel/gzip decoder case.  After we fix the decoder to accept less bytes from read() than available() has promised, we will have a test for that too.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: test v2 alt

Hmmm.. better not to run for cache1.  It doesn't make sense for cache1 and I don't want to keep the additional asyncOpen when not necessary.  When cache1 goes away, we will just remove the don't-run condition.

Comment 15

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c660e993c557
https://tbpl.mozilla.org/?tree=Try&rev=25cc09360110

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/91a4b6f09d18
https://hg.mozilla.org/mozilla-central/rev/1527e6a3c46c