Bug 54380

Summary: [Qt]tst_QWebPage::modified() asserts
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WORKSFORME    
Severity: Critical CC: abarth, aparna.nand, cmarcelo, eric, japhet, kling
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
GDB backtrace in debug mode (r78457)
none
Patch for review kling: review-

Csaba Osztrogonác
Reported 2011-02-14 02:01:34 PST
Created attachment 82298 [details] GDB backtrace in debug mode (r78457) ASSERTION FAILED: !m_enqueueEvents ../../../Source/WebCore/page/FrameView.cpp(184) : virtual WebCore::FrameView::~FrameView() GDB backtrace: <attached>
Attachments
GDB backtrace in debug mode (r78457) (7.06 KB, text/plain)
2011-02-14 02:01 PST, Csaba Osztrogonác 		no flags
Details
Patch for review (3.52 KB, patch)
2011-02-20 10:50 PST, Aparna Nandyal 		kling: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Aparna Nandyal
Comment 1 2011-02-16 09:40:46 PST
Analysis of the bug: When the following line in the tester program is called: m_page->mainFrame()->setUrl(QUrl("data:text/html,<body>This is second page")); Function FrameView::pauseScheduledEvents is called 5 times and its equivalent FrameView::resumeScheduledEvents is called only 4 times, leaving the value of m_enqueueEvents=1. So in the tester program when tst_QWebPage::cleanup is called, the FrameView::~FrameView is called which asserts when m_enqueueEvents=1. We need to check the FrameView::layout() to further find the root cause of the problem as the extra calls to FrameView::pauseScheduledEvents is made from this function.
Aparna Nandyal
Comment 2 2011-02-20 10:50:18 PST
Created attachment 83098 [details] Patch for review 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!
Eric Seidel (no email)
Comment 3 2011-02-24 03:17:13 PST
I'm not sure this is right.
Andreas Kling
Comment 4 2011-02-24 04:59:53 PST
Comment on attachment 83098 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=83098&action=review r- because we should implement QWebFrame::setUrl() in a cleaner way. > Source/WebKit/qt/Api/qwebframe.cpp:765 > + // Resetting the load type here can be removed, if resetting > + // happens at end of every load or abort > + d->frame->loader()->resetLoadType(); > d->frame->loader()->activeDocumentLoader()->writer()->begin(absolute); > d->frame->loader()->activeDocumentLoader()->writer()->end(); I have a feeling that we're doing something wrong here (way too many ->'s..) The difference between QWebFrame::setUrl() and QWebFrame::load() is that setUrl() should clear the frame immediately, instead of when the new page has content. There must be a cleaner way than reaching all the way down to the writer(). FrameLoader::clear() perhaps?
Aparna Nandyal
Comment 5 2011-03-01 03:50:29 PST
FrameLoader::clear cannot directly replace d->frame->loader()->activeDocumentLoader()->writer()->begin(absolute); d->frame->loader()->activeDocumentLoader()->writer()->end(); 1. It does not actually clear the view 2. It destroys the Document object which gets created in QWebFrame::QWebFrame Another observation I have is, the current code with DocumentWriter::begin() and DocumentWriter::end() doesnt seem to be clearing the view in all scenarios. If the url is being loaded and setUrl is called before the loading is completed, then the view is not cleared, it acts like QWebFrame::load
Aparna Nandyal
Comment 6 2011-03-14 02:16:51 PDT
This bug no longer is reproducible due to changes made in http://trac.webkit.org/changeset/80210 The variable m_enqueueEvents no longer exists hence the assertion seen is gone. But the root cause of the problem has not been addressed yet that the wrong loadType is set when setUrl is called + setUrl is not actually clearing the view if the page loading is ongoing as mentioned in comment#5. Should we have different bugs to track the problems?
Caio Marcelo de Oliveira Filho
Comment 7 2011-03-14 16:01:12 PDT
Yes, I think creating other bugs for the different issues you've found is a good idea. Would even better if you could provide test cases for the problems, so they can be added to Qt auto tests suite. ;-) I'm closing this bug.
Note You need to log in before you can comment on or make changes to this bug.