Closed Bug 189570 Opened 22 years ago Closed 18 years ago

Aborting a page load causes untransferred content that already exists to be doomed from the disk cache

Categories

(Core :: Networking: HTTP, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: dpoch, Assigned: darin.moz)

References

()

Details

(Keywords: fixed1.8.1, Whiteboard: [FF2])

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20030113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20030113

This bug is present all the way back to Mozilla 1.1, and maybe before (I didn't
check any earlier versions.)

According to the code around line 1346 in nsHttpChannel.cpp, entries from the
cache are deleted if they are opened for writing or read/write.  This means all
page aborts (either by the stop button/esc or clicking on a link before the page
completes) delete entries from the cache that were there on a previous visit. 
This causes a major performance hit during revisits if the user doesn't let
every web page they visit completely finish before clicking on a link to go
somewhere else.  (And at 21600 bps, that's a long wait!)  The deletions are
unnecessary and impractical.

Reproducible: Always

Steps to Reproduce:
1. Find a slow connection, or simulate one.
2. Set the cache update mode to something other than "never".
3. Go to yerf.com/recent.html (or any page with lots of images that revalidate
(do conditional GETs) when you revisit them.)
4. Visit the page once and allow all of the thumbnail images to transfer and the
Mozilla throbber to stop moving.
5. Renew the session by closing and restarting the browser (or whatever is
necessary to get the page to revalidate the images (do conditional GETs)
6. Stop the loading of the page before all of the thumbnails on the page load.
7. Switch to offline mode and browse away and then back to the page.
Actual Results:  
The images that didn't transfer now show the placeholders and a broken image
icon, and about:cache shows that they have been deleted.  Enabling logging shows
that all of the entries that were never transferred get a "dooming cache
entry!!" message in the log.

Expected Results:  
Revisiting the page that was aborted should still have the images present in the
cache.

These incomplete page loads are the actual cause of the bug I thought was
related to expire times I reported last week.

I'm not sure what the purpose of deleting any entry that has been opened for
writing is?  It seems to me that if the cache entry is opened for writing,
either it's been replaced by valid complete new data, partial new data, or it
hasn't been actually written to at all.  If it's partial, shouldn't the browser
just store the partial entry and do a byte-range get the next time around?

Maybe the real problem (and solution) is when a user aborts a page,
NS_FAILED(status) shouldn't return TRUE if the 'error' was simply the user
clicking a new URL or pressing stop?

If that won't work, then the only solution is to flag whether any data actually
gets written to an entry and only doom the entry if that is the case.  (Though
again that seems to defeat the whole purpose of resuming a partial transfer.)
OK, so I commented out the code in nsHttpHChannel.cpp that dooms any entries
that are opened for writing when the entry is closed.  Having done that, I
noticed on rare occasions Mozilla doesn't recognize an entry is only partially
loaded into the cache and therefore revisiting the page results in only seeing
the part of the document that loaded.

So.... Maybe the solution is better recognition of when a page is only partially
transferred and set the partial transfer flag accordingly, but also leave the
'dooming' code out?  I'll have to give the Http code a lookover to try and
understand what it is doing on partial transfers.

I'm including Darin since he probably has a better understanding of the HTTP
network code than I do and can perhaps offer some advice.
good catch!  we definitely don't need to be dooming the cache entries if we are
just revalidating our local copy.  in fact, there is some code to make sure that
we don't do this if the document is only partially downloaded/revalidated. 
however, if the response headers have not been received or if the request has
not even been written out yet, we'll doom the cache entry :(   ... this should
be pretty easy to fix.

-> me
Assignee: gordon → darin
Severity: major → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → mozilla1.4alpha
Target Milestone: mozilla1.4alpha → Future
Severity: normal → major
Status: NEW → ASSIGNED
Component: Networking: Cache → Networking: HTTP
OS: Windows XP → All
Priority: P3 → --
Hardware: PC → All
Target Milestone: Future → mozilla1.9alpha
Whiteboard: [FF2]
Attached patch v1 patchSplinter Review
This patch makes the HTTP code only doom entries that it started writing to that cannot be resumed (via byte range requests).  I implemented this logic by setting a flag whenever we open an output stream to a cache entry.  If that flag is not set then we never doom.
Attachment #211310 - Flags: superreview?(bzbarsky)
Attachment #211310 - Flags: review?(cbiesinger)
Attachment #211310 - Flags: superreview?(bzbarsky) → superreview+
I've patched a copy of the Firefox 1.5.0.1 source with the bugfix and will test it over the next few weeks and report back if I find any unusual side effects.  Preliminary testing seems to indicate the problem is fixed with the patch, and revisiting web pages on dialup is speedier than ever.  Very nice!
Comment on attachment 211310 [details] [diff] [review]
v1 patch

ok... but see my comments in bug 278007
Attachment #211310 - Flags: review?(cbiesinger) → review+
Hmm.. yeah, interesting.  I worry a little about having cache entries in an inconsistent state, but otherwise your suggestion sounds good to me.
Priority: -- → P1
*** Bug 278007 has been marked as a duplicate of this bug. ***
QA Contact: tever → networking.http
I went ahead with this patch for now.  If we keep all partial files in the cache, then I think we might end up with a bunch of partial entries taking up space in the cache for no reason.  It makes sense to prune them out up front if we can.

fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #211310 - Flags: approval-branch-1.8.1?(bzbarsky)
Attachment #211310 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
fixed1.8.1
Keywords: fixed1.8.1
Did this cause Bug 329260?
Depends on: 329260
Blocks: 330397
You need to log in before you can comment on or make changes to this bug.