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
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-21 21:01 PDT by Ami Fischman
Modified: 2012-10-24 13:00 PDT (History)
12 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ami Fischman 2012-06-21 21:01:42 PDT
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 Alexey Proskuryakov 2012-06-22 09:52:48 PDT
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 Simon Fraser (smfr) 2012-06-22 11:05:07 PDT
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 Ami Fischman 2012-06-22 21:39:08 PDT
(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 Ami Fischman 2012-10-23 15:26:47 PDT
@simon.fraser: ping.  Any ideas about comment #3?
Comment 5 Simon Fraser (smfr) 2012-10-23 15:30:00 PDT
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 Dana Jansens 2012-10-23 17:34:23 PDT
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 James Robinson 2012-10-23 17:41:41 PDT
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 Ami Fischman 2012-10-24 00:00:45 PDT
@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 Tony Chang 2012-10-24 10:20:38 PDT
Thanks for the stack trace!  The call to updatePlayer in RenderVideo::paintReplaced() seems incorrect.  I filed bug 100265 for this.
Comment 10 Marc-André Decoste 2012-10-24 12:27:10 PDT
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 Ami Fischman 2012-10-24 13:00:46 PDT
(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.