Last Comment Bug 605101 - Unfortunate link between writing content to disk-entry and indicating whether a separate file is used
: Unfortunate link between writing content to disk-entry and indicating whether...
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 605108 (view as bug list)
Depends on:
Blocks: 605108
  Show dependency treegraph
 
Reported: 2010-10-18 05:31 PDT by Bjarne (:bjarne)
Modified: 2016-02-08 07:08 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Checks DataLocationInitialized() in addition to DataFile() (7.48 KB, patch)
2010-10-24 13:00 PDT, Bjarne (:bjarne)
no flags Details | Diff | Review
Slightly improved version (better comments and keeping assertion) (7.78 KB, patch)
2010-11-16 05:47 PST, Bjarne (:bjarne)
no flags Details | Diff | Review

Description Bjarne (:bjarne) 2010-10-18 05:31:41 PDT
This comes from bug #588804, comment #23.

The method nsDiskCacheRecord::DataFile() returns 0 if the content of the record is stored in a separate file. It is used several places in the code to decide whether to use block-files or separate files. However, as described in bug #588804, comment #23 this method returns 0 also for entries which has no content (e.g. certain 3xx responses).

IMO this is unfortunate and can lead to issues which we don't really understand at the moment. So I'll suggest to remove this link between whether content has been written and whether content is/should be stored in a separate file by re-defining the contract for the method to return e.g. -1 (or some very high int-value) if the entry has no content.
Comment 1 Bjarne (:bjarne) 2010-10-24 13:00:01 PDT
Created attachment 485610 [details] [diff] [review]
Checks DataLocationInitialized() in addition to DataFile()

When looking more closely at this issue it seems like checking nsDiskCacheRecord::DataLocationInitialized() in addition to nsDiskCacheRecord::DataFile() will give us what we need. I.e. if nsDiskCacheRecord::DataLocationInitialized() is true in addition to nsDiskCacheRecord::DataFile() == 0 we know for sure that the content of the entry is really in a separate file.

With this patch the issue in bug #605108 is resolved. It passes tryserver and random local browsing.
Comment 2 Bjarne (:bjarne) 2010-11-16 05:47:24 PST
Created attachment 490856 [details] [diff] [review]
Slightly improved version (better comments and keeping assertion)

Also changing reviewer since Michal seems to be busy.

A quick comment to the removed else-block in nsDiskCacheDevice::GetFileForEntry(): IMO it is completely wrong. We are asking for the file for an entry and if it is not initialized at this point, we should certainly not do it here. (And if we do want to initialize the file here, the method-name should really indicate this, IMHO.) The only combination after this fix which will return a file is (DataLocationInitialized() && DataFile()==0).
Comment 3 Bjarne (:bjarne) 2011-01-12 01:54:16 PST
*** Bug 605108 has been marked as a duplicate of this bug. ***
Comment 4 Jason Duell [:jduell] (needinfo? me) 2012-01-04 12:22:54 PST
Comment on attachment 490856 [details] [diff] [review]
Slightly improved version (better comments and keeping assertion)

Bjarne,

Sorry to sit on this review so long just to punt it back to Michal, but I do think he's the better reviewer for this.
Comment 5 Michal Novotny (:michal) 2012-01-12 18:02:34 PST
Comment on attachment 490856 [details] [diff] [review]
Slightly improved version (better comments and keeping assertion)

(In reply to Bjarne (:bjarne) from comment #2)
> A quick comment to the removed else-block in
> nsDiskCacheDevice::GetFileForEntry(): IMO it is completely wrong. We are
> asking for the file for an entry and if it is not initialized at this point,
> we should certainly not do it here. (And if we do want to initialize the
> file here, the method-name should really indicate this, IMHO.) The only
> combination after this fix which will return a file is
> (DataLocationInitialized() && DataFile()==0).

I tend to agree with you. Maybe we might want to create an empty file in case that the STORE_AS_FILE policy was explicitly specified (i.e. nsHttpChannel::SetCacheAsFile() was called)?


> -    } else if (fileIndex < 4) {
> +    } else if (fileIndex > 0 && fileIndex < 4) {
>          // deallocate blocks
>          PRUint32  startBlock = metaData ? record->MetaStartBlock() : record->DataStartBlock();
>          PRUint32  blockCount = metaData ? record->MetaBlockCount() : record->DataBlockCount();

What about to add NS_ASSERTION(isInitialized,...) to this block in nsDiskCacheMap::DeleteStorage()?


>      mStreamEnd = mBinding->mCacheEntry->DataSize();
> -    if (mStreamEnd == 0) {
> +    if (mStreamEnd == 0 || !mBinding->mRecord.DataLocationInitialized()) {
>          // there's no data to read
>          NS_ASSERTION(!mBinding->mRecord.DataLocationInitialized(), "storage allocated for zero data size");
>      } else if (mBinding->mRecord.DataFile() == 0) {

Wouldn't make more sense to change the second if-statement? I.e.

} else if (mBinding->mRecord.DataFile() == 0 && mBinding->mRecord.DataLocationInitialized()) {


> +    // BGH: make an auto-object here which dooms cache-entry unless told otherwise?

You mean to doom the entry in case of any error, right? See my comment #7 in bug #712277. We should IMO handle errors (and then doom the entry) in nsCacheEntryDescriptor. It would solve this issue for any output regardless of whether it is nsOutputStreamWrapper or nsCompressOutputStreamWrapper.
Comment 6 Bjarne (:bjarne) 2012-02-06 03:20:53 PST
-> default owner
Comment 7 Patrick McManus [:mcmanus] 2016-02-08 07:08:29 PST
new cache code

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