Closed Bug 90288 Opened 23 years ago Closed 23 years ago

not honoring "Pragma: no-cache" from HTTP-EQUIV

Categories

(Core :: Networking: HTTP, defect, P1)

x86
Windows NT
defect

Tracking

()

RESOLVED FIXED
mozilla0.9.2

People

(Reporter: gagan, Assigned: darin.moz)

References

Details

(Keywords: topembed, Whiteboard: r=gagan, sr=dougt)

Attachments

(8 files)

fell out of investigation of bug 82418 and several other symptoms that lead to 
this bug...

Steps to reproduce-- put <META HTTP-EQUIV="Pragma" CONTENT="no-cache"> in an 
HTML file and watch it be cached and reused :-(
I've marked this bug pdt+ after today's discussion in PDT.  NOTE: This bug makes
explicit mention of us not respecting HTTP-EQUIV level information. HOWEVER,
look at the HTTP headers placed below. In addition to not respecting HTTP-EQUIV,
we're also not respecting HTTP headers. (HTTP-EQUIV goes with META, but before
we get to the actual text/html, we ought to respect cache directives from
headers).

Here are more comments, along with HTTP headers: 

"We are experiencing an anomaly with regard to Netscape 6 handling of SSL
content, regardless of Edit-Preferences-Advanced-Cache setting of:
Automatically, Once per session, Every time I view the page.

In NN 4.7x, when we <BACK> our way to a form page (like the sign on page)
the re-displayed page is empty of values in the text input boxes.

In Netscape 6.0x and Netscape 6.1 beta, when we <BACK> our way to a form
page (like the sign on page) the re-displayed page has the inputs of the
text boxes filled in as they were input earlier. The password field is still
masked, but the behavior is obviously very different from ANY previous
Netscape release or any other browser we are familiar with.

Is this a known bug?

How is cache control handled in Netscape 6?

We are emitting these types of HTTP headers:

Refresh: 1;
URL=https://moose.wellsfargo.com/~jaschun/production_jaschun/cgi-bin/session
.cgi?sessargs=09a247c9a63d4d95222c9cb4714ddba91753a27379a71f3d8a9eefef2c00d4
8a
Pragma: no-cache
Expires: Mon, 01 Jan 1990 00:00:00 GMT
Content-type: text/html
Whiteboard: pdt+
Blocks: 82418
I am inclined to say that the HTTP's headers issue of Pragma no-cache is a
different bug but I will continue to investigate if this patch that I am
attaching fixes the problem (I can't connect to internal cvsserver to confirm on
commercial as yet) but I will do that as soon as I can. 
No longer blocks: 82418
Lemme explain why I don't want this as the final fix-- I'd really like us to be
able to reuse "pragma: no-cache" pages for mem cache (back buttons) purposes.
What I am doing in this patch is to actually doom the cacheentry as soon as I
see the "pragma: no-cache" This will fix part of the problem BUT will give a
slight hit on the back button performance ONLY with "pragma: no-cache" pages.
Small cost to pay for this urgent a bug... 

Later on we should go back and do the right thing-- i.e. when darin gets back :-)
Forgot to add-- we need to let go of mChannel in the HTMLContentSink at a later
stage too. I'll add that once I can verify the wellsfargo problem.
please also read:
http://bugzilla.mozilla.org/show_bug.cgi?id=65947
which basiclly talks about the same thing and also has fixes attached to it...
That bug (bug 65947) makes explicit mention of password behavior, and may
approach this fix from a different angle.  Here's more information on HTTP
headers w.r.t caching not being respected:
    *  HTTP1.0 cache directives as demonstrated earlier

    nor

    * HTTP1.1 cache directives such as:
          * Cache-Control: no-cache
          * Cache-Control: private
          * Cache-Control: must-revalidate


Whiteboard: pdt+ → pdt+, have short term fix.
I am trying to make sure this bug addresses the wellsfargo problem as well. If 
not I will add patch here to take care of that as well. I know what the fix is 
(at least the short term) ETA tonight possibly.
Is this almost ready to go in?  Need reviews??
yes almost ready :)
Attached patch final fixSplinter Review
Whiteboard: pdt+, have short term fix. → pdt+ need r=, sr=
assuming that the channel lives at least as long as the nsHTMLContentSink, the 
patch looks good. sr/r
after talking to gordon about this, I have some better ideas on how to do 
this-- if the nsHTMLContentSink lives shorter than the http channel then there 
is no reason why we can't just use nsCOMPtr for the channel. 

Additionally we don't have to doom the cache object (something that I had hoped 
all along and only recently understood the way out) This will help us not throw 
away the page (and refetch from server) for all the cases of session-history, 
printing and such... 

Expect a better patch later tonight.
adding harish and gordon to cc list. removing need r/sr
Whiteboard: pdt+ need r=, sr= → pdt+, newer patch expected later tonight
Gagan, is the patch in hand good enough for now?
*** Bug 82418 has been marked as a duplicate of this bug. ***
I've checked in the patch on bug 65947, per Gagan's approval, which addresses
the security aspect of this problem
need r,sr...
Whiteboard: pdt+, newer patch expected later tonight → pdt+, need r=,sr=
got r=gordon and sr=dougt, checking it in now... 
Whiteboard: pdt+, need r=,sr= → pdt+
fix checked into the MOZILLA_0_9_2_BRANCH. Will check it into the trunk later 
tonight... 
Whiteboard: pdt+ → pdt+, checked into the branch
(from lchiang - ben or bbaetz - pls verify fix on branch)
Keywords: vbranch
I'll take this for verification. I disagree with the fix, but I'll argue with
darin next week... Also see bug 86264.
QA Contact: benc → bbaetz
VERIFIED 2001071908 on linux, windows, and mac, using http://g/test-cookie.html

Removing vbranch keyword.
Keywords: vbranch
err, 2001071904-0.9.2, rather, not 08.
These changes caused bug 91543.
Bug 91538 too.  What are the benefits of this fix when compared to other
possible regressions this may have caused, that we won't find until after
release...?
this has now been backed out of the branch. 
Whiteboard: pdt+, checked into the branch → pdt+
taking pdt+ off. gagan will come back if he gets something good together for
this one in the next few days.
Whiteboard: pdt+
It looks like the patch that was checked into the branch caused bug #91710.
hey gagan,
this patch is *nasty* ;-)

it seems like the nsIHttpChannel::SetEquivHeader(...) method is just a "back
door" which could cause problems since response headers are changed whitout
notifying any listeners (interested in HTTP headers)

there must be a cleaner way to process <meta http-equiv ...> headers...
actually this patch does not address that issue as yet-- I only intended to 
handle the pragma: no-cache issues. And I agree that a complete implementation 
of SetEquivHeader should turn around and notify observers correctly but then 
currently in the branch the HTMLContentSink also independently handles a bunch 
of headers on its own. (which should really just be called via 
ProcessHTTPHeaders) I wasn't too motivated to make that big a cleanup effort on 
the branch at the last minute (which could have added other risks) and so I 
restricted the usage of SetEquivHeader (see its implementation) to only 
understand Pragma: no-cache or Cache-Control: no-[cach|stor]e

Do you have a suggestion for a better solution to report HTTP-Equiv headers back 
onto the channel?
Blocks: 83465
adding myself to cc-list
No longer blocks: 83465
marking PDT+ for the specific fix which darin mentioned to PDT today for
checking into the branch.
Whiteboard: [PDT+]
lisa: i've opened bug 92598 to track my patch, since my patch doesn't completely
solve this problem.
Whiteboard: [PDT+]
-> me
Assignee: gagan → darin
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.4
adding topembed to the bug
Keywords: topembed
these are a couple of preliminary patches which make things work.  there are
still some issues with the http changes that i'll need to sort out next week.
this final http patch has the following changes:

1) block setting certain response headers which shouldn't be set via HTTP-EQUIV
   including:
     Content-Type
     Content-Length
     Content-Encoding
     Trailer
     Transfer-Encoding

2) don't save HTTP-EQUIV response headers in the cache along with normal headers
   since it is unnecessary... because "Pragma: no-cache" for example simply 
   implies an expiration time in the past.
Keywords: patch
am glad that as per our discussion you are blocking some of these headers off. 
I feel much comfortable now. And I agree that we don't need to save these in the 
cache since the way the META tag parsing works it will kick in again for the 
cache load as well.

On the review note-- just curious on the reasoning behind doing a static (and 
then a temporary variable assignment) for blocked_headers. Wouldn't this be 
faster/smaller? --

if ((atom == nsHttp::Content_Type) ||
  (atom == nsHttp::Content_Length) || ... ) // and so on
       return NS_ERROR_ILLEGAL_VALUE;

I'd recommend the change above and with that r=gagan

quick sanity pause here. "technically" (via loosely defined spec), HTTP-EQUIV
can specific *any* HTTP header. why have we gone down the explicit blocking path?
jud: it just doesn't make any sense to change certain headers.  take
Content-Type for example: how can the Content-Type change?  how can the
Content-Length change?  how can the Content-Encoding change?  etc...

gagan: thanks for the suggestion
Whiteboard: r=gagan, sr=?
sr=me.
Whiteboard: r=gagan, sr=? → r=gagan, sr=dougt
fixed-on-trunk
Whiteboard: r=gagan, sr=dougt → r=gagan, sr=dougt, fixed-on-trunk
Target Milestone: mozilla0.9.4 → mozilla0.9.2
my patch resulted in a crash regression within the nsHTMLContentSink.cpp (see
bug 96440).  this patch should not be accepted on the 0.9.2 branch until that
bug is fixed.
I'll wait for the crasher to be fixed before verifying this.
Darin: you'd need to ensure that mParser is valid before using it. I had made
the same mistake and then I corrected it on-the-fly without submitting a newer
patch since it was crashing at a lot of places. If you need more details on
this-- check the previous versions of nsHTMLContentSink v.3.464.2.6

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/content/html/document/src&command=DIFF_FRAMESET&file=nsHTMLContentSink.cpp&rev1=3.464.2.5&rev2=3.464.2.6&root=/cvsroot

or/additionally talk to harishd.
gagan: i have a patch in bug 96440 which adds the null check, but harish tells
me that 1) it shouldn't be necessary and 2) he knows why and how to fix it for
real.  i'll try to land my work around patch today, but the real fix should go
in eventually.
QA -> benc
QA Contact: bbaetz → benc
benc@netscape.com, could you verify the bug on the trunk build before chris
will check this into the 0.9.2 branch? thanks

fixed-on-branch
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
verified on trunk:

WinNT4 2001082803
Mac os9 2001082908
linux rh6 2001082908
qa -> me
QA Contact: benc → tever
Whiteboard: r=gagan, sr=dougt, fixed-on-trunk → r=gagan, sr=dougt, verified-on-trunk
Is this fixed on the 0.9.4 branch?
yes, this should be fixed on the 0.9.4 branch.
verified:

Win NT4 2001-09-24-05-0.9.4
Status: RESOLVED → VERIFIED
Whiteboard: r=gagan, sr=dougt, verified-on-trunk → r=gagan, sr=dougt
I'm re-opening this bug -- shut me down if necessary.  The patch stops short at
the finish line because we distinguish between "Cache" and "Session History." 
This is a semantic difference that doesn't win any friends in the real world. 
For example, the Communicator 4.x behavior was to not reproduce form values upon
click of "Back" if Pragma: no-cache was specified in the META tag.  Should we
contemplate including Session History for forms in the sweep of things to
forget?  CC'ing Radha. If you thing this should be a fresh bug altogether, I'll
log one -- let me know (I wasn't sure).
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
aruner: a fresh bug would probably be better.  thx!
Darin, OK: Bug 104162 logged.
reclosing
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: