Bug 53648 – REGRESSION (r77355): Page cache layout tests crash

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Mihai Parparita]

fast/events/pageshow-pagehide-on-back-cached.html and fast/history/timed-refresh-in-cached-frame.html crash after r77355 with this assert:

ASSERTION FAILED: !m_suspended
(/Volumes/Data/WebKit-BuildSlave/snowleopard-intel-leaks/build/Source/WebCore/page/SuspendableTimer.cpp:62 virtual void WebCore::SuspendableTimer::suspend(WebCore::ActiveDOMObject::ReasonForSuspension))

[Mihai Parparita]

+Alexey for ActiveDOMObject
+Brady for page cache
+Jeremy for IndexedDB since it's also looking to use ActiveDOMObject
+James since we just talked about this
+Dmitry since he asked about this bug in #webkit

This assert happens because those test show an alert in a pagehide handler. In CachedFrame::CachedFrame (http://trac.webkit.org/browser/trunk/Source/WebCore/history/CachedFrame.cpp#L125) we first suspend ActiveDOMObjects (line 139) and then invoke pagehide handlers (line 148). Therefore the SuspendableTimer that's associated with the EventQueue is suspended once by CachedFrame and then again when the modal dialog is shown.

This problem existed before r77355, it's just that it was harder to trigger before (now there's a SuspendableTimer associated with every Document). Here's another way in which it gets triggered: http://persistent.info/webkit/test-cases/webkit-53648/timeout-pagehide-alert.html (that creates a DOMTimer (which subclasses SuspendableTimer) via setTimeout, and then navigates away and shows an alert in the pagehide handler).

I also would have expected this to happen when nesting modal dialogs (by having showModalDialog call alert), but that doesn't seem to happen (tests in http://persistent.info/webkit/test-cases/webkit-53648/nested-dialogs.html and http://persistent.info/webkit/test-cases/webkit-53648/nested-dialogs-2.html).

There's a few ways of fixing this:

1. Re-order the suspendActiveDOMObjects and stopLoading calls in CachedFrame. Brady, is there a reason why we suspend active DOM objects before triggering more JS execution?

2. Handle this nesting at the SuspendableTimer level. However, other ActiveDOMObject subclasses may be suspendable to this problem too.

3. Handle this nesting at the ActiveDOMObject or ScriptExecutionContext level, so that all classes get sane default nesting behavior. However, some ActiveDOMObjects (e.g. HTMLMediaElement) don't actually suspend/resume in some cases (e.g. for modal dialogs) so they may not want this.

[Alexey Proskuryakov]

> we first suspend ActiveDOMObjects (line 139) and then invoke pagehide handlers (line 148).

This sounds pretty terrible - it means that active objects added by pagehide handlers won't be suspended, right?

[James Robinson]

(In reply to comment #2)
> > we first suspend ActiveDOMObjects (line 139) and then invoke pagehide handlers (line 148).
> 
> This sounds pretty terrible - it means that active objects added by pagehide handlers won't be suspended, right?

Yeah that does sound bad - sounds like we should probably do the suspend-for-inactive call _after_ firing pagehide.

[Mihai Parparita]

(In reply to comment #2)
> > we first suspend ActiveDOMObjects (line 139) and then invoke pagehide handlers (line 148).
> 
> This sounds pretty terrible - it means that active objects added by pagehide handlers won't be suspended, right?

Yes, see test case at http://persistent.info/webkit/test-cases/webkit-53648/timeout-pagehide.html (the alert that fires was created by the previous page by setting a timeout  in the pagehide handler).

I'll try re-ordering the calls in CachedFrame and see if anything breaks.

[Mihai Parparita]

Created attachment 81113 [details]
Patch

[Mihai Parparita]

(In reply to comment #4)
> I'll try re-ordering the calls in CachedFrame and see if anything breaks.

Nothing broke, except for bug 53716 (which is a coincidence, since the patch adds a test that causes recorded-keydown-event.html and tabindex-focus-blur-all.html in fast/events to get run in the same DRT session).

[Adam Roben (:aroben)]

Unsurprisingly, this is also happening on Windows: http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r77534%20(24887)/CrashLog_0e24_2011-02-03_14-55-43-359.txt

[Alexey Proskuryakov]

Comment on attachment 81113 [details]
Patch

This seems fine, but doesn't fix a more general case with multiple frames calling into each other from pagehide.

[Mihai Parparita]

(In reply to comment #8)
> (From update of attachment 81113 [details])
> This seems fine, but doesn't fix a more general case with multiple frames calling into each other from pagehide.

Filed bug 53733 about this.

[Mihai Parparita]

Committed r77559: <http://trac.webkit.org/changeset/77559>

[WebKit Review Bot]

http://trac.webkit.org/changeset/77559 might have broken GTK Linux 32-bit Release, GTK Linux 32-bit Debug, and GTK Linux 64-bit Debug
The following tests are not passing:
fast/events/pagehide-timeout.html

[Mihai Parparita]

(In reply to comment #11)
> http://trac.webkit.org/changeset/77559 might have broken GTK Linux 32-bit Release, GTK Linux 32-bit Debug, and GTK Linux 64-bit Debug
> The following tests are not passing:
> fast/events/pagehide-timeout.html

Filed bug 53771 about this.