Bug 28683

hmm, we do ActiveDOMObject::restore() twice on going back :-) Fixing...

Created attachment 38572 [details] Proposed patch A fix and a new test.

Comment on attachment 38572 [details] Proposed patch +PASS with 206 ms delay. Will cause this to fail on some systems. That should say "PASS delay was at least 200ms" or similar.

I'd really like to take a full look at this before someone else r+'s. I'm looking at it now.

Comment on attachment 38572 [details] Proposed patch +#ifndef NDEBUG + , m_suspended(false) +#endif Seems like you should guard this with #if !ASSERT_DISABLED instead of the NDEBUG check, just in case some crazy person comes along and turns on assertions on a Release build, or turns them off on a Debug build.

Does the delay during the test need to be as large as 200 ms? Ideally we'd like our regression tests to be much quicker than that.

Comment on attachment 38572 [details] Proposed patch r=me with minor tweaks - prepare for some extreme nit-pickyness ;) > +2009-08-25 Dmitry Titov <dimich@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Timers from cached pages fire instantly rather than the specified timeout delay "...rather than after the specified timeout delay," maybe. > +This test verifies that when page is loaded from the page cache on navigation back, the suspended timers are resumed for a duration left when they were suspended. This is a test for https://bugs.webkit.org/show_bug.cgi?id=28683. > +The test navigates to a page, starts a timer and then navigates to another page and back. It then measures time when the timer is actually fired and makes sure that it is at least the time set at the beginning. If successful, it outputs 'PASS' below. > +PASS with 206 ms delay. Eric is dead on here - the delay will not always be exactly 206 ms, even on the same machine over multiple runs. Also, I think it is possible that a run of the test could hit 200ms exactly. Just "is equal to or greater than 200ms" would be fine. > + (WebCore::CachedPage::restore): > + remove duplicated Frame::restore() call, it should be done only once in FraeLoader::open(cachedFrame) Typo "FrameLoader" > + * page/DOMTimer.cpp: added a debug-only flag and ASSERT to catch out-of-order suspense/restore. > + (WebCore::DOMTimer::DOMTimer): ditto. > + (WebCore::DOMTimer::suspend): ditto. > + (WebCore::DOMTimer::resume): ditto. > + * page/DOMTimer.h: ditto. > + You capitalized all statements in the LayoutTest ChangeLog - should capitalize all the WebCore/ChangeLog stuff, too.

Definitely account for Mark's and Adam's comments, as well. I agree with Mark in principle that a 200ms delay is a tad high, but I also don't know a way to determine what the *smallest* delay possible to still reliably pass the test is.

Although there's clearly a bug in code, it's not clear to me what the right behavior is. If we start a 1 second timer and suspend the page for 5 seconds, I'd personally expect it to fire immediately after resuming. As Ada said, testing other browsers is the key.

That great point completely slipped my mind Alexey. I think this patch is inarguably an improvement over current behavior, but exploring what other browsers do and matching them is also important. Removing my r+ for now. If other browsers match this patches behavior, I'll restore the r+. But if other browsers fire a 1 seconds timer immediately after returning to a page after being away for 5 seconds, the patch needs reworking.

btw, the test I attached initially is from a different bug (28659). It's from investigating that bug that I found this timer issue. The delay was there so that the window resize to (300, 300) can cause a relayout before navigating to the data url. But the window resizing is not the main issue here, and we may be able to rewrite this test without the delay.

Created attachment 38582 [details] Test page to find out what FF is doing. This is a modified/simplified page that works in both Safari and FF (for some reason, history.back() does not work in FF immediately from onload, needs setTimeout). The page starts 1000ms timer, then navigates away, sits on another page for 500ms and then goes back - and this results in timer firing in ~1500ms from initialization. So FF 'freezes' the timers. If there are no significant arguments, we probably should keep it similar. This is how current patch behaves. Will upload updated patch in a few minutes.

Created attachment 38584 [details] Updated patch. (In reply to comment #3) > (From update of attachment 38572 [details]) > +PASS with 206 ms delay. > Will cause this to fail on some systems. Replaced with just 'PASS', output real delay only if failed - in case reduced timeout will cause problems it'd be nice to know what it was in a test run. (In reply to comment #5) > (From update of attachment 38572 [details]) > +#ifndef NDEBUG > + , m_suspended(false) > +#endif > > Seems like you should guard this with #if !ASSERT_DISABLED instead of the > NDEBUG check, just in case some crazy person comes along and turns on > assertions on a Release build, or turns them off on a Debug build. Done. (In reply to comment #6) > Does the delay during the test need to be as large as 200 ms? Ideally we'd > like our regression tests to be much quicker than that. Replaced with 100 ms. I measured time needed on my Mac Pro, debug build, in DRT, to navigate to another page and back - it was a bit under 50ms. I figured double this to be sure it fails in most cases. If it doesn't fail on a slow machine it's probably ok since it will fail on retail build for sure. (In reply to comment #7) > (From update of attachment 38572 [details]) > r=me with minor tweaks - prepare for some extreme nit-pickyness ;) > > + Timers from cached pages fire instantly rather than the specified timeout delay > "...rather than after the specified timeout delay," maybe. Done. > > + remove duplicated Frame::restore() call, it should be done only once in FraeLoader::open(cachedFrame) > Typo "FrameLoader" Done. > You capitalized all statements in the LayoutTest ChangeLog - should capitalize > all the WebCore/ChangeLog stuff, too. Done. (In reply to comment #9) > Although there's clearly a bug in code, it's not clear to me what the right > behavior is. If we start a 1 second timer and suspend the page for 5 seconds, > I'd personally expect it to fire immediately after resuming. As Ada said, > testing other browsers is the key. Tested FF3.5, it behaves as this patch. Attached the test page for reference.

Comment on attachment 38584 [details] Updated patch. If it matches Firefox, then it sounds reasonable. Thanks for looking into it in detail! R+

Landed in http://trac.webkit.org/changeset/47772