Bug 38910 – PageGroupLoadDeferrer should also defer executeScriptSoonTimer

• Component: Page Loading
• Status: RESOLVED FIXED

[Yong Li]

PageGroupLoadDeferrer suspends all active dom timers and loading jobs. But it doesn't suspend Document::m_executeScriptSoonTimer. In the case a m_executeScriptSoonTimer is already scheduled, and a JS alert window pops up (which is guarded by PageGroupLoadDeferrer), Document::m_executeScriptSoonTimer can still fire and run JS on the same doc, and then can schedule DOM timers on the doc.

I've seen it results ASSERTION failure in DOMTimer when the PageGroupLoadDeferrer destructs. (it resumes dom timers, and every dom timer asserts that it is currently suspended.)

But this problem cannot be reproduced reliably because it depends on timing. It happens only when m_executeScriptSoonTimer has been scheduled by not fired yet before a PageGroupLoadDeferrer is constructed.

The solution is simple: Just suspend all executeScriptSoonTimer timers in PageGroupLoadDeferrer ctor, and resume them in PageGroupLoadDeferrer dtro.

But it's very hard to create a layout test for it. Help needed!!!

[Yong Li]

Created attachment 55707 [details]
the patch

[Yong Li]

Created attachment 55718 [details]
fix merge error

[Yong Li]

Created attachment 57099 [details]
Test case included thanks to Robin!

Thanks to Robin Cao, who created the test case.

This test cast can reproduce the problem reliably.

Robin has verified this problem happens on both Safari for Windows and QtLauncher.

[WebKit Review Bot]

Attachment 57099 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/manual-tests/resources/load-deferrer-script-element.js:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yong Li]

Created attachment 57102 [details]
With enhanced test case

[Darin Adler]

Comment on attachment 57102 [details]
With enhanced test case

Can this work be put into the suspendActiveDOMObjects and resumeActiveDOMObjects function rather than adding two more member functions to Document? I’d prefer to do it that way. It’s OK this way, but I’d prefer not to modify Document.h and PageGroupLoadDeferrer.cpp.

r=me

[George Staikos]

Let's open a second bug report to refactor it.  I think we have a couple of additional similar patches coming.

[Yong Li]

Comment on attachment 57102 [details]
With enhanced test case

I agree they should be merged into one function, but the old function name seems a little bit misleading. PageGroupLoadDeferrer needs to be purified more, and refactoring can be done in next patches. Committing it for now?

[WebKit Commit Bot]

Comment on attachment 57102 [details]
With enhanced test case

Clearing flags on attachment: 57102

Committed r61126: <http://trac.webkit.org/changeset/61126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.