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

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Shayne Jewers]

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

Comment 1

[Shayne Jewers]

Attachment: sscreenshot

should also note, some people say this also happens when logging in and logging
out

Comment 2

[Ria Klaassen (not reading all bugmail)]

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
 

Comment 3

[Martijn Wargers (dead)]

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

[Brett Wilson]

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

Comment 5

[Brian Ryner (not reading)]

targetting fix for beta

Comment 6

[Ria Klaassen (not reading all bugmail)]

Regressed between ID 2005062806 and ID 2005062823.

Comment 7

[Darin Fisher]

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

Comment 8

[Brian Ryner (not reading)]

darin graciously offered to take this.

Comment 9

[Darin Fisher]

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

Comment 10

[Darin Fisher]

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

Comment 11

[Darin Fisher]

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

Comment 12

[Darin Fisher]

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 13

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 14

[Darin Fisher]

> +	     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 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 192315 [details] [diff] [review]
v1 patch

ok, r=biesi with swap() used

Comment 16

[Darin Fisher]

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

Comment 17

[Mike Schroepfer]

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.

Comment 18

[Darin Fisher]

fixed-on-trunk

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.