Bug 52449 – Crash when logging into gmail.com with frame flattening turned on.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Yael]

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]

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

[Antonio Gomes]

is resizesToContent ON? related to bug 43852?

[Laszlo Gombos]

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

[Alexey Proskuryakov]

Recursive layout?

[Yael]

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.

(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]

Fix required for Qtwebkit 2.2

[Yael]

(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 ?

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]

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.

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]

(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]

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]

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

[Yael]

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]

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]

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)]

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]

(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]

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]

(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]

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]

(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]

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]

(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]

(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]

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

[Yael]

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]

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]

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]

Created attachment 81666 [details]
Patch.

Sorry, I just found a typo in the changelog. Might as well fix it :)

[Simon Fraser (smfr)]

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 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]

Created attachment 81674 [details]
Patch.

Address comment #33.

[Antti Koivisto]

Comment on attachment 81674 [details]
Patch.

r=me

[Yael]

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 on attachment 81674 [details]
Patch.

Clearing flags on attachment: 81674

Committed r77988: <http://trac.webkit.org/changeset/77988>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

(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]

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

[Yael]

(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]

(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]

(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]

(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]

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

[Nancy Piedra]

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]

(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]

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

[Ademar Reis]

Reverted on 2.1.x: 4892409e6f6360e034111ca9cc7c38f9e5a04522

[Viatcheslav Ostapenko]

(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]

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