Closed Bug 212650 Opened 19 years ago Closed 18 years ago

View source on error page reloads page

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.8alpha3

People

(Reporter: dmartensson, Assigned: bzbarsky)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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.

*** This bug has been marked as a duplicate of 166786 ***
Status: UNCONFIRMED → RESOLVED
Closed: 19 years ago
Resolution: --- → DUPLICATE
Reopening; this is a separate problem.
Status: RESOLVED → UNCONFIRMED
OS: Windows 2000 → All
Hardware: PC → All
Resolution: DUPLICATE → ---
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?
Assignee: doron → darin
Status: UNCONFIRMED → NEW
Component: ViewSource → Networking: HTTP
Ever confirmed: true
QA Contact: pmac → httpqa
Summary: View source on page with no cache flags reloads page → View source on error page reloads page
this shouldn't be very difficult to fix.  taking for 1.5 beta.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.5beta
Sure, i'll leave the testcase on the server.

If you need any changes to it just let me know.
Target Milestone: mozilla1.5beta → mozilla1.6alpha
i'll try for 1.6 beta, but no promises.
Keywords: helpwanted
Target Milestone: mozilla1.6alpha → mozilla1.6beta
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.
Target Milestone: mozilla1.6beta → mozilla1.7alpha
Target Milestone: mozilla1.7alpha → mozilla1.7beta
Severity: normal → minor
Priority: -- → P5
Target Milestone: mozilla1.7beta → mozilla1.8alpha
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.
Priority: P5 → --
Target Milestone: mozilla1.8alpha1 → Future
Whiteboard: [good first bug]
Attached patch Like so? (obsolete) — Splinter Review
Comment on attachment 153068 [details] [diff] [review]
Like so?

darin, what do you think?
Attachment #153068 - Flags: superreview?(darin)
Attachment #153068 - Flags: review?(darin)
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.
Attached patch So more like this? (obsolete) — Splinter Review
Attachment #153068 - Attachment is obsolete: true
Attachment #153068 - Flags: superreview?(darin)
Attachment #153068 - Flags: review?(darin)
Attachment #153080 - Flags: superreview?(darin)
Attachment #153080 - Flags: review?(darin)
Er, make that !=2 && !=3 in the patch, I think.  We do want to allow caching of
redirects.
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?
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 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.
Attachment #153080 - Flags: superreview?(darin)
Attachment #153080 - Flags: superreview-
Attachment #153080 - Flags: review?(darin)
Attachment #153080 - Flags: review-
Attached patch Address commentsSplinter Review
Attachment #153080 - Attachment is obsolete: true
Comment on attachment 155398 [details] [diff] [review]
Address comments

Darin, how's this look?
Attachment #155398 - Flags: superreview?(darin)
Attachment #155398 - Flags: review?(darin)
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 on attachment 155398 [details] [diff] [review]
Address comments

does the compiler optimize away all of the |return PR_TRUE| cases?
Attachment #155398 - Flags: superreview?(darin)
Attachment #155398 - Flags: superreview+
Attachment #155398 - Flags: review?(darin)
Attachment #155398 - Flags: review+
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.
Assignee: darin → bzbarsky
Status: ASSIGNED → NEW
Keywords: helpwanted
Whiteboard: [good first bug]
Target Milestone: Future → mozilla1.8alpha3
Er.. I did check this in on August 9.  So this is long fixed.
Status: NEW → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.