210208 - nsIHttpNotify OnExamineResponse called with bad response when content cached

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Ryan Gammon]

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

[Josh]

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...

Comment 2

[Henrik Gemal]

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.

Comment 3

[Ryan Gammon]

Attachment: 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

[[e-mail]]

you should request review from [e-mail] (module owner) if you'd like him
to look at the patch.

Comment 5

[Ryan Gammon]

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

This patch is against 1.4

Comment 6

[Darin Fisher]

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 7

[Darin Fisher]

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

minusing patch based on previous comments.

Comment 8

[Ryan Gammon]

"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.

Comment 9

[Darin Fisher]

>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 :)

Comment 10

[Darin Fisher]

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.

Comment 11

[alanjstr]

Well, Darin, bug 217766 has landed.  Ryan, can you make a new patch against the
trunk?

Comment 12

[Darin Fisher]

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

[Henrik Gemal]

Ryan: any update here?
could you make a new patch?

Comment 14

[Darin Fisher]

-> future

if you want more notifications in HTTP land, please speak up.

Comment 15

[alanjstr]

I'd like this fixed because the ForceContentType extension needs it.  

Comment 16

[Ryan Gammon]

Attachment: Adds an examine response after merged notification

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

Comment 17

[Darin Fisher]

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

r=darin with some changes made to my local tree.

Comment 18

[Darin Fisher]

fixed-on-trunk