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

Status: RESOLVED FIXED

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

Description

[Gagan]

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

Comment 1

[Arun Ranganathan]

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

Comment 2

[Gagan]

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. 

Comment 3

[Gagan]

Attachment: a potential quick fix (not the final fix)

Comment 4

[Gagan]

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

Comment 5

[Gagan]

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.

Comment 6

[Henrik Gemal]

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

Comment 7

[Arun Ranganathan]

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

Comment 8

[Gagan]

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.

Comment 9

[rubydoo123]

Is this almost ready to go in?  Need reviews??

Comment 10

[Gagan]

yes almost ready :)

Comment 11

[Gagan]

Attachment: final fix

Comment 12

[Doug Turner (:dougt)]

assuming that the channel lives at least as long as the nsHTMLContentSink, the 
patch looks good. sr/r

Comment 13

[Gagan]

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.

Comment 14

[Gagan]

adding harish and gordon to cc list. removing need r/sr

Comment 15

[selmer (gone)]

Gagan, is the patch in hand good enough for now?

Comment 16

[harishd]

Attachment: patch v1.2 [ parser changes ]

Comment 17

[Gagan]

*** Bug 82418 has been marked as a duplicate of this bug. ***

Comment 18

[Eric Pollmann]

I've checked in the patch on bug 65947, per Gagan's approval, which addresses
the security aspect of this problem

Comment 19

[Gagan]

Attachment: yet another final patch :-)

Comment 20

[Gagan]

need r,sr...

Comment 21

[Gagan]

got r=gordon and sr=dougt, checking it in now... 

Comment 22

[Gagan]

fix checked into the MOZILLA_0_9_2_BRANCH. Will check it into the trunk later 
tonight... 

Comment 23

[selmer (gone)]

(from lchiang - ben or bbaetz - pls verify fix on branch)

Comment 24

[Bradley Baetz (:bbaetz)]

I'll take this for verification. I disagree with the fix, but I'll argue with
darin next week... Also see bug 86264.

Comment 25

[Bradley Baetz (:bbaetz)]

VERIFIED 2001071908 on linux, windows, and mac, using http://g/test-cookie.html

Removing vbranch keyword.

Comment 26

[Bradley Baetz (:bbaetz)]

err, 2001071904-0.9.2, rather, not 08.

Comment 27

[Chris Waterson]

These changes caused bug 91543.

Comment 28

[Blake Ross]

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

Comment 29

[Gagan]

this has now been backed out of the branch. 

Comment 30

[chris hofmann]

taking pdt+ off. gagan will come back if he gets something good together for
this one in the next few days.

Comment 31

[rpotts (gone)]

It looks like the patch that was checked into the branch caused bug #91710.

Comment 32

[rpotts (gone)]

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

Comment 33

[Gagan]

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?

Comment 34

[Paul Chen]

adding myself to cc-list

Comment 35

[lchiang]

marking PDT+ for the specific fix which darin mentioned to PDT today for
checking into the branch.

Comment 36

[Darin Fisher]

lisa: i've opened bug 92598 to track my patch, since my patch doesn't completely
solve this problem.

Comment 37

[Darin Fisher]

-> me

Comment 38

[Marek Z. Jeziorek]

adding topembed to the bug

Comment 39

[Darin Fisher]

Attachment: v2.0 htmlparser/content changes

Comment 40

[Darin Fisher]

Attachment: v2.0 http changes

Comment 41

[Darin Fisher]

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.

Comment 42

[Darin Fisher]

Attachment: v2.1 http changes

Comment 43

[Darin Fisher]

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.

Comment 44

[Gagan]

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

Comment 45

[Judson Valeski]

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?

Comment 46

[Darin Fisher]

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

Comment 47

[Darin Fisher]

Attachment: v2.2 http changes

Comment 48

[Doug Turner (:dougt)]

sr=me.

Comment 49

[Darin Fisher]

fixed-on-trunk

Comment 50

[Darin Fisher]

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.

Comment 51

[Bradley Baetz (:bbaetz)]

I'll wait for the crasher to be fixed before verifying this.

Comment 52

[Gagan]

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.

Comment 53

[Darin Fisher]

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.

Comment 54

[Bradley Baetz (:bbaetz)]

QA -> benc

Comment 55

[Marek Z. Jeziorek]

[e-mail], could you verify the bug on the trunk build before chris
will check this into the 0.9.2 branch? thanks

Comment 56

[Darin Fisher]

fixed-on-branch

Comment 57

[Tom Everingham]

verified on trunk:

WinNT4 2001082803
Mac os9 2001082908
linux rh6 2001082908

Comment 58

[Tom Everingham]

qa -> me

Comment 59

[Ken]

Is this fixed on the 0.9.4 branch?

Comment 60

[Darin Fisher]

yes, this should be fixed on the 0.9.4 branch.

Comment 61

[Tom Everingham]

verified:

Win NT4 2001-09-24-05-0.9.4

Comment 62

[Arun Ranganathan]

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

Comment 63

[Darin Fisher]

aruner: a fresh bug would probably be better.  thx!

Comment 64

[Arun Ranganathan]

Darin, OK: Bug 104162 logged.

Comment 65

[Bradley Baetz (:bbaetz)]

reclosing