nsIHttpNotify OnExamineResponse called with bad response when content cached

RESOLVED FIXED in mozilla1.6beta

Status

()

Core
Networking: HTTP
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Ryan Gammon, Assigned: Darin Fisher)

Tracking

Trunk
mozilla1.6beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030312

When dealing with a cached document, the OnExamineResponse method of a
nsIHttpNotify handler is called before mResponseHead has been properly
initialized from the cache. 

OnExamineResponse is called early on in nsHttpChannel::ProcessResponse, but the
cached headers aren't merged into the mResponseHead until later, in
nsHttpChannel::ProcessNotModified.

I'm willing to submit a patch this is verified as a bug.

Reproducible: Always

Steps to Reproduce:
1. Download forcecontenttype mozdev component
2. Try to force the content type of a cached document
3. Content-type is not set when OnExamineResponse is called

Actual Results:  
The Content-Type header isn't set when OnExamineResponse is called.

Expected Results:  
The Content-Type should be set to the value originally indicated by the
webserver (cached content doesn't reflect changes made by calling SetContentType
after the response is initially served -- a good thing, IMO, but worth noting).

Comment 1

14 years ago
Not a whole lots seems to be happening with this bug even though a patch has
been offered. Maybe if it actually gets submitted for review it will speed
things up a bit? I'd really like to see this get fixed personally...
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.5beta

Comment 2

14 years ago
please submit the patch. This way the engineer will read the bug. Otherwise it's
like that he/she wont. The engineers are flooded with bugs.
(Reporter)

Comment 3

14 years ago
Created attachment 127881 [details] [diff] [review]
Proposed patch sets headers from cache before calling OnExamineResponse

I've attached a patch that fixes this bug. 

For those running debian-unstable linux, you can download a new libnecko.so
from
http://www.ece.uwaterloo.ca/~rggammon/libnecko.so and drop it into
/usr/lib/mozilla/components

Sorry to hear about the changes at AOL.

Comment 4

14 years ago
you should request review from darin@meer.net (module owner) if you'd like him
to look at the patch.
(Reporter)

Comment 5

14 years ago
Comment on attachment 127881 [details] [diff] [review]
Proposed patch sets headers from cache before calling OnExamineResponse

This patch is against 1.4
Attachment #127881 - Flags: review?(darin)
(Assignee)

Comment 6

14 years ago
hmm... i'm not sure about this patch.  for example, this patch will break
livehttpheaders (or any other nsIHttpNotify instance that wants to observe the
actual headers sent from the server).  one thing is for sure, i've never been
particularly happy with the nsIHttpNotify mechanism.  perhaps now it is time to
revise it to be more flexible, e.g., perhaps we need to have notifications
before and after headers are merged.

this patch does make things more sane i agree, since the headers available when
OnExamineResponse is called correspond to those that are available when
OnStartRequest is called.  but, i'm concerned about changing the behavior of
this notification since it will break livehttpheaders :-(
(Assignee)

Comment 7

14 years ago
Comment on attachment 127881 [details] [diff] [review]
Proposed patch sets headers from cache before calling OnExamineResponse

minusing patch based on previous comments.
Attachment #127881 - Flags: review?(darin) → review-
(Reporter)

Comment 8

14 years ago
"break" is kinda a harsh word -- livehttpheaders would still work, it would just
be reporting a mishmash of headers pulled both off the wire, and from the cache.

How about a patch that changes the way the Content-Type header is merged (IE -
if it's set in the response [by me w/ SetContentType], let that override the
cached response)? More of a hack, but doesn't change the behavior of
livehttpheaders.

Anyway, if you can provide some direction here, I'd be glad to implement an
appropriate patch. 

I'll point the people on the livehttpheaders at this bug too, in case they have
any comments.
(Assignee)

Comment 9

14 years ago
>How about a patch that changes the way the Content-Type header is merged (IE -
>if it's set in the response [by me w/ SetContentType], let that override the
>cached response)? More of a hack, but doesn't change the behavior of
>livehttpheaders.

yeah, something like that might be the simplest thing to do... let me think
about it some :)
(Assignee)

Updated

14 years ago
Target Milestone: mozilla1.5beta → mozilla1.6alpha
(Assignee)

Comment 10

14 years ago
ok, i think i have a good solution for this bug.  once the patch for bug 217766
lands, we will be able to easily introduce new notification events, and we can
add an event that occurs before and after the headers are merged.  that will
allows folks to listen for the event that they care about.
Depends on: 217766

Comment 11

14 years ago
Well, Darin, bug 217766 has landed.  Ryan, can you make a new patch against the
trunk?
(Assignee)

Comment 12

14 years ago
note: we can add an additional event to necko that can be used to inspect the
response headers before they are merged with the cached response headers.

Comment 13

14 years ago
Ryan: any update here?
could you make a new patch?
(Assignee)

Comment 14

14 years ago
-> future

if you want more notifications in HTTP land, please speak up.
Target Milestone: mozilla1.6alpha → Future

Comment 15

14 years ago
I'd like this fixed because the ForceContentType extension needs it.  
(Reporter)

Comment 16

14 years ago
Created attachment 134750 [details] [diff] [review]
Adds an examine response after merged notification

See the attached patch. ForceContentType is working perfectly with this sort of
notification.
(Reporter)

Updated

14 years ago
Attachment #127881 - Attachment is obsolete: true

Updated

14 years ago
Attachment #134750 - Flags: review?(darin)
(Assignee)

Updated

14 years ago
Target Milestone: Future → mozilla1.6beta
(Assignee)

Comment 17

14 years ago
Comment on attachment 134750 [details] [diff] [review]
Adds an examine response after merged notification

r=darin with some changes made to my local tree.
Attachment #134750 - Flags: review?(darin) → review+
(Assignee)

Comment 18

14 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.