The default bug view has changed. See this FAQ.

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

RESOLVED WONTFIX

Status

()

Core
Networking: Cache
RESOLVED WONTFIX
7 years ago
a year ago

People

(Reporter: bjarne, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
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.
Assignee: nobody → bjarne
Status: NEW → ASSIGNED
Attachment #485610 - Flags: review?(michal.novotny)
(Reporter)

Updated

7 years ago
Blocks: 605108
(Reporter)

Comment 2

7 years ago
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).
Attachment #485610 - Attachment is obsolete: true
Attachment #490856 - Flags: review?(jduell.mcbugs)
Attachment #485610 - Flags: review?(michal.novotny)
(Reporter)

Updated

6 years ago
Duplicate of this bug: 605108
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.
Attachment #490856 - Flags: review?(jduell.mcbugs) → review?(michal.novotny)
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.
Attachment #490856 - Flags: review?(michal.novotny)
(Reporter)

Comment 6

5 years ago
-> default owner
Assignee: bjarne → nobody
new cache code
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.