Closed Bug 1010783 Opened 6 years ago Closed 6 years ago

Base CacheFileInputStream::Read on ReadSegments

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mayhemer, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

to adopt the multisegment reading logic.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Blocks: 1011873
Attached patch v1 (obsolete) — Splinter Review
- 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.
Attachment #8424332 - Flags: review?(michal.novotny)
Attached patch test v1 (obsolete) — Splinter Review
Attachment #8424349 - Flags: review?(michal.novotny)
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #8424332 - Attachment is obsolete: true
Attachment #8424332 - Flags: review?(michal.novotny)
Attachment #8424352 - Flags: review?(honzab.moz)
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.
Attachment #8424349 - Flags: review?(michal.novotny) → review+
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
Attachment #8424352 - Flags: review?(honzab.moz) → feedback+
Attached patch patch v3Splinter Review
(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.
Attachment #8424352 - Attachment is obsolete: true
Attachment #8424356 - Flags: review?(honzab.moz)
Comment on attachment 8424356 [details] [diff] [review]
patch v3

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

thanks!  r=honzab
Attachment #8424356 - Flags: review?(honzab.moz) → review+
Blocks: 1012190
Blocks: 1012331
Attached patch test v2 (obsolete) — Splinter Review
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.
Attachment #8424349 - Attachment is obsolete: true
Attachment #8424445 - Flags: review?(michal.novotny)
Attached patch test v2 altSplinter Review
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.
Attachment #8424445 - Attachment is obsolete: true
Attachment #8424445 - Flags: review?(michal.novotny)
Attachment #8424447 - Flags: review?(michal.novotny)
Duplicate of this bug: 1011873
Duplicate of this bug: 1012190
Attachment #8424447 - Flags: review?(michal.novotny) → review+
Duplicate of this bug: 1012237
Duplicate of this bug: 1012414
Duplicate of this bug: 1012470
Duplicate of this bug: 1012502
Duplicate of this bug: 1012428
Duplicate of this bug: 1012636
Duplicate of this bug: 1012708
Duplicate of this bug: 1012685
https://hg.mozilla.org/mozilla-central/rev/91a4b6f09d18
https://hg.mozilla.org/mozilla-central/rev/1527e6a3c46c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Duplicate of this bug: 1013285
Duplicate of this bug: 1013086
Duplicate of this bug: 1013281
Duplicate of this bug: 1013515
Duplicate of this bug: 1013923
Duplicate of this bug: 1014172
Duplicate of this bug: 1012706
You need to log in before you can comment on or make changes to this bug.