Summary: | Crash when logging into gmail.com with frame flattening turned on. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yael <yael> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | ademar, bdakin, commit-queue, ddkilzer, koivisto, laszlo.gombos, mike.zraly, mitz, mrobinson, nancy.piedra, ostap73, simon.fraser, suresh.voruganti | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
Attachments: |
|
Description
Yael
2011-01-14 08:13:56 PST
The reason for the crash is that the resizeEvent causes a size change, which triggers a layout, and another resize event ... Antonio, resizesToContent is not ON, this seems to be a different issue. Recursive layout? 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. Fix required for Qtwebkit 2.2 (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. 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? :-) (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? 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. Some compositing use cases seem to suffer from a similar problem. Bug #41999 is one example of this. 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?
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.
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. 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. (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. 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?
(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. 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 (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. 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. (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? (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 huh! Same use case crashes with my patch too. Removing the review flag for now. 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> 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. 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.
Created attachment 81666 [details]
Patch.
Sorry, I just found a typo in the changelog. Might as well fix it :)
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. 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. Created attachment 81674 [details] Patch. Address comment #33. Comment on attachment 81674 [details]
Patch.
r=me
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. Comment on attachment 81674 [details] Patch. Clearing flags on attachment: 81674 Committed r77988: <http://trac.webkit.org/changeset/77988> All reviewed patches have been landed. Closing bug. (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'll take a look in a bit :) Sorry about that. (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. (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! (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! (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. Revision r77988 cherry-picked into qtwebkit-2.1.x with commit ea0fd56 <http://gitorious.org/webkit/qtwebkit/commit/ea0fd56> 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. (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. No flash content is rendered on Symbian using QtTestBrowser and Webkit 2.1.x. I have not checked other platforms. Reverted on 2.1.x: 4892409e6f6360e034111ca9cc7c38f9e5a04522 (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. 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 |