605108 - nsICacheEntryDescriptor for disk-cache entry with no content points to non-existent file

Status: RESOLVED DUPLICATE of bug 605101

(Core :: Networking: Cache, defect)

Description

[Bjarne (:bjarne)]

This comes from bug #588804, comment #23.

If a cached entry on disk has no content, the about:cache description for the entry will claim content is stored in a separate file which doesn't exist. This is simple to observe for e.g. certain 3xx-responses which are cached but do not have content.

Not sure what consequences this may have, except for the obvious fact that the about:cache description presented to a user is wrong. Components, add-ons or plugins may all use this info.

The code in nsAboutCacheEntry::WriteCacheEntryDescription() uses the attribute nsICacheEntryDescriptor::file to get the filename. IMO, we could extend this interface to throw if the returned file doesn't exist.

Comment 1

[Bjarne (:bjarne)]

Fixing bug #605101 will most likely also handle this one in practice, but we may still want to extend nsICacheEntryDescriptor to throw is the file-attribute denotes a file which doesn't exist.

Comment 2

[Bjarne (:bjarne)]

Attachment: Simple, v1 patch

Simple check in nsDiskCacheDevice

Afaics, the only possible issue with this is performance but I doubt it will be a huge problem...

Comment 3

[Michal Novotny [:michal]]

Comment on attachment 483969 [details] [diff] [review]
Simple, v1 patch

(In reply to comment #1)
> Fixing bug #605101 will most likely also handle this one in practice, but we
> may still want to extend nsICacheEntryDescriptor to throw is the file-attribute
> denotes a file which doesn't exist.

Why do we need this if bug #605101 will solve the problem for empty entries (like 304 response)? After fixing #605101 this could happen e.g. in case when user manually removes the file. But this patch doesn't fix this problem. It only changes what about:cache-entry displays.

Comment 4

[Bjarne (:bjarne)]

(In reply to comment #3)
> But this patch doesn't fix this problem. It
> only changes what about:cache-entry displays.

Yes - ref the title of the bug. :)

Fixing this is a lot simpler (and has probably infinitely less potential for regression) than fixing bug #605101. In fact, this can be considered a hack to fix the display-issue until a more proper fix is done. Whether or not the display-issue *needs* to be fixed is a decision for whoever approves it for a release, IMO.

Comment 5

[Michal Novotny [:michal]]

(In reply to comment #4)
> > But this patch doesn't fix this problem. It
> > only changes what about:cache-entry displays.
> 
> Yes - ref the title of the bug. :)

So why don't to fix it in nsAboutCacheEntry.cpp? There we could simply differentiate between none and missing (file isn't present but should be!).

Comment 6

[Bjarne (:bjarne)]

Good point - guess I instinctively put the fix as "deep" in the code as reasonable. :\

I've changed the title of this bug to refer to instances of nsICacheEntryDescriptor, broadening the scope of the fix. We agree that the real fix for this problem is bug #605101, but it will modify internals of the disk-cache and I'm not sure we want to do that now.

The proposed patch for this bug impacts all users of nsICacheEntryDescriptor, among them the about:cache description. I've pushed the patch to tryserver to get a thorough check because of this.

Comment 7

[Bjarne (:bjarne)]

> I've pushed the patch to tryserver to
> get a thorough check because of this.

Something weird happened to the push...  pushed again.

Comment 8

[Michal Novotny [:michal]]

(In reply to comment #6)
> Good point - guess I instinctively put the fix as "deep" in the code as
> reasonable. :\

That's a good idea in general, but I still do think that this fix is wrong.

In case when the file is missing the user gets a wrong information. Why he should see "none" when even the cache code tries to open some file when loading the resource? I'm proposing a fix inside nsAboutCacheEntry.cpp:

1) if data size is 0, we shouldn't call GetFile() and display any file at all
2) if data size > 0 and GetFile() doesn't return a file, we should display "none"
3) if data size > 0 and Getfile() returns a file, we should display that file even if it doesn't exist

IMO this would give a user the most correct information.

Comment 9

[Bjarne (:bjarne)]

This is handled by patch in bug #605101, comment #2.

Comment 10

[Benjamin Smedberg]

Comment on attachment 483969 [details] [diff] [review]
Simple, v1 patch

I don't see a reason to take this fix before we branch, unless there's a specific case that we know is currently failing.

Comment 11

[Bjarne (:bjarne)]

I'm duping this since this is a special case of bug #605101 and the proposed fix handles this case as well.