605101 - Unfortunate link between writing content to disk-entry and indicating whether a separate file is used

Status: RESOLVED WONTFIX

(Core :: Networking: Cache, defect)

Description

[Bjarne (:bjarne)]

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)]

Attachment: 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)]

Attachment: 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 4

[Jason Duell]

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]]

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)]

-> default owner

Comment 7

[Patrick McManus [:mcmanus]]

new cache code