Full Text Bug Listing

Yael

Reported 2011-01-14 08:13:56 PST

Using QtTestBrowser with frame flattening turned on, this crash is very consistent when logging into gmail.com. (On mac and Symbian). In debug builds we hit ASSERT, in release builds, infinite loop. 0 WebCore::Document::updateStyleIfNeeded Document.cpp 1633 0x0000000100789bcd 1 WebCore::Document::updateLayout Document.cpp 1668 0x00000001007899be 2 WebCore::Document::updateLayout Document.cpp 1666 0x00000001007899a8 3 WebCore::Document::updateLayoutIgnorePendingStylesheets Document.cpp 1704 0x000000010078bb30 4 WebCore::Element::clientWidth Element.cpp 381 0x00000001007d393c 5 WebCore::jsElementClientWidth JSElement.cpp 351 0x00000001002b47d7 6 JSC::PropertySlot::getValue PropertySlot.h 78 0x000000010024bea2 7 JSC::JSValue::get JSObject.h 661 0x00000001006020bc 8 cti_op_get_by_id JITStubs.cpp 1657 0x00000001011fcc0e 9 WTF::doubleHash HashTable.h 447 0x00000001011f0541 10 JSC::JITCode::execute JITCode.h 77 0x00000001011cbe57 11 JSC::Interpreter::executeCall Interpreter.cpp 849 0x00000001011c5b8f 12 JSC::call CallData.cpp 38 0x0000000101248551 13 WebCore::JSMainThreadExecState::call JSMainThreadExecState.h 48 0x00000001005c104d 14 WebCore::JSEventListener::handleEvent JSEventListener.cpp 124 0x00000001005edcc5 15 WebCore::EventTarget::fireEventListeners EventTarget.cpp 342 0x00000001007dd283 16 WebCore::EventTarget::fireEventListeners EventTarget.cpp 311 0x00000001007ddbf5 17 WebCore::DOMWindow::dispatchEvent DOMWindow.cpp 1549 0x0000000100b328af 18 WebCore::Document::dispatchWindowEvent Document.cpp 3498 0x00000001007865d0 19 WebCore::EventHandler::sendResizeEvent EventHandler.cpp 2795 0x0000000100b44c56 20 WebCore::FrameView::performPostLayoutTasks FrameView.cpp 1839 0x0000000100b6bdd6 21 WebCore::FrameView::layout FrameView.cpp 928 0x0000000100b6f57c 22 WebCore::FrameView::visibleContentsResized FrameView.cpp 1393 0x0000000100b6fd0c 23 WebCore::ScrollView::updateScrollbars ScrollView.cpp 424 0x0000000100c2edf5 24 WebCore::ScrollView::setFrameRect ScrollView.cpp 789 0x0000000100c2fce2 25 WebCore::FrameView::setFrameRect FrameView.cpp 335 0x0000000100b707bc 26 WebCore::RenderWidget::setWidgetGeometry RenderWidget.cpp 177 0x0000000100da0c91 27 WebCore::RenderWidget::updateWidgetPosition RenderWidget.cpp 352 0x0000000100da0ea7 28 WebCore::RenderFrameBase::layoutWithFlattening RenderFrameBase.cpp 55 0x0000000100d00f51 29 WebCore::RenderIFrame::layout RenderIFrame.cpp 114 0x0000000100d056ec 30 WebCore::RenderObject::layoutIfNeeded RenderObject.h 501 0x0000000100cc7b57 31 WebCore::RenderBlock::layoutInlineChildren RenderBlockLineLayout.cpp 566 0x0000000100cc5764 32 WebCore::RenderBlock::layoutBlock RenderBlock.cpp 1230 0x0000000100ca4752 33 WebCore::RenderBlock::layout RenderBlock.cpp 1128 0x0000000100ca32f4 34 WebCore::RenderBlock::layoutBlockChild RenderBlock.cpp 1959 0x0000000100ca25fc 35 WebCore::RenderBlock::layoutBlockChildren RenderBlock.cpp 1897 0x0000000100ca40fa 36 WebCore::RenderBlock::layoutBlock RenderBlock.cpp 1232 0x0000000100ca476b 37 WebCore::RenderBlock::layout RenderBlock.cpp 1128 0x0000000100ca32f4 38 WebCore::RenderBlock::layoutBlockChild RenderBlock.cpp 1959 0x0000000100ca25fc 39 WebCore::RenderBlock::layoutBlockChildren RenderBlock.cpp 1897 0x0000000100ca40fa 40 WebCore::RenderBlock::layoutBlock RenderBlock.cpp 1232 0x0000000100ca476b 41 WebCore::RenderBlock::layout RenderBlock.cpp 1128 0x0000000100ca32f4 42 WebCore::RenderBlock::layoutBlockChild RenderBlock.cpp 1959 0x0000000100ca25fc 43 WebCore::RenderBlock::layoutBlockChildren RenderBlock.cpp 1897 0x0000000100ca40fa 44 WebCore::RenderBlock::layoutBlock RenderBlock.cpp 1232 0x0000000100ca476b 45 WebCore::RenderBlock::layout RenderBlock.cpp 1128 0x0000000100ca32f4 46 WebCore::RenderView::layout RenderView.cpp 130 0x0000000100d97ef1 47 WebCore::FrameView::layout FrameView.cpp 872 0x0000000100b6f2b0 48 WebCore::FrameView::visibleContentsResized FrameView.cpp 1393 0x0000000100b6fd0c 49 WebCore::ScrollView::updateScrollbars ScrollView.cpp 424 0x0000000100c2edf5 50 WebCore::ScrollView::setScrollbarModes ScrollView.cpp 156 0x0000000100c300ce 51 WebCore::FrameView::layout FrameView.cpp 837 0x0000000100b6f04c 52 WebCore::Document::updateLayout Document.cpp 1673 0x0000000100789a22 53 WebCore::Document::updateLayout Document.cpp 1666 0x00000001007899a8 54 WebCore::Document::updateLayoutIgnorePendingStylesheets Document.cpp 1704 0x000000010078bb30 55 WebCore::Element::clientWidth Element.cpp 381 0x00000001007d393c 56 WebCore::jsElementClientWidth JSElement.cpp 351 0x00000001002b47d7 57 JSC::PropertySlot::getValue PropertySlot.h 78 0x000000010024bea2 58 JSC::JSValue::get JSObject.h 661 0x00000001006020bc 59 cti_op_get_by_id JITStubs.cpp 1657 0x00000001011fcc0e 60 WTF::doubleHash HashTable.h 447 0x00000001011f0541 61 JSC::JITCode::execute JITCode.h 77 0x00000001011cbe57 62 JSC::Interpreter::execute Interpreter.cpp 778 0x00000001011c6ae5 63 JSC::evaluate Completion.cpp 62 0x00000001012525d2 64 WebCore::JSMainThreadExecState::evaluate JSMainThreadExecState.h 54 0x00000001005fa419 65 WebCore::ScriptController::evaluateInWorld ScriptController.cpp 148 0x000000010060ee79 66 WebCore::ScriptController::evaluate ScriptController.cpp 171 0x000000010060f2d6 67 WebCore::ScriptController::executeScript ScriptControllerBase.cpp 60 0x00000001005b5dd6 68 WebCore::ScriptElement::executeScript ScriptElement.cpp 216 0x0000000100825a5b 69 WebCore::HTMLScriptRunner::runScript HTMLScriptRunner.cpp 316 0x00000001009cf11f 70 WebCore::HTMLScriptRunner::execute HTMLScriptRunner.cpp 173 0x00000001009cfd17 71 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder HTMLDocumentParser.cpp 199 0x00000001009c1454 72 WebCore::HTMLDocumentParser::pumpTokenizer HTMLDocumentParser.cpp 244 0x00000001009c1a2b 73 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible HTMLDocumentParser.cpp 169 0x00000001009c1d40 74 WebCore::HTMLDocumentParser::append HTMLDocumentParser.cpp 320 0x00000001009c222b 75 WebCore::DecodedDataDocumentParser::appendBytes DecodedDataDocumentParser.cpp 54 0x0000000100776775 76 WebCore::DocumentWriter::addData DocumentWriter.cpp 200 0x0000000100abb3a6 77 WebCore::DocumentLoader::commitData DocumentLoader.cpp 310 0x0000000100aad5dc 78 WebCore::FrameLoaderClientQt::committedLoad FrameLoaderClientQt.cpp 882 0x0000000100e4fd38 79 WebCore::DocumentLoader::commitLoad DocumentLoader.cpp 295 0x0000000100aad6dc 80 WebCore::DocumentLoader::receivedData DocumentLoader.cpp 322 0x0000000100aad75a 81 WebCore::MainResourceLoader::addData MainResourceLoader.cpp 157 0x0000000100ae29fc 82 WebCore::ResourceLoader::didReceiveData ResourceLoader.cpp 278 0x0000000100af0312 83 WebCore::MainResourceLoader::didReceiveData MainResourceLoader.cpp 442 0x0000000100ae1e9f 84 WebCore::ResourceLoader::didReceiveData ResourceLoader.cpp 429 0x0000000100aefaa0 85 WebCore::QNetworkReplyHandler::forwardData QNetworkReplyHandler.cpp 482 0x0000000100e1670d 86 WebCore::QNetworkReplyHandler::qt_metacall moc_QNetworkReplyHandler.cpp 86 0x0000000100e18834 87 QObject::event 0 0x000000010776c169 88 QApplicationPrivate::notify_helper 0 0x0000000106a3628d 89 QApplication::notify 0 0x0000000106a3db2e 90 QCoreApplication::notifyInternal 0 0x00000001076827cc 91 QCoreApplicationPrivate::sendPostedEvents 0 0x000000010775f2ab 92 __CFRunLoopDoSources0 0 0x00007fff8804b401 93 __CFRunLoopRun 0 0x00007fff880495f9 94 CFRunLoopRunSpecific 0 0x00007fff88048dbf 95 RunCurrentEventLoopInMode 0 0x00007fff8293991a 96 ReceiveNextEventCommon 0 0x00007fff8293971f 97 BlockUntilNextEventMatchingListInMode 0 0x00007fff829395d8 98 _DPSNextEvent 0 0x00007fff86aa2e64 99 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] 0 0x00007fff86aa27a9 100 -[NSApplication run] 0 0x00007fff86a6848b 101 QEventDispatcherMac::processEvents 0 0x00000001069f2084 102 QEventLoop::processEvents 0 0x000000010775dc14 103 QEventLoop::exec 0 0x000000010775df34 104 QCoreApplication::exec 0 0x000000010775f55c 105 launcherMain main.cpp 41 0x0000000100011163 106 main main.cpp 274 0x000000010001186d

Yael

Comment 1 2011-01-14 08:19:09 PST

The reason for the crash is that the resizeEvent causes a size change, which triggers a layout, and another resize event ...

Antonio Gomes

Comment 2 2011-01-14 08:37:25 PST

is resizesToContent ON? related to bug 43852?

Laszlo Gombos

Comment 3 2011-01-14 09:19:33 PST

Antonio, resizesToContent is not ON, this seems to be a different issue.

Alexey Proskuryakov

Comment 4 2011-01-14 10:23:05 PST

Recursive layout?

Yael

Comment 5 2011-01-15 17:55:29 PST

This bug appears only with frame flattening turned on. I can reproduce this crash very easily with this markup: Main frame: <!DOCTYPE html> <html > <head> <style> #My { background-color: blue; width: 100px; height: 100px; } </style> </head> <body > <iframe id="My" src="frm.html"></iframe> </body></html> IFrame: <!DOCTYPE html> <script> function res() { var i = document.getElementsByTagName('html')[0].clientWidth; } </script> <style> #p { border: 4px solid red;} </style> <body onresize="res();"> <div id='p'><br><br><br><br><br></div> <script> </script> </body></html> When we do frame flattening, after we "flatten" the frame, we schedule a relayout of all ancestors. See comment in FrameView::scheduleRelayout(). Then we send a resize event for the resized iframe while a new layout is pending for its parent. Callbacks from the resize event handler to the DOM trigger the new layout recursively. I think we should not send a resize event on iframes when we do frame flattening. The resize event is supposed to tell the JavaScript that the user resized the page or frame, not that the browser resized the frame programmatically. Patch coming soon.

mitz

Comment 6 2011-01-15 18:02:09 PST

(In reply to comment #5) > I think we should not send a resize event on iframes when we do frame flattening. Perhaps. > The resize event is supposed to tell the JavaScript that the user resized the page or frame, not that the browser resized the frame programmatically. I am not sure that is correct. With frame flattening disabled, we always fire the resize event when an iframe’s size changes, regardless of the reason (which is typically layout of the containing document). This makes sense to me, because whatever the event handler does to adapt the document to the new size most likely needs to be done regardless of how the frame was resized. I think the right fix for this should be along the lines of making the post-layout-task-deferral mechanism act on a document scope instead of on a single frame scope.

Suresh Voruganti

Comment 7 2011-01-17 06:11:11 PST

Fix required for Qtwebkit 2.2

Yael

Comment 8 2011-01-17 12:01:37 PST

(In reply to comment #6) > I think the right fix for this should be along the lines of making the post-layout-task-deferral mechanism act on a document scope instead of on a single frame scope. While working on a solution that defers the post-layout tasks, I hit the problem that if we start a layout with (m_hasPendingPostLayoutTasks == true), we call performPostLayoutTasks() before the layout and now that crashes :( Is it ok to skip this call if we are about to do another layout ?

mitz

Comment 9 2011-01-17 12:06:46 PST

Yes, post-layout tasks are the tasks that may be deferred unti after layout() returns. There’s a mechanism that sometimes schedules them on a 0-delay timer (roughly speaking, the code tries to perform post-layout tasks synchronously immediately after layout once, but if they need to be performed again, it will schedule them asynchronously). I suspect that the mechanism might be defeated in certain nested-frame configurations.

Yael

Comment 10 2011-01-18 19:23:56 PST

Created attachment 79384 [details] Work in progress. This patch is work in progress. It breaks 3 compositioning/iframes tests that I need to figure out why.

mitz

Comment 11 2011-01-18 20:09:12 PST

Comment on attachment 79384 [details] Work in progress. View in context: https://bugs.webkit.org/attachment.cgi?id=79384&action=review > Source/WebCore/page/FrameView.cpp:2282 > + while (needsLayout()); What? :-)

Yael

Comment 12 2011-01-19 05:50:21 PST

(In reply to comment #11) > (From update of attachment 79384 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79384&action=review > > > Source/WebCore/page/FrameView.cpp:2282 > > + while (needsLayout()); > > What? :-) Looks strange, doesn't it ? When we do layout with frame flattening, the child frame marks its parent for layout again and again until there are no more changes. Without this change, if we try to paint when a layout is pending (e.g. during load), we first call updateLayoutAndStyleIfNeededRecursive, but when we exit this function, the parent still has its layout flag set, and we ASSERT in paintContents() Could you please suggest how to resolve this in a better way?

Yael

Comment 13 2011-01-19 05:53:11 PST

Is there an example other than frame flattening, of something that needs to do layout in asynchronous way? I would try to understand how it is done there and hopefully find a better solution.

Yael

Comment 14 2011-01-19 09:10:12 PST

Some compositing use cases seem to suffer from a similar problem. Bug #41999 is one example of this.

Yael

Comment 15 2011-01-19 11:35:08 PST

Created attachment 79457 [details] Still work in progress This patch fixes the regression in compositioning/iframes. Apparently, we have to keep the calls to performPostLayoutTasks from inside nested layouts. Mitz, you did not like the way I force multiple layouts, could you please suggest a better way of doing it?

Yael

Comment 16 2011-01-21 12:36:20 PST

Created attachment 79776 [details] Patch Frame flattening algorithm requires that layout always starts from the main frame, since layout of subframes impacts the layout of their parents. There are places in the code that call view->layout() not on the main frame. Instead of changing all the callsites, I changed FrameView::layout() to force layout from the main frame if frame flattening is enabled. In addition, postLayoutTasks can trigger relayout, so make it use the timer even more.

Yael

Comment 17 2011-01-24 07:02:55 PST

If we call document::updateLayout() when a style recalc is pending on the iframe document, what happens is 1. We do layout, starting from the main frame. 2. Update the style of the iframe 3. We do a second layout, but this time we don't start from the main frame. We start from the iframe. As a result, when frame flattening is enabled, we get out of updateLayout() with a layout still pending in the main frame, and that can cause all sorts of problems. My patch is preventing that situation by forcing the second layout round to start from the main frame as well if frame flattening is enabled.

Simon Fraser (smfr)

Comment 18 2011-01-24 08:29:39 PST

It seems that if frame flattening is enabled, a layout in any frame should be treated as a layout of the main frame and all subframes.

Yael

Comment 19 2011-01-24 08:51:46 PST

(In reply to comment #18) > It seems that if frame flattening is enabled, a layout in any frame should be treated as a layout of the main frame and all subframes. That is what my patch is doing - checking if the layout of an iframe was started from the main frame ( by checking parentView->m_nestedLayoutCount), and if not, call layout on the main frmae.

Antti Koivisto

Comment 20 2011-01-26 15:00:04 PST

Created attachment 80244 [details] another strategy This simpler patch fixes the test case (without affecting other tests). The idea is to avoid running synchronous post-layout tasks when in nested layout (which happens in frame flattening case). Yael, could you try it a bit in real world cases to see if it is sufficient?

Yael

Comment 21 2011-01-26 16:36:22 PST

(In reply to comment #20) > Created an attachment (id=80244) [details] > another strategy > > This simpler patch fixes the test case (without affecting other tests). The idea is to avoid running synchronous post-layout tasks when in nested layout (which happens in frame flattening case). > > Yael, could you try it a bit in real world cases to see if it is sufficient? Thank you for taking the time to look at this error. I will test it tomorrow morning.

Yael

Comment 22 2011-01-26 17:37:21 PST

I did not test this on Symbian yet, but this was crashing (ASSERT) on Linux : ASSERTION FAILED: !view() || (!view()->isInLayout() && !view()->isPainting()) (../../../../Source/WebCore/dom/Document.cpp:1639 virtual void WebCore::Document::updateStyleIfNeeded()) Segmentation fault

Yael

Comment 23 2011-01-27 05:48:25 PST

(In reply to comment #22) > I did not test this on Symbian yet, but this was crashing (ASSERT) on Linux : > > ASSERTION FAILED: !view() || (!view()->isInLayout() && !view()->isPainting()) > (../../../../Source/WebCore/dom/Document.cpp:1639 virtual void WebCore::Document::updateStyleIfNeeded()) > Segmentation fault To be clear, I was able to load gmail.com with the patch, but then I clicked with the mouse either on empty area or on a link and got the ASSERT.

Yael

Comment 24 2011-01-27 06:26:26 PST

Antti, you asked me on IRC why do we call postLayoutTasks synchronously before starting a new layout: It was introduced in http://trac.webkit.org/changeset/28371, when the method performPostLayoutTasks was introduced. The test that was introduced in that patch is still passing, and we still send resize events for iframes, so I think that change should be ok.

Antti Koivisto

Comment 25 2011-01-27 09:29:10 PST

(In reply to comment #24) > Antti, you asked me on IRC why do we call postLayoutTasks synchronously before starting a new layout: > It was introduced in http://trac.webkit.org/changeset/28371, when the method performPostLayoutTasks was introduced. > The test that was introduced in that patch is still passing, and we still send resize events for iframes, so I think that change should be ok. Thats a good information. What is the stack for the assert after loading the page? Could you make a test case for it?

Yael

Comment 26 2011-01-27 10:05:38 PST

(In reply to comment #25) > (In reply to comment #24) > > What is the stack for the assert after loading the page? Could you make a test case for it? Below is the callstack. I am still working on updating the test case. 0 WebCore::Document::updateStyleIfNeeded Document.cpp 1644 0x0180c6de 1 WebCore::Document::updateLayout Document.cpp 1679 0x0180c93d 2 WebCore::Document::updateLayout Document.cpp 1677 0x0180c929 3 WebCore::Document::updateLayoutIgnorePendingStylesheets Document.cpp 1715 0x0180ca89 4 WebCore::VisiblePosition::canonicalPosition VisiblePosition.cpp 459 0x01978a18 5 WebCore::VisiblePosition::init VisiblePosition.cpp 61 0x01976e4e 6 WebCore::VisiblePosition::VisiblePosition VisiblePosition.cpp 48 0x01976bda 7 WebCore::SelectionController::updateCaretRect SelectionController.cpp 980 0x0195e48c 8 WebCore::SelectionController::localCaretRect SelectionController.cpp 1032 0x0195e7d1 9 WebCore::SelectionController::recomputeCaretRect SelectionController.cpp 1083 0x0195eaac 10 WebCore::SelectionController::updateAppearance SelectionController.cpp 1490 0x01960ac0 11 WebCore::FrameView::layout FrameView.cpp 897 0x01bef352 12 WebCore::FrameView::visibleContentsResized FrameView.cpp 1406 0x01bf1240 13 WebCore::ScrollView::updateScrollbars ScrollView.cpp 418 0x01cbfc07 14 WebCore::ScrollView::setFrameRect ScrollView.cpp 774 0x01cc1758 15 WebCore::FrameView::setFrameRect FrameView.cpp 338 0x01bed812 16 WebCore::RenderWidget::setWidgetGeometry RenderWidget.cpp 176 0x01e33030 17 WebCore::RenderWidget::updateWidgetPosition RenderWidget.cpp 344 0x01e33ef8 18 WebCore::RenderFrameBase::layoutWithFlattening RenderFrameBase.cpp 55 0x01d94151 19 WebCore::RenderIFrame::layout RenderIFrame.cpp 114 0x01d98bb5 20 WebCore::RenderObject::layoutIfNeeded RenderObject.h 503 0x01d237c9 21 WebCore::RenderBlock::layoutInlineChildren RenderBlockLineLayout.cpp 569 0x01d57275 22 WebCore::RenderBlock::layoutBlock RenderBlock.cpp 1221 0x01d2b317 23 WebCore::RenderBlock::layout RenderBlock.cpp 1119 0x01d2ad1e 24 WebCore::RenderBlock::layoutBlockChild RenderBlock.cpp 1958 0x01d2e2b8 25 WebCore::RenderBlock::layoutBlockChildren RenderBlock.cpp 1896 0x01d2df73 26 WebCore::RenderBlock::layoutBlock RenderBlock.cpp 1223 0x01d2b336 27 WebCore::RenderBlock::layout RenderBlock.cpp 1119 0x01d2ad1e 28 WebCore::RenderBlock::layoutBlockChild RenderBlock.cpp 1958 0x01d2e2b8 29 WebCore::RenderBlock::layoutBlockChildren RenderBlock.cpp 1896 0x01d2df73 30 WebCore::RenderBlock::layoutBlock RenderBlock.cpp 1223 0x01d2b336 31 WebCore::RenderBlock::layout RenderBlock.cpp 1119 0x01d2ad1e 32 WebCore::RenderBlock::layoutBlockChild RenderBlock.cpp 1958 0x01d2e2b8 33 WebCore::RenderBlock::layoutBlockChildren RenderBlock.cpp 1896 0x01d2df73 34 WebCore::RenderBlock::layoutBlock RenderBlock.cpp 1223 0x01d2b336 35 WebCore::RenderBlock::layout RenderBlock.cpp 1119 0x01d2ad1e 36 WebCore::RenderView::layout RenderView.cpp 130 0x01e27732 37 WebCore::FrameView::layout FrameView.cpp 884 0x01bef2b1 38 WebCore::FrameView::layoutTimerFired FrameView.cpp 1522 0x01bf1855 39 WebCore::Timer<WebCore::FrameView>::fired Timer.h 99 0x01bf9960 40 WebCore::ThreadTimers::sharedTimerFiredInternal ThreadTimers.cpp 112 0x01cd54a3 41 WebCore::ThreadTimers::sharedTimerFired ThreadTimers.cpp 90 0x01cd53ed 42 WebCore::SharedTimerQt::timerEvent SharedTimerQt.cpp 116 0x01ec40aa 43 QObject::event qobject.cpp 1175 0x0456249c 44 QApplicationPrivate::notify_helper qapplication.cpp 4396 0x038617b6 45 QApplication::notify qapplication.cpp 3798 0x0385effa 46 QCoreApplication::notifyInternal qcoreapplication.cpp 732 0x0454962f 47 QCoreApplication::sendEvent qcoreapplication.h 215 0x0806e02f 48 QTimerInfoList::activateTimers qeventdispatcher_unix.cpp 602 0x045865ce 49 timerSourceDispatch qeventdispatcher_glib.cpp 184 0x04582140 50 g_main_context_dispatch /lib/libglib-2.0.so.0 0 0x0542f5e5 51 ?? /lib/libglib-2.0.so.0 0 0x054332d8 52 g_main_context_iteration /lib/libglib-2.0.so.0 0 0x054334b8 53 QEventDispatcherGlib::processEvents qeventdispatcher_glib.cpp 415 0x04583312 54 QGuiEventDispatcherGlib::processEvents qguieventdispatcher_glib.cpp 204 0x0393c5fe 55 QEventLoop::processEvents qeventloop.cpp 149 0x045469ef 56 QEventLoop::exec qeventloop.cpp 201 0x04546b34 57 QCoreApplication::exec qcoreapplication.cpp 1009 0x04549d21 58 QApplication::exec qapplication.cpp 3672 0x0385ec50 59 launcherMain main.cpp 41 0x080701fb 60 main main.cpp 274 0x08072644

Yael

Comment 27 2011-01-27 11:19:54 PST

huh! Same use case crashes with my patch too. Removing the review flag for now.

Yael

Comment 28 2011-02-07 14:43:05 PST

Seems like selections and frame flattening do not want to work together. :( With frame flattening we do layout recursively, and when we finish the layout of the iframe, we update the selection of that iframe. The parent frame still has its layout flag turned on, and that is causing the crash in comment #26. This simple example will crash when frame flattening is on: main frame: <!DOCTYPE html> <html > <body > <iframe id="My" src="child.html"></iframe> </body></html> iframe (child.html) <!DOCTYPE html> <script> window.onload=function(){ document.getElementById('in').focus(); document.getElementById('p').appendChild(document.createElement("br")); } </script> <body> <div id='p'><input id="in" value="abcd"><br><br><br><br><br><br><br><br><br></div> </body></html>

Yael

Comment 29 2011-02-07 20:25:28 PST

One possible way to fix this crash is to move the selection handling from layout() to performaPostLayoutTasks(). I will test it for possible regressions and propose a patch.

Yael

Comment 30 2011-02-08 11:08:04 PST

Created attachment 81665 [details] Patch. Frame flattening algorithm requires that layout always starts from the main frame, since layout of subframes impacts the layout of their parents. There are places in the code that call view->layout() not on the main frame. Instead of changing all the callsites, I changed FrameView::layout() to force layout from the main frame if frame flattening is enabled. In addition, postLayoutTasks can trigger relayout, so make it use the timer even more. Move the call to SelectionController::updateAppearance() to performPostLayoutTasks(), because calling ths from layout() leads to a crash in pages that have a selection in an iframe. Antti, with your approach, I still see sometimes a race conditions when we render the page. It is possible to call FrameView::paintContents() when a layout is pending, and that would still crash sometimes.

Yael

Comment 31 2011-02-08 11:10:13 PST

Created attachment 81666 [details] Patch. Sorry, I just found a typo in the changelog. Might as well fix it :)

Simon Fraser (smfr)

Comment 32 2011-02-08 11:24:20 PST

Comment on attachment 81666 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=81666&action=review > Source/WebCore/page/FrameView.cpp:954 > + if (!m_hasPendingPostLayoutTasks && ((needsLayout() || m_inSynchronousPostLayout) || deferPostLayoutTasks)) { Extra space here.

Antti Koivisto

Comment 33 2011-02-08 11:46:41 PST

Comment on attachment 81666 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=81666&action=review The patch is basically ok but I still say r- as I'd like to see a round of cleanups. > Source/WebCore/page/FrameView.cpp:728 > + if (m_frame->settings() && m_frame->settings()->frameFlatteningEnabled()) { Please make a variable for "parent() && m_frame->settings() && m_frame->settings()->frameFlatteningEnabled()" and call it "inSubframeLayoutWithFrameFlattening" or something. You can use it here and later... > Source/WebCore/page/FrameView.cpp:730 > + FrameView* parentView = static_cast<FrameView*>(parent()); > + if (parentView && !parentView->m_nestedLayoutCount) { You shouldn't just cast without testing the type (even if it currently might work in practice). See other places in the file for examples on how to get the parent FrameView without casting by using tree(). > Source/WebCore/page/FrameView.cpp:770 > + if (!m_nestedLayoutCount && !m_inSynchronousPostLayout && m_hasPendingPostLayoutTasks && (!(parent() && m_frame->settings() && m_frame->settings()->frameFlatteningEnabled()))) { ...here... > Source/WebCore/page/FrameView.cpp:947 > + bool deferPostLayoutTasks = parent() && m_frame->settings() && m_frame->settings()->frameFlatteningEnabled(); > + if (!m_inSynchronousPostLayout && !deferPostLayoutTasks) { and here... >> Source/WebCore/page/FrameView.cpp:954 >> + if (!m_hasPendingPostLayoutTasks && ((needsLayout() || m_inSynchronousPostLayout) || deferPostLayoutTasks)) { > > Extra space here. and here. Also remove the extra space and remove the inner parenthesis from the or condition, they do nothing.

Yael

Comment 34 2011-02-08 12:20:01 PST

Created attachment 81674 [details] Patch. Address comment #33.

Antti Koivisto

Comment 35 2011-02-08 12:37:17 PST

Comment on attachment 81674 [details] Patch. r=me

Yael

Comment 36 2011-02-08 14:30:31 PST

Ademar, when I tested this on Symbian, I needed to pull parts of http://trac.webkit.org/changeset/66552 and http://trac.webkit.org/changeset/66555. Please let me know if you need help merging this to WebKit 2.1.x.

WebKit Commit Bot

Comment 37 2011-02-08 16:26:11 PST

Comment on attachment 81674 [details] Patch. Clearing flags on attachment: 81674 Committed r77988: <http://trac.webkit.org/changeset/77988>

WebKit Commit Bot

Comment 38 2011-02-08 16:26:18 PST

All reviewed patches have been landed. Closing bug.

Martin Robinson

Comment 39 2011-02-09 00:00:04 PST

(In reply to comment #38) > All reviewed patches have been landed. Closing bug. fast/frames/flattening/iframe-flattening-crash.html is failing on GTK+ for some reason. It seems that the onresize event of the iframe is never called. Any clue what might be happening?

Yael

Comment 40 2011-02-09 05:17:09 PST

I'll take a look in a bit :) Sorry about that.

Yael

Comment 41 2011-02-09 06:06:44 PST

(In reply to comment #39) > (In reply to comment #38) > > All reviewed patches have been landed. Closing bug. > > fast/frames/flattening/iframe-flattening-crash.html is failing on GTK+ for some reason. It seems that the onresize event of the iframe is never called. Any clue what might be happening? I am not able to compile WebKitGTK on my Ubuntu 10.4 machine, so this might complicate things :( One thing I noticed is that all other frame flattening tests use absolute positioning, to force a layout (with flattening) after the frame flattening flag was turned on. This test does not do that. I'll convert it to use absolute positioning and see if someone with a working WebKitGTK can test that for me.

Martin Robinson

Comment 42 2011-02-09 08:29:56 PST

(In reply to comment #41) > I am not able to compile WebKitGTK on my Ubuntu 10.4 machine, so this might complicate things :( If you have a 10.10 machine, I've created a PPA with the necessary packages. I'll see if I can get it working for 10.04 as well. > One thing I noticed is that all other frame flattening tests use absolute positioning, to force a layout (with flattening) after the frame flattening flag was turned on. > This test does not do that. > I'll convert it to use absolute positioning and see if someone with a working WebKitGTK can test that for me. Sure, I'll be happy to test it. Thanks for looking into this!

Yael

Comment 43 2011-02-09 08:35:49 PST

(In reply to comment #42) > (In reply to comment #41) > > If you have a 10.10 machine, I've created a PPA with the necessary packages. I'll see if I can get it working for 10.04 as well. > Sorry, I don't :( My workstation won't even boot into Ubuntu 10.10 live CD. > > Sure, I'll be happy to test it. Thanks for looking into this! The new test is in https://bugs.webkit.org/show_bug.cgi?id=54106 . thanks!

Ademar Reis

Comment 44 2011-02-11 09:30:54 PST

(In reply to comment #36) > Ademar, when I tested this on Symbian, I needed to pull parts of http://trac.webkit.org/changeset/66552 and http://trac.webkit.org/changeset/66555. Please let me know if you need help merging this to WebKit 2.1.x. Thanks for leting me know of that. I'll see what I can do earlier next week. The two commits you mentioned appear to be simple and backporting them should be no problem.

Ademar Reis

Comment 45 2011-02-14 13:36:04 PST

Revision r77988 cherry-picked into qtwebkit-2.1.x with commit ea0fd56 <http://gitorious.org/webkit/qtwebkit/commit/ea0fd56>

Nancy Piedra

Comment 46 2011-02-17 10:31:35 PST

This change has caused a regression in Webkit 2.1.x. With this change flash content is not displayed (on Symbian). I'm not sure the impact to other platforms.

Yael

Comment 47 2011-02-17 10:35:40 PST

(In reply to comment #46) > This change has caused a regression in Webkit 2.1.x. With this change flash content is not displayed (on Symbian). I'm not sure the impact to other platforms. Can you be more specific? I can watch youtube videos With QtTestBrowser on linux, when frame flattening is enabled, so it is not that flash is always broken.

Nancy Piedra

Comment 48 2011-02-17 10:40:20 PST

No flash content is rendered on Symbian using QtTestBrowser and Webkit 2.1.x. I have not checked other platforms.

Ademar Reis

Comment 49 2011-02-17 12:06:24 PST

Reverted on 2.1.x: 4892409e6f6360e034111ca9cc7c38f9e5a04522

Viatcheslav Ostapenko

Comment 50 2011-02-22 16:19:01 PST

(In reply to comment #48) > No flash content is rendered on Symbian using QtTestBrowser and Webkit 2.1.x. I have not checked other platforms. Yaels patch requires this http://trac.webkit.org/changeset/75878 change to work correctly, because without it webkit creates extra subframe when load plugin and this way interferes with frame flattening.

Ademar Reis

Comment 51 2011-02-25 11:01:57 PST

Revision r77988 cherry-picked into qtwebkit-2.1.x with commit ea0fd56 <http://gitorious.org/webkit/qtwebkit/commit/ea0fd56> cherry-picked again into qtwebkit-2.1.x (after also cherry-picking http://trac.webkit.org/changeset/75878): c3702ad

Attachments
Work in progress. (8.02 KB, patch) 2011-01-18 19:23 PST, Yael	no flags	Details Formatted Diff Diff
Still work in progress (7.67 KB, patch) 2011-01-19 11:35 PST, Yael	no flags	Details Formatted Diff Diff
Patch (6.50 KB, patch) 2011-01-21 12:36 PST, Yael	no flags	Details Formatted Diff Diff
another strategy (1.77 KB, patch) 2011-01-26 15:00 PST, Antti Koivisto	no flags	Details Formatted Diff Diff
Patch. (9.66 KB, patch) 2011-02-08 11:08 PST, Yael	no flags	Details Formatted Diff Diff
Patch. (9.66 KB, patch) 2011-02-08 11:10 PST, Yael	koivisto: review-	Details Formatted Diff Diff
Patch. (9.72 KB, patch) 2011-02-08 12:20 PST, Yael	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Bug 52449