212650 - View source on error page reloads page

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[David Mårtensson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624

When trying to view source on a page set to no cache, mozilla tries to reload page.

Normaly this should not be to big a problem but, IIS sets no cache on ASP pages
with an error.

This makes it much harder to debug since sometimes the next fetch does not
reproduce the error.

Reproducible: Always

Steps to Reproduce:
1.Try the url
2.Reload the url
3.View source
4.Works fine
5.Enter something in the textfield and submit
6.View source

Actual Results:  
It tries to repost the form

Expected Results:  
It should Not repost the form since that could change the state of the server.

I do not know if this problem existed before but I have not seen it before 1.4
final and since I do all my development on work in mozilla I think I should have
seen it before, but I am not 100% certain.

This will probably only affect developes and I think I can do a workaround for
my pages but still, it annoying to have to rewrite a page to debug it.

I do not whant to switch to IE because I still have to reverify everything in
mozilla.

Comment 1

[Frank Wein [:mcsmurf]]

*** This bug has been marked as a duplicate of 166786 ***

Comment 2

[Boris Zbarsky [:bzbarsky]]

Reopening; this is a separate problem.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Over to darin; the problem here is that the page in question is a 500 error
page, and we don't cache error pages.  Per discussion, we should probably cache
these and expire them immediately, so that back/forward/viewsource work but
reload refetches the right thing.

David, could you please keep the test page up for a bit so we can test possible
fixes?

Comment 4

[Darin Fisher]

this shouldn't be very difficult to fix.  taking for 1.5 beta.

Comment 5

[David Mårtensson]

Sure, i'll leave the testcase on the server.

If you need any changes to it just let me know.

Comment 6

[Darin Fisher]

i'll try for 1.6 beta, but no promises.

Comment 7

[Hixie (not reading bugmail)]

bz says that fixing this should fix a similar problem with HTTP auth pages.

STEPS TO REPRODUCE
   1. Load http://www.sbscpressinfo.org/
   2. Cancel the prompt.
   3. View source

ACTUAL RESULTS
   You are prompted again.

EXPECTED RESULTS
   The source with no prompt.

Comment 8

[Darin Fisher]

no time to work on this now... the solution, i think, is to cache error pages
with zero freshness lifetime.  as a result, they will be accessible from the
cache when loading the page via back/forward, viewsource, or saveas.  however,
normal browsing will always hit the server.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153068 [details] [diff] [review]
Like so?

darin, what do you think?

Comment 11

[Darin Fisher]

Comment on attachment 153068 [details] [diff] [review]
Like so?

actually, i think we want to use an expiration time of 0, which implies that
the document is already expired (0 means the Epoch, 1970).

see nsHttpChannel::UpdateExpirationTime() where we set 0 as the expirationTime
in cases where 'Cache-control: no-cache' is returned from the server, for
example.

i'm also a bit concerned about logic in ProcessNormal that calls
InitCacheEntry.  it calls UpdateExpirationTime.  perhaps the solution here is
to coerce nsHttpResponseHead into returning PR_TRUE for MustValidate().  that
may be better.	in fact, you could probably just use the response code for
that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: So more like this?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Er, make that !=2 && !=3 in the patch, I think.  We do want to allow caching of
redirects.

Comment 14

[Darin Fisher]

yeah, something like that.  we may want to synchronize this filter with the
switch-case in nsHttpChannel::ProcessResponse() to be 100% sure that it doesn't
have unexpected side-effects.  like what about the 2xx responses not covered in
the switch-case.  i'll need to think about this some more, and do some thorough
testing.  what sort of testing have you done?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Synchronizing makes sense.  I can do that.

I've tested that it fixes this bug, but I don't have any error pages on hand
that would allow themselves to be cached in a detectable way...

Comment 16

[Darin Fisher]

Comment on attachment 153080 [details] [diff] [review]
So more like this?

In fact, this is incorrect since some redirects may be cached.	Even a 302 can
be cached if the server sends a Cache-control header that specifies a max-age
or some equivalent header.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Address comments

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155398 [details] [diff] [review]
Address comments

Darin, how's this look?

Comment 19

[David Mårtensson]

My testpage from when I reported the bug still exists, does it require any
changes to test the patch, please just tell me what and I will try to update the
testpage or create a new one.

Comment 20

[Darin Fisher]

Comment on attachment 155398 [details] [diff] [review]
Address comments

does the compiler optimize away all of the |return PR_TRUE| cases?

Comment 21

[Boris Zbarsky [:bzbarsky]]

David, your testpage is one of the things I tested the patch on.  It doesn't
need any changes that I can see.

Darin, the compiler does optimize all that away.

Fix checked in.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Er.. I did check this in on August 9.  So this is long fixed.