HTTP "refresh" doesn't load page with bfcache enabled

RESOLVED FIXED in mozilla1.8beta4

Status

()

Core
HTML: Parser
P1
major
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Shayne Jewers, Assigned: Darin Fisher)

Tracking

Trunk
mozilla1.8beta4
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [serious bfcache regression] [1.8 Branch ETA 8/9])

Attachments

(2 attachments)

35.32 KB, image/png
Details
5.15 KB, patch
Biesinger
: review+
Brian Ryner (not reading)
: superreview+
Benjamin Smedberg
: approval1.8b4+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050719 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050719 Firefox/1.0+

pages using meta refresh dont seem to load the page and i have to reload the
page for it to come up

Reproducible: Always

Steps to Reproduce:
1) go to www.thedailypos.org/forum/
2) login with username: Name password: password
3) click the profile button
4) select "Forum Profile Information"
5) Click "Change Info"


Actual Results:  
Blank page comes up, though reloading the page makes it come up

Expected Results:  
the pages loads
Blocks: 274784
(Reporter)

Comment 1

13 years ago
Created attachment 189824 [details]
sscreenshot

should also note, some people say this also happens when logging in and logging
out
I see it also happen that way, get twice a blank page.
With browser.sessionhistory.max_viewers to 0  I go to the pages rightaway.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050719
Firefox/1.0+ ID:2005071906
 
Please check when this regressed. This may be the same issue as bug 300944 (so
the regression period should be the same then).

Comment 4

13 years ago
This bug is only triggered when the refresh is in the HTTP header and *not* in a
"meta http-equiv=refresh" in the document itself. Time does not seem to matter.

See testcase
  http://maxradi.us/post/bugzilla/refresh/
which can not be attached for obvious reasons. Here's the CGI:
  echo "refresh: 2;url=foo.html"
  echo
  echo This is the refresh page, it should go away after 2 secs

For comparison, here's a simple meta refresh page which works:
  http://maxradi.us/post/bugzilla/refresh/meta.html
Assignee: parser → bryner
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Meta refresh doesent load page with bfcache enabled → HTTP "refresh" doesen't load page with bfcache enabled
targetting fix for beta
Target Milestone: --- → mozilla1.8beta4
Regressed between ID 2005062806 and ID 2005062823.
Flags: blocking1.8b4?
(Assignee)

Comment 7

13 years ago
There's probably a bug in the suspending and resuming of refresh timers.  HTTP
refresh headers are inserted into the refresh timer list from nsDocument.cpp:
http://lxr.mozilla.org/mozilla/source/content/base/src/nsDocument.cpp#4540
That code runs from nsDocument::StartDocumentLoad.

whereas META refresh timers are inserted from the nsContentSink's call to
nsDocument's SetHeaderData method:
http://lxr.mozilla.org/mozilla/source/content/base/src/nsContentSink.cpp#313

so, it's possible that the docshell would learn about the refresh timers at
different times...

Updated

13 years ago
Flags: blocking1.8b4? → blocking1.8b4+
darin graciously offered to take this.
Assignee: bryner → darin
(Assignee)

Updated

13 years ago
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 9

13 years ago
So, this bug appears to be fixed in my debug CVS build from today.  However, my
trunk build from yesterday does exhibit the problem.  Investigating...
(Assignee)

Comment 10

13 years ago
(In reply to comment #9)
> So, this bug appears to be fixed in my debug CVS build from today.  However, my
> trunk build from yesterday does exhibit the problem.  Investigating...

Whoops.. ignore that.  It helps to have a clean profile that doesn't have a
certain preference set to 0 :-/
Summary: HTTP "refresh" doesen't load page with bfcache enabled → HTTP "refresh" doesn't load page with bfcache enabled

Updated

13 years ago
Whiteboard: [serious bfcache regression]
(Assignee)

Updated

13 years ago
Whiteboard: [serious bfcache regression] → [serious bfcache regression] [1.8 Branch ETA 8/9]
(Assignee)

Updated

13 years ago
Blocks: 300860
(Assignee)

Updated

13 years ago
No longer blocks: 300860
(Assignee)

Comment 11

13 years ago
Created attachment 192315 [details] [diff] [review]
v1 patch

OK, so bryner and I devised a pretty straightforward solution to this bug.  We
realized along the way that nsDocShell::LoadURI ends up canceling all timers,
which means that clicking on a link that results in a download kills timers. 
We found that IE does the same, so this patch doesn't fix that behavior. 
However, when you navigate back to such a page, we will restore the timers
(also similar to how IE works and Moz works w/o fastback).  The trick to this
patch is that we change the cancelation in Stop to a suspend, and we save off
mRefreshURIList.  This allows us to get access to the list inside CaptureState.
 The only remaining trick then is to clear the saved copy of mRefreshURIList
inside CreateContentViewer and its peer RestoreFromHistory.
Attachment #192315 - Flags: superreview?(bryner)
Attachment #192315 - Flags: review?(cbiesinger)
(Assignee)

Comment 12

13 years ago
This patch also makes meta refresh work properly when the user navigates back to
a restored page.  Previously, we were not saving any meta refreshes for the page
when leaving it, and this patch fixes that as well.
Comment on attachment 192315 [details] [diff] [review]
v1 patch

+	     mSavedRefreshURIList = mRefreshURIList;
+	     mRefreshURIList = nsnull;

.swap() ?

So.. if a load ends up in a different docshell (e.g. it is a download), then
shouldn't the timers be resumed?
(Assignee)

Comment 14

13 years ago
> +	     mSavedRefreshURIList = mRefreshURIList;
> +	     mRefreshURIList = nsnull;
> 
> .swap() ?

Yeah, good idea :)


> So.. if a load ends up in a different docshell (e.g. it is a download), then
> shouldn't the timers be resumed?

Yeah, that is what I would think too, but as I commented above that bug exists
today, and moreover it appears to exist in IE as well.  So, I think we can
separate the issues.  We don't need to solve that bug here.
Comment on attachment 192315 [details] [diff] [review]
v1 patch

ok, r=biesi with swap() used
Attachment #192315 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 16

13 years ago
OK.  I'm still nulling out mRefreshURIList after the .swap call though since I
don't trust that mSavedRefreshURIList is null at that point.

Updated

13 years ago
Blocks: 300860

Comment 17

13 years ago
Reminder that we are closing the tree tonight at 11:59PM PDT for branch.  Let us
know if this isn't going to make it.
Attachment #192315 - Flags: superreview?(bryner) → superreview+
(Assignee)

Updated

13 years ago
Attachment #192315 - Flags: approval1.8b4?

Updated

13 years ago
Attachment #192315 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 18

13 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Depends on: 312769
This caused bug 312769 -- when destroying a docshell (window close, subframe,
etc) we call Stop(), and there will be no more content viewer creations. So with
this patch we never broke the timer -> docshell -> supports array -> timer
cycles in those cases.
You need to log in before you can comment on or make changes to this bug.