[webkit-reviews] review requested: [Bug 54380] [Qt]tst_QWebPage::modified() asserts : [Attachment 83098] Patch for review

[bugzilla-daemon at webkit.org]

Aparna Nandyal <aparna.nand at wipro.com> has asked  for review:
Bug 54380: [Qt]tst_QWebPage::modified() asserts
https://bugs.webkit.org/show_bug.cgi?id=54380

Attachment 83098: Patch for review
https://bugs.webkit.org/attachment.cgi?id=83098&action=review

------- Additional Comments from Aparna Nandyal <aparna.nand at wipro.com>
Reason for assert:

1. When FrameView's destructor is called, it asserts when m_enqueueEvents is
not 0.
2. This variable is incremented in FrameView::pauseScheduledEvents and
decremented in FrameView::resumeScheduledEvents. 
3. In normal circumstances very time QWebFrame::setUrl is called,
pauseScheduledEvents is called 3 times (once in
d->frame->loader()->activeDocumentLoader()->writer()->begin(absolute) and twice
in d->frame->loader()->activeDocumentLoader()->writer()->end())
3. But if back/forward navigation has been done before current
QWebFrame::setUrl (as in the case of test case),
FrameView::pauseScheduledEvents is called 5 times and
FrameView::resumeScheduledEvents is called 4 times, leaving the
m_enqueueEvents=1 which is causing the assert.
4. Extra calls to FrameView::pauseScheduledEvents is made due to the wrong code
path taken in 
FrameLoader::didFirstLayout which is called from
d->frame->loader()->activeDocumentLoader()->writer()->end()

void FrameLoader::didFirstLayout()
{
    if (m_frame->page() && isBackForwardLoadType(m_loadType))
	history()->restoreScrollPositionAndViewState();
    
    .......
}
The function isBackForwardLoadType() returns true. But it is the previous
operation's m_loadType that is used and the new m_loadType is set only in
FrameLoader::load (which apparently has a "// FIXME: is this the right place to
reset loadType? Perhaps this should be done after loading is finished or
aborted."). But in QWebFrame::setUrl, the damage has been done before the call
to FrameLoader::load as there is a wrong m_loadType.

Ways to fix this:
1. Make sure that m_loadType is set before DocumentWriter's begin and end is
called. Infact as the FIXME suggests, the value should be reset after loading
is completed or aborted. But any changes to those functions seemed risky as
they are called in different scenarios and there wasnt a single ending function
for the operation (several paths were there). Hence put the fix is relatively
safer haven QWebFrame::setUrl. 
2. To fix the test case, a wait for loadFinished signal will do the magic. This
would actually reset the m_enqueueEvents to 0 immaterial of its current value
in FrameView::~FrameView. This fix would Camouflage the root cause. So not a
good idea!