Closed Bug 1289163 Opened 3 years ago Closed 3 years ago

Number of empty entries created in the HTTP cache, for unexpected schemes.

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

Nightly (up to date), working profile (way old), windows.

I have a cache (~240MB of the 350MB limit) having a huge (one third) of entries (~3000) being completely empty (=no data and no metadata, just URL + control sums).

I can see cache entries created also for data: (usually images) and also even for about:blank.  All are just those empty entries that should never exist in the cache.

This might be coming from net predictor, that is probably using the API correctly, but the cache does something wrong then.
Just a data point - predictor shouldn't be doing *anything* for non-http{,s} schemes, let alone opening/creating cache entries for them. If it is, that's something separate we should investigate.
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #1)
> Just a data point - predictor shouldn't be doing *anything* for non-http{,s}
> schemes, let alone opening/creating cache entries for them. If it is, that's
> something separate we should investigate.

Thanks!  Good to know.
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #1)
> Just a data point - predictor shouldn't be doing *anything* for non-http{,s}
> schemes, let alone opening/creating cache entries for them. If it is, that's
> something separate we should investigate.

So...

 	xul.dll!mozilla::net::CacheStorage::AsyncOpenURI(0x1dcd0fa0, {...}, 0x00000032, 0x218466a0) Line 79	C++
 	xul.dll!mozilla::net::Predictor::UpdateCacheabilityInternal(0x1dcd0fa0, 0x1ecb0600, 0x000000c8, {...}) Line 2459	C++
 	xul.dll!mozilla::net::Predictor::UpdateCacheability(0x1dcd0fa0, 0x1ecb0600, 0x000000c8, {...}, 0x143c75e0, 0x218451c0) Line 2441	C++
>	xul.dll!mozilla::net::nsHttpChannel::ProcessResponse() Line 1798	C++
 	xul.dll!mozilla::net::nsHttpChannel::OnStartRequest(0x1f016580, 0x00000000) Line 6225	C++
 	xul.dll!nsInputStreamPump::OnStateStart() Line 524	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(0x20262160) Line 426	C++

Where sourceURI == "about:blank", because the http channel's referrer is "about:blank".

Obtained:

    nsCOMPtr<nsIURI> referrer = GetReferringPage();
    if (!referrer) {
        referrer = mReferrer;
    }
    if (referrer) {
        nsCOMPtr<nsILoadContextInfo> lci = GetLoadContextInfo(this);
        mozilla::net::Predictor::UpdateCacheability(referrer, mURI, httpStatus,
                                                    mRequestHead, mResponseHead,
                                                    lci);
    }

mReferrer is null.

Nick, can you do anything about this?

Michal, would it be easy (better said - trivial) enough to don't allow such entries be flushed to the cache actually?  I first would like to prevent their existence at all (hence handing to Nick), because for the cache to decide whether or how long to keep an empty entry around may be hard.
Assignee: nobody → hurley
Flags: needinfo?(michal.novotny)
Whiteboard: [necko-active]
(In reply to Honza Bambas (:mayhemer) from comment #3)
> Nick, can you do anything about this?

Yep, looks like I just forgot to put the http{,s}-only checks in UpdateCacheability. I'll prep the patch once my current local build completes.
Ups... had to look at the open flags first!  Yes, it's opened as READONLY, so no file should ever be created for this.  And it isn't!

I'm looking further and it seems that the report of empty entries is coming just from the about:cache.  Those are temporarily kept in memory (and in the index) and SHOULD never go to the disk.  But do.  Why?  When you click an entry in about:cache, it's open WITH the READONLY flag, but even just that makes the entry be (somehow) marked dirty and written to disk.


I'll reassign to Michal, since file handling in the cache is his area more than mine.

The goal here would be to simply never write anything, until the entry is written at least something (meta or data).

I'll make this blocking bug 1289164 since that one could hide this issue.

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #4)
> (In reply to Honza Bambas (:mayhemer) from comment #3)
> > Nick, can you do anything about this?
> 
> Yep, looks like I just forgot to put the http{,s}-only checks in
> UpdateCacheability. I'll prep the patch once my current local build
> completes.

I don't think there is much for you to do here actually, Nick.  If you filter this just by http(s), you may still create 'empty' entries that will get reported in about:cache and may get to the disk.
Assignee: hurley → michal.novotny
Flags: needinfo?(michal.novotny)
(In reply to Honza Bambas (:mayhemer) from comment #5)
> I don't think there is much for you to do here actually, Nick.  If you
> filter this just by http(s), you may still create 'empty' entries that will
> get reported in about:cache and may get to the disk.

Maybe not on this bug, but it does make sense for me to make the change mentioned anyway - no use in doing work/attempting to open cache entries that we'll never make use of! I'll file a new bug for that one, though.
Blocks: 1289164
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #6)
> Maybe not on this bug, but it does make sense for me to make the change

Bug 1289564.
The real problem actually is not in the file back end but on the cache entry level.  We touch the cache file state (mFile->OnFetched) even when an entry has not been found.  That makes it write itself to the disk.
Assignee: michal.novotny → honzab.moz
Status: NEW → ASSIGNED
- call mFile->OnFetched() only when we actually deliver an entry
- it was happening also when a readonly open was performed and there were no entry (no file)
- it marked the metadata as dirty and those were written (perfectly legal, we are deliberately dumb at this, upper layers take care to not write empty entries - now)
Attachment #8779439 - Flags: review?(michal.novotny)
Attachment #8779439 - Flags: review?(michal.novotny) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a66acdcf5646
Make sure no HTTP cache entry file is created when URL opened as read-only. r=michal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a66acdcf5646
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.