Closed Bug 620183 Opened 9 years ago Closed 4 years ago

nsHttpChannel::CheckCache should cache requestedETag (If_Match) the first time

Categories

(Core :: Networking: HTTP, enhancement)

enhancement
Not set

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug)

Details

(Keywords: coverity)

Attachments

(1 file, 1 obsolete file)

1.18 KB, patch
Biesinger
: review-
Details | Diff | Splinter Review
2430 nsHttpChannel::CheckCache()
2608         const char *requestedETag, *cachedETag;
2609         cachedETag = mCachedResponseHead->PeekHeader(nsHttp::ETag);
2610         requestedETag = mRequestHead.PeekHeader(nsHttp::If_Match);
2611         if (cachedETag && (!strncmp(cachedETag, "W/", 2) ||
2612             strcmp(requestedETag, cachedETag))) {
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #498577 - Flags: review?(cbiesinger)
Attachment #498577 - Flags: approval2.0?
This code is inside this if condition:
 
2606     if (!doValidation && mRequestHead.PeekHeader(nsHttp::If_Match) &&
2607         (method == nsHttp::Get || method == nsHttp::Head)) {

so requestedETag can't be null here; we just checked it above!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: crash
Resolution: --- → INVALID
Severity: critical → enhancement
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: [@ nsHttpChannel::CheckCache] doesn't handle null requestedETag → nsHttpChannel::CheckCache should cache requestedETag (If_Match) the first time
Status: REOPENED → ASSIGNED
Attached patch cacheSplinter Review
Attachment #498577 - Attachment is obsolete: true
Attachment #499398 - Flags: review?(cbiesinger)
Attachment #499398 - Flags: approval2.0?
Attachment #498577 - Flags: review?(cbiesinger)
Attachment #498577 - Flags: approval2.0?
Comment on attachment 499398 [details] [diff] [review]
cache

Mass minusing patch approval that don't have high return. Please renominate if this is more important for 2.0 than it appears.
Attachment #499398 - Flags: approval2.0? → approval2.0-
Comment on attachment 499398 [details] [diff] [review]
cache

I find this hard to read. please move the assignment outside the if condition.
Attachment #499398 - Flags: review?(cbiesinger) → review-
Status: ASSIGNED → RESOLVED
Closed: 9 years ago4 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.