Bug 89733 – Figure out why FrameView.cpp's ASSERT(!needsLayout()) is firing in chromium & chromium's DRT

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Ami Fischman]

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

[Alexey Proskuryakov]

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.

[Simon Fraser (smfr)]

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

[Ami Fischman]

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

[Ami Fischman]

@simon.fraser: ping.  Any ideas about comment #3?

[Simon Fraser (smfr)]

Not really. I'd think that TiledLayerChromium.cpp or something at that level should make sure that layout is up-to-date before painting.

[Dana Jansens]

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?

[James Robinson]

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.

[Ami Fischman]

@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().

[Tony Chang]

Thanks for the stack trace!  The call to updatePlayer in RenderVideo::paintReplaced() seems incorrect.  I filed bug 100265 for this.

[Marc-André Decoste]

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

[Ami Fischman]

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