Closed Bug 1188387 Opened 7 years ago Closed 6 years ago

URI is not loaded in case some failure occurs in nsHttpChannel::ProcessNotModified

Categories

(Core :: Networking: Cache, defect)

Product:
Core
Component:
Networking: Cache
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox42 --- affected
firefox50 --- fixed

People

(Reporter: michal, Assigned: mayhemer)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 2 obsolete files)

v1.1 [rebased]
4.67 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review 
I hit this problem while testing bug 1177278. If something goes wrong in nsHttpChannel::ProcessNotModified (e.g. CacheEntry::SetMetadataElement returns NS_ERROR_OUT_OF_MEMORY) then nsHttpChannel::ProcessNormal is called and continues like it wasn't a conditional request, so no content is served by the channel. We should handle this in the same way we recover from CacheFile error, i.e. do an internal redirect.
Honza, you know this code better than me, do you want to take the bug?
Flags: needinfo?(honzab.moz)
Yep.  Thanks.
Assignee: nobody → honzab.moz
Flags: needinfo?(honzab.moz)
Attached patch v1 (obsolete) — — Splinter Review 
potential patch, I'll look over it more and then submit rr.
Attached patch v1.1 (obsolete) — — Splinter Review 
Slightly updated.
- problem was that ProcessNotModified could fail even when it just had to be bypassed
- hence I introduced ShouldBypassProcessNotModified() method that does the checks and can be used separately
- when process modified fails, we do the internal redirect via StartRedirectChannelToURI (it correctly handles redirect vetoing/proceeding)
- entry is doomed and threw away


https://treeherder.mozilla.org/#/jobs?repo=try&revision=252d6d76a3f4
Attachment #8640123 - Attachment is obsolete: true
Attachment #8640453 - Flags: review?(michal.novotny)
Michal, ping on a review here. Thanks.
Status: NEW → ASSIGNED
Flags: needinfo?(michal.novotny)
Whiteboard: [necko-active]
Attachment #8640453 - Flags: review?(michal.novotny) → review+
Flags: needinfo?(michal.novotny)
Rebase, retest, land...
Flags: needinfo?(honzab.moz)
Attached patch v1.1 [rebased] — — Splinter Review 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28b551603729
Attachment #8640453 - Attachment is obsolete: true
Flags: needinfo?(honzab.moz)
Attachment #8761145 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d922a8d7ed26
On failure of nsHttpChannel::ProcessNotModified redirect to reload from network, r=michal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d922a8d7ed26
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.