Bug 89733 - Figure out why FrameView.cpp's ASSERT(!needsLayout()) is firing in chromium & chromium's DRT
: Figure out why FrameView.cpp's ASSERT(!needsLayout()) is firing in chromium &...
Status: RESOLVED WONTFIX
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-06-21 21:01 PST by
Modified: 2012-10-24 13:00 PST (History)


Attachments


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-06-21 21:01:42 PST
For years(!) the ASSERT(!needsLayout()) in http://trac.webkit.org/browser/trunk/Source/WebCore/page/FrameView.cpp#L3046 has been triggering in debug builds of chromium & DumpRenderTree, apparently spuriously.  
The assert is immediately followed by an early return (for NDEBUG builds).
Does it really need to be there?  I don't know.  And I'm not sure who to ask about this, either.  I'm hoping one of the people I (semi-randomly) cc on this bug will at least know the latter, if not the former.

For backtraces and various ways this assert has been triggered, see http://code.google.com/p/chromium/issues/detail?id=128776
------- Comment #1 From 2012-06-22 09:52:48 PST -------
My non-expert understanding is that it's a sign of a serious bug whenever this assertion fails, and that a number of specific cases have been addressed over the years.
------- Comment #2 From 2012-06-22 11:05:07 PST -------
That assertion is there for a reason. We should never be painting if layout is not up-to-date. There should be mechanisms that ensure that (viewWillDraw in WK1 etc).
------- Comment #3 From 2012-06-22 21:39:08 PST -------
(In reply to comment #2)
> That assertion is there for a reason. We should never be painting if layout is not up-to-date. There should be mechanisms that ensure that (viewWillDraw in WK1 etc).

Thanks for the info, Simon!

Searching blindly for viewWillDraw (since I know next to nothing about the code in question) led me to http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm&exact_package=chromium&q=viewWillDraw&l=1313 which makes me wonder if some of those "known cases where -viewWillDraw is not called on all views being drawn" are the problem chromium is tripping over (without compensating logic like that WebHTMLView.mm code).  Do you know what that comment is about and whether it is likely ir/relevant to the chromium bug?
(or, does anything in the stacktrace in http://code.google.com/p/chromium/issues/detail?id=128776#c5 ring a bell?)
------- Comment #4 From 2012-10-23 15:26:47 PST -------
@simon.fraser: ping.  Any ideas about comment #3?
------- Comment #5 From 2012-10-23 15:30:00 PST -------
Not really. I'd think that TiledLayerChromium.cpp or something at that level should make sure that layout is up-to-date before painting.
------- Comment #6 From 2012-10-23 17:34:23 PST -------
AIUI, the chromium compositor assumes that when the main thread is idle, (ie a posted task from the compositor gets scheduled) that layout is always up to date.

I guess this is not correct?
------- Comment #7 From 2012-10-23 17:41:41 PST -------
We run layout() and then iterate through our composited layers and paint various ones.  We assume that layout is still up-to-date since we don't run any other tasks after calling layout() and we don't do anything other than GraphicsLayerClient::paint..() calls.  Various bits of RenderObject implementations incorrectly mark themselves or the FrameView as needing layout either during the layout() call or during paint, leading to this assertion.  The fix is to figure out who's issuing the spurious setNeedsLayout() calls and killing them.
------- Comment #8 From 2012-10-24 00:00:45 PST -------
@jamesr: (keeping in mind that I still have no idea what I'm doing) I followed up your hint and added megahacks to assert if RenderObject::setNeedsLayout() is called during WebViewImpl::paint(), and the assert indeed triggered:

        WebCore::RenderObject::setNeedsLayout() [0x7f7e5781e4ff]
        WebCore::RenderVideo::updateIntrinsicSize() [0x7f7e57a5353a]
        WebCore::RenderVideo::updatePlayer() [0x7f7e57a53d79]
        WebCore::RenderVideo::paintReplaced() [0x7f7e57a53acc]
        WebCore::RenderReplaced::paint() [0x7f7e579f6299]    
        WebCore::RenderImage::paint() [0x7f7e5794c0ca]       
        WebCore::RenderLayer::paintLayerContents() [0x7f7e5796c35e]
        WebCore::RenderLayer::paintLayerContentsAndReflection() [0x7f7e5796b63f]
        WebCore::RenderLayer::paintLayer() [0x7f7e5796ac36]
        WebCore::RenderLayer::paintList() [0x7f7e5796d3c5]
        WebCore::RenderLayer::paintLayerContents() [0x7f7e5796c58d]
        WebCore::RenderLayer::paintLayerContentsAndReflection() [0x7f7e5796b63f]
        WebCore::RenderLayer::paintLayer() [0x7f7e5796ac36]
        WebCore::RenderLayer::paintList() [0x7f7e5796d3c5]
        WebCore::RenderLayer::paintLayerContents() [0x7f7e5796c5fb]
        WebCore::RenderLayer::paintLayerContentsAndReflection() [0x7f7e5796b63f]
        WebCore::RenderLayer::paintLayer() [0x7f7e5796ac36]
        WebCore::RenderLayer::paint() [0x7f7e5796a49d]
        WebCore::FrameView::paintContents() [0x7f7e58865327]
        WebCore::ScrollView::paint() [0x7f7e57eba158]
        WebKit::PageWidgetDelegate::paint() [0x7f7e573f0956]
        WebKit::WebViewImpl::paint() [0x7f7e574b55eb]
        content::RenderWidget::PaintRect() [0x7f7e55d97da6]
        content::RenderWidget::DoDeferredUpdate() [0x7f7e55d9312d]
        content::RenderWidget::DoDeferredUpdateAndSendInputAck() [0x7f7e55d97319]
        content::RenderWidget::InvalidationCallback() [0x7f7e55d98a7a]

Do you know which of the calls in the chain going from things named paint* to setNeedsLayout() is inappropriate?

For the record, my repro cmdline was:
../ninja -k0 content_browsertests && xvfb-run ./ninja/Debug/content_browsertests --gtest_filter=ExternalClearKey/EncryptedMediaTest.FrameChangeVideo/0 --no-sandbox --gtest_repeat=100
and I was shutting off all but one of my machine's cores with:
cd /sys/devices/system/cpu && for c in cpu[1-9]*; do echo 0|sudo dd of=$c/online 2> /dev/null & done && wait && grep processor /proc/cpuinfo |wc -l
and then keeping that one core semi-distracted with md5sum /boot/vml*
It only took a handful of iterations of the test to make it fail w/ the !needsLayout() assert originally reported in this bug, but it saw no failures with the setNeedsLayout() commented out in RenderVideo::updateIntrinsicSize().
------- Comment #9 From 2012-10-24 10:20:38 PST -------
Thanks for the stack trace!  The call to updatePlayer in RenderVideo::paintReplaced() seems incorrect.  I filed bug 100265 for this.
------- Comment #10 From 2012-10-24 12:27:10 PST -------
If it might help with debugging, using gmail on a recent Chrome debug build trips on this bug and crash the render process quite easily...
------- Comment #11 From 2012-10-24 13:00:46 PST -------
(In reply to comment #10)
> If it might help with debugging, using gmail on a recent Chrome debug build trips on this bug and crash the render process quite easily...

That's a similar but different crash; filed bug 100282 for it.