Bug 78038

Summary: [Chromium] Assertion failure minX <= maxX in Region.cpp
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Dana Jansens <danakj>
Severity: Normal CC: andersca, cc-bugs, danakj, enne, jamesr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch for landing none

Description Julien Chaffraix 2012-02-07 14:01:30 PST
The ASSERT is in Region.cpp - line 214:

ASSERT(minX <= maxX);

It seems to be hit frequently by our bots on test following test:


I don't know if it's a Chromium only bug.
Comment 1 James Robinson 2012-02-07 14:13:02 PST
Can we get a full stack or link to failing runs?
Comment 2 Anders Carlsson 2012-02-07 14:24:32 PST
It would also be helpful if we could figure out which Region operations lead up to this invalid region being created.
Comment 3 Dana Jansens 2012-02-07 14:34:42 PST
I managed to get this running invisible-nested-iframe-show.html enough times (7 in a row seems to be a magic number):

[1522:1522:530403859202:ERROR:process_util_posix.cc(142)] Received signal 11
	base::debug::StackTrace::StackTrace() [0x7fe70a800c06]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x7fe70a85e55d]
	WebCore::Region::Shape::bounds() [0x7fe70660409f]
	WebCore::Region::subtract() [0x7fe706604595]
	WebCore::TiledLayerChromium::updateBounds() [0x7fe70663b354]

When I print out the two rects going into Regions in updateBounds() I see the following:
old: 0 0 new: 800 600
old: 880 1879 new: 0 0
old: 0 0 new: 15 600
old: 0 0 new: 800 600
old: 0 0 new: 800 600
old: 0 0 new: 800 600
old: 0 0 new: 210 210
old: 0 0 new: 250 170
old: 0 0 new: 250 230
old: 0 0 new: 250 230
old: 0 0 new: 210 210
old: 0 0 new: 260 193
old: 0 0 new: -15 15

-15 isn't a good size width. contentBounds() is wrong here in this case.
Comment 4 Dana Jansens 2012-02-07 14:41:53 PST
Just in case it's of interest and others have difficulty reproing, here is the full backtrace.

Summary: Region seems okay, though asserting sooner on negative widths might be useful.

[3526:3526:531217440850:ERROR:process_util_posix.cc(142)] Received signal 11
	base::debug::StackTrace::StackTrace() [0x7f6fd8a6bc06]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x7f6fd8ac955d]
	WebCore::Region::Shape::bounds() [0x7f6fd486f30f]
	WebCore::Region::subtract() [0x7f6fd486f805]
	WebCore::TiledLayerChromium::updateBounds() [0x7f6fd48a662f]
	WebCore::TiledLayerChromium::invalidateRect() [0x7f6fd48a75c7]
	WebCore::TiledLayerChromium::setNeedsDisplayRect() [0x7f6fd48a753d]
	WebCore::LayerChromium::setNeedsDisplay() [0x7f6fd3dfb03c]
	WebCore::LayerChromium::setBounds() [0x7f6fd4887302]
	WebCore::GraphicsLayerChromium::updateLayerSize() [0x7f6fd4883d58]
	WebCore::GraphicsLayerChromium::setSize() [0x7f6fd4882e24]
	WebCore::positionScrollbarLayer() [0x7f6fd47df99a]
	WebCore::ScrollView::positionScrollbarLayers() [0x7f6fd47dfc00]
	WebCore::RenderLayerCompositor::updateOverflowControlsLayers() [0x7f6fd424d7ef]
	WebCore::RenderLayerCompositor::frameViewDidChangeSize() [0x7f6fd424ae0d]
	WebCore::RenderLayerCompositor::ensureRootLayer() [0x7f6fd424dd68]
	WebCore::RenderLayerCompositor::enableCompositingMode() [0x7f6fd4247bc5]
	WebCore::RenderLayerCompositor::updateBacking() [0x7f6fd42485e4]
	WebCore::RenderLayerCompositor::computeCompositingRequirements() [0x7f6fd424a416]
	WebCore::RenderLayerCompositor::computeCompositingRequirements() [0x7f6fd4249f63]
	WebCore::RenderLayerCompositor::computeCompositingRequirements() [0x7f6fd424a00a]
	WebCore::RenderLayerCompositor::updateCompositingLayers() [0x7f6fd42483a9]
	WebCore::FrameView::updateCompositingLayers() [0x7f6fd4e2753a]
	WebCore::FrameView::layout() [0x7f6fd4e28c5f]
	WebCore::FrameView::layoutTimerFired() [0x7f6fd4e2bb99]
	WebCore::Timer<>::fired() [0x7f6fd4e35728]
	WebCore::ThreadTimers::sharedTimerFiredInternal() [0x7f6fd47ede30]
	WebCore::ThreadTimers::sharedTimerFired() [0x7f6fd47edd67]
	webkit_glue::WebKitPlatformSupportImpl::DoTimeout() [0x7f6fd8eae30c]
	base::BaseTimer<>::TimerTask::Run() [0x7f6fd8eaeb7f]
	base::internal::RunnableAdapter<>::Run() [0x7f6fd8b07f61]
	base::internal::InvokeHelper<>::MakeItSo() [0x7f6fd8b07ee5]
	base::internal::Invoker<>::Run() [0x7f6fd8b07e91]
	base::Callback<>::Run() [0x7f6fd8a64267]
	MessageLoop::RunTask() [0x7f6fd8aa0710]
	MessageLoop::DeferOrRunPendingTask() [0x7f6fd8aa0827]
	MessageLoop::DoWork() [0x7f6fd8aa1049]
	base::MessagePumpGlib::RunWithDispatcher() [0x7f6fd8a47738]
	base::MessagePumpGlib::Run() [0x7f6fd8a47b18]
	MessageLoop::RunInternal() [0x7f6fd8aa03cb]
	MessageLoop::RunHandler() [0x7f6fd8aa027e]
	MessageLoop::Run() [0x7f6fd8a9fbb3]
	webkit_support::RunMessageLoop() [0x57299c]
	TestShell::waitTestFinished() [0x53826e]
	TestShell::runFileTest() [0x5308f9]
	runTest() [0x4fa214]
	main [0x4facd6]
Comment 5 Julien Chaffraix 2012-02-07 14:52:41 PST
FYI, the traces on the bot are the same as in comment #4.
Comment 6 Dana Jansens 2012-02-07 15:39:05 PST
When I run ntyimes in a debugger I end up seeing this assertion as well. In this case the size in RenderLayerBacking::m_compositedBounds is bad:

(gdb) print m_compositedBounds  
$7 = {m_location = {m_x = 0, m_y = 0}, m_size = {m_width = -180, m_height = 930}}

And this comes all the way in through the GraphicsLayer to cause this ASSERT.
Comment 7 James Robinson 2012-02-09 14:07:27 PST
We should clamp the rects somewhere or revert the change to pipe these rects into regions, this is causing an unacceptable number of ASSERT failures on the bots and with interactive debugging.
Comment 8 Dana Jansens 2012-02-09 14:14:12 PST
Would it be wrong to clamp them in GraphicsLayerChromium?
What do you think about the fact this hides the problem in WebCore for others to discover somehow?
Comment 9 James Robinson 2012-02-09 14:17:24 PST
(In reply to comment #8)
> Would it be wrong to clamp them in GraphicsLayerChromium?

I don't know.  If in doubt, we should do something conservative (i.e. doesn't break rendering).

> What do you think about the fact this hides the problem in WebCore for others to discover somehow?

I don't see how making tests crash and debugging impossible helps anyone.
Comment 10 Dana Jansens 2012-02-09 14:30:33 PST
Created attachment 126374 [details]
Comment 11 James Robinson 2012-02-09 14:37:04 PST
Comment on attachment 126374 [details]

I think checking for size.isEmpty() and setting size to (0, 0) would be slightly preferable - I'm not sure I want to "grow" a -50,50 layer to 0,50, or if that'll have any effect further down the pipe.
Comment 12 Dana Jansens 2012-02-09 14:44:30 PST
Created attachment 126377 [details]
Comment 13 Dana Jansens 2012-02-09 14:48:33 PST
Comment on attachment 126377 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=126377&action=review

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:179
> +    // avoid assertions in the compositor.

ill fix that redundant comment before landing
Comment 14 James Robinson 2012-02-09 14:53:00 PST
Comment on attachment 126377 [details]

OK thanks, I think this will work for now. Can you file a bug to investigate if we need to do anything more on the RenderLayerBacking or earlier phases? I have some theories but haven't investigated them on these particular test cases.
Comment 16 Fady Samuel 2012-02-09 15:32:47 PST
Created attachment 126389 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-02-09 22:08:54 PST
Comment on attachment 126389 [details]
Patch for landing

Clearing flags on attachment: 126389

Committed r107360: <http://trac.webkit.org/changeset/107360>
Comment 18 WebKit Review Bot 2012-02-09 22:08:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Adrienne Walker 2012-03-14 11:33:33 PDT
Committed r110719: <http://trac.webkit.org/changeset/110719>