Closed Bug 1007071 Opened 6 years ago Closed 6 years ago

Check cache entry size in CacheFileOutputStream::Write()

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mayhemer, Assigned: michal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

(In reply to Jason Duell (:jduell) from comment #13)
> I'd like to understand how netflix manages to store 10GB worth of stuff when
> we allegedly enforce a 5MB max object size in the memory cache (so I'd
> assume large video loads wouldn't get stored).  Honza, we already talked
> about this but I wanted to make sure we don't forget to figure out why, so
> I'm needinfo-ing you :)

(By Michal:)

I think you were talking about it with me. We check whether the entry is too big only in CacheEntry::SetPredictedDataSize() which is called from nsHttpChannel::CallOnStartRequest(). When the content length is unknown (-1) we always cache the entry. I think we should check the entry's size also in CacheFileIOManager::WriteInternal().
Question here is, what to do about memory-only entries.  Should we throw the entry's data (cached chunks) away when there is just a single/none input stream open?  We already have a mechanism when data are not found (NS_ERROR_FILE_CORRUPTED or NS_ERROR_FILE_NOT_FOUND error in nsHttpChannel::OnStartRequest - is it applicable here as well?) the channel will restart and load from network transparently.
Summary: Consider cache entry size checking in CacheFileIOManager::WriteInternal() → Check cache entry size in CacheFileOutputStream::Write()
Attached patch patch v1 (obsolete) — Splinter Review
The right place to check the entry size is probably in CacheFileOutputStream::Write() since this covers memory-only entries as well. I've tested locally that when there is a second channel that wants to read the data, it successfully reads the data when the writer dooms the entry. But AFAICT it isn't easy to make an automated test for this scenario.

This patch also fixes setting an error in input and output stream. Now in case of some internal error, the stream is immediately closed with an error instead of just setting mStatus member.
Assignee: nobody → michal.novotny
Status: NEW → ASSIGNED
Attachment #8421735 - Flags: review?(honzab.moz)
There are some Android 4.0 Opt X failures:
TEST-UNEXPECTED-FAIL | /builds/panda-0573/test/build/tests/xpcshell/tests/netwerk/test/unit/test_bug654926_doom_and_read.js | Test timed out
TEST-UNEXPECTED-FAIL | /builds/panda-0573/test/build/tests/xpcshell/tests/netwerk/test/unit/test_bug654926_doom_and_read.js | test failed (with xpcshell return code: -1), see following log:
TEST-UNEXPECTED-FAIL | /builds/panda-0573/test/build/tests/xpcshell/tests/netwerk/test/unit/test_bug654926_test_seek.js | Test timed out
TEST-UNEXPECTED-FAIL | /builds/panda-0573/test/build/tests/xpcshell/tests/netwerk/test/unit/test_bug654926_test_seek.js | test failed (with xpcshell return code: -1), see following log:
Comment on attachment 8421735 [details] [diff] [review]
patch v1

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

r=honzab but pending on the test failures resolved.

::: netwerk/cache2/CacheFileInputStream.cpp
@@ +467,3 @@
>    }
>  
>    MaybeNotifyListener();

MaybeNotifyListener() is called twice

@@ +571,4 @@
>      mListeningForChunk = static_cast<int64_t>(chunkIdx);
>    }
>  
>    MaybeNotifyListener();

as well here.

::: netwerk/cache2/CacheFileOutputStream.cpp
@@ +97,5 @@
> +  if (CacheObserver::EntryIsTooBig(mPos + aCount, mFile->mMemoryOnly)) {
> +    LOG(("CacheFileOutputStream::Write() - Entry is too big, failing and "
> +         "dooming the entry. [this=%p]", this));
> +
> +    mFile->DoomLocked(nullptr);

this won't do anything when mFile->mMemoryOnly is memory only, should we throw the chunks away when there is no input stream?  we can, since we have the restart logic in httpchannel (followup)
Attachment #8421735 - Flags: review?(honzab.moz) → review+
Blocks: 913823
Comment on attachment 8421735 [details] [diff] [review]
patch v1

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

::: netwerk/cache2/CacheFileOutputStream.cpp
@@ +93,5 @@
>  
>      return NS_FAILED(mStatus) ? mStatus : NS_BASE_STREAM_CLOSED;
>    }
>  
> +  if (CacheObserver::EntryIsTooBig(mPos + aCount, mFile->mMemoryOnly)) {

the second arg is aUsingDisk!  So it has to be !mFile->mMemoryOnly.
(In reply to Honza Bambas (:mayhemer) from comment #6)
> > +  if (CacheObserver::EntryIsTooBig(mPos + aCount, mFile->mMemoryOnly)) {
> 
> the second arg is aUsingDisk!  So it has to be !mFile->mMemoryOnly.

Right! And that was the cause of the failure on Android. The failing tests try to write 1MB entry to disk and the size was compared against the maximum memory entry size because of the negated argument. The memory cache size on Android is 1MB, so the maximum entry size is 128kB. Unfortunately, the write() in write_and_check() fails silently, so it is quite hard to find out what's wrong when the test times out.
https://hg.mozilla.org/mozilla-central/rev/d67479bdf342
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.