305167 - bfcache stops javascript ticker on bbc homepage

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Mark Perris]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050818 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050818 Firefox/1.6a1

after using the back button (with bfcache) to return to the bbc news homepage
(news.bbc.co.uk), the ticker has stopped.

Reproducible: Always

Steps to Reproduce:
1.visit http://news.bbc.co.uk
2.visit a different site
3.use the back button to return to bbc news

Actual Results:  
the ticker has stopped at the exact point in time where the new page was opened

Expected Results:  
the ticker should have returned to action from the said point in time

Comment 1

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050820
Firefox/1.0+ ID:2005082003
I see this too.

Comment 2

[Mike Schroepfer]

Bryner - looks like this is related to 304860 and bfcache - can you take a look
to figure out what is going on here?

Comment 3

[Brian Ryner (not reading)]

Oops, as of bug 301516 we're no longer restoring timeouts correctly for 
iframes (we're trying to restore them _before_ restoring the docshell tree).  
Fix coming up.

Comment 4

[Brian Ryner (not reading)]

Attachment: patch

I think it's still desirable to restore the Javascript properties of the window
before we dispatch DOMTitleChanged, so I split the restoration of the state and
resuming the timeouts into two separate methods.  We don't resume the timeouts
until after the docshell tree is returned to its original state.

Comment 5

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

Comment on attachment 193757 [details] [diff] [review]
patch

r=biesi (I'm not a super-reviewer)

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 193757 [details] [diff] [review]
patch

sr=jst

Comment 7

[Brian Ryner (not reading)]

checked in on the trunk, leaving open for branch checkin

Comment 8

[Mark Perris]

Thanks for the hard work, but unfortunatly i believe there's still a bug,
although its a little more obscure.

To reproduce:

1: Visit the BBC News homepage (http://news.bbc.co.uk)
2: Visit any link on the page
3: Return to the homepage using the back button
4: Wait until the ticker has typed out to the end of the line
5: Press 'forward', then 'back'
6: The ticker has stopped.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050825
Firefox/1.6a1 (Pacifica Hourly, with the fix above checked in)

Comment 9

[Brian Ryner (not reading)]

Confirmed, it's not 100% reproduceable for me but I do see the problem.

Comment 10

[Brian Ryner (not reading)]

Did some more investigation... it seems that the timer is getting restored, 
but with a wildly incorrect unit of time remaining.  If you let it sit for say 
3 minutes, the ticker will resume.  Obviously not correct... and the problem 
seems to be in the calculation of time-remaining when we suspend the timer in 
the first place.

Comment 11

[Brian Ryner (not reading)]

Attachment: additional patch

This is needed in addition to the first set of changes.

The root problem here is that we weren't converting PRIntervalTime units to
milliseconds, and on Windows at least they can be different... so the timer was
restored for much later than it should have been.  While figuring all of this
out, I found that we keep nsTimeout::mWhen as a PRInt64, which is _very_
confusing given that it only ever stores PRIntervalTime values (32-bit).  I
can't see any reason to keep this as a PRInt64 since it doesn't buy us any
useful overflow protection... so I changed it to be a PRIntervalTime.

If you think this is risky in some way for 1.5, I can make a version of this
that just fixes the ResumeTimeouts problem.

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 193757 [details] [diff] [review]
patch

Checked in on the branch.  I'm not closing the bug or marking it fixed1.8 until
the second patch is addressed.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 193891 [details] [diff] [review]
additional patch

r+sr=jst

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 193891 [details] [diff] [review]
additional patch

checked in on the trunk, requesting branch approval.

Comment 15

[Mark Perris]

Again, thanks for the hard work on this, I noticed on tinderbox that the fix for
the ticker's very long end of line pauses has been checked in, and I can confirm
it works as expected on the latest trunk build, however, it seems the original
bug has arisen again, navigating forwards or backwards when the ticker has
half-printed its line causes the ticker to stop.

if anyone can confirm, feel free to re-open.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050831
Firefox/1.6a1

Comment 16

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050830
Firefox/1.0+ ID:2005083023
this was checked in on branch 2005-08-26 16:05
verified fixed on branch
kwd->fixed1.8

Comment 17

[Brian Ryner (not reading)]

confirmed regression

Comment 18

[Brian Ryner (not reading)]

Attachment: fix a couple of problems with the last patch

There were two problems with the last patch:

1. mWhen is now unsigned, so we need to change the way we cap off mWhen if the
timeout was supposed to have already fired when we suspend it.
2. DOM_MIN_TIMEOUT_VALUE is in milliseconds, so it needs to be compared against
the output of PR_IntervalToMilliseconds, not the input to it.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 194448 [details] [diff] [review]
fix a couple of problems with the last patch

r+sr=jst

Comment 20

[Brian Ryner (not reading)]

checked in on the trunk

Comment 21

[Brian Ryner (not reading)]

Attachment: combined patch for the branch

This combines the last two patches

Comment 22

[Tracy Walker [:tracy]]

verified on trunk with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.9a1) Gecko/20050901 Firefox/1.6a1

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 194468 [details] [diff] [review]
combined patch for the branch

Please file a followup bug to get rid of < (formerly LL_CMP) comparisons of
mWhen to now or deadline.

The TIMER_LESS_THAN macro from xpcom/threads/nsTimerImpl.h and related macros
should be copied or moved to a common place, and the maximum delay constrained
to be < DELAY_INTERVAL_LIMIT.

This allows wrap-safe testing of whether one PRIntervalTime denotes a time
prior to another PRIntervalTime.

/be

Comment 24

[Brian Ryner (not reading)]

Filed bug 306759 on the wrapping.

Comment 25

[Brian Ryner (not reading)]

Checked in all of the patches on the branch (fixed1.8 already there, maybe 
should have been taken off when the bug was reopened).

Comment 26

[Tracy Walker [:tracy]]

verified on Firefox 1.4 -mozilla1.8 branch- Win, Lin and Mac : 2005-09-07