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).
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...
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.5beta
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.
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.
you should request review from email@example.com (module owner) if you'd like him to look at the patch.
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)
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 :-(
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-
"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.
>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 :)
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
Well, Darin, bug 217766 has landed. Ryan, can you make a new patch against the trunk?
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.
Ryan: any update here? could you make a new patch?
-> future if you want more notifications in HTTP land, please speak up.
Target Milestone: mozilla1.6alpha → Future
I'd like this fixed because the ForceContentType extension needs it.
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.
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+
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.