Bug 93393 – Overflow regions sometimes repaint incorrectly after going into or coming out of compositing mode

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

[Beth Dakin]

Overflow regions sometimes repaint incorrectly after going into or coming out of compositing mode. We have seen this very infrequently at bloglines.com, and we have seen it more frequently on an internal Apple site.

<rdar://problem/12006463>

[Beth Dakin]

Created attachment 157005 [details]
Patch

[WebKit Review Bot]

Attachment 157005 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderLayer.cpp:493:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 157006 [details]
Patch to make style bot happy

[Eric Seidel (no email)]

Seems reasonable, but Simon's your man here.

[Adrienne Walker]

Can you extend compositing/repaint/newly-composited-repaint-rect.html (or make a new test) to cover this issue?

[Beth Dakin]

So far I have been unsuccessful at creating a test for this bug, but I will try a few more things.

[Simon Fraser (smfr)]

Comment on attachment 157006 [details]
Patch to make style bot happy

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

> Source/WebCore/rendering/RenderLayer.cpp:494
> +    computeRepaintRects();
> +    
> +    for (RenderLayer* layer = firstChild(); layer; layer = layer->nextSibling())
> +        layer->computeRepaintRectsIncludingDescendants();

I think this deserves a FIXME comment about how computeRepaintRects() has to walk up the parent chain for every layer to compute the rects.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:537
> +        // The layer's cached repaints rects are relative to the repaint container, so change when

Not just this layer, but for all descendant layers for which this layer is a repaint container.

Which suggests that we could possibly optimize this; computeRepaintRectsIncludingDescendants() could avoid subtrees which are themselves compositing (taking care to deal with the fact that it's walking parent/child layers, not in z-order). But I think that could be done in a later patch.

[Simon Fraser (smfr)]

Comment on attachment 157006 [details]
Patch to make style bot happy

Ooops, r- for lack of a test.

[Beth Dakin]

Created attachment 157120 [details]
Test case

Here's a reduction. To reproduce the bug, you have to make the window small enough so that the green box is entirely out of view. Then scroll, and the green box will come only partially into view.

I will try to turn this into a repaint test tomorrow.

[Beth Dakin]

Created attachment 157287 [details]
Patch with test

[Beth Dakin]

Thanks Simon! http://trac.webkit.org/changeset/125086

[Florin Malita]

This is triggering an assert with fast/table/table-row-compositing-repaint-crash.html on Chromium:

STDERR: ASSERTION FAILED: parent()
STDERR: ../../third_party/WebKit/Source/WebCore/rendering/RenderTableRow.cpp(181) : virtual WebCore::LayoutRect WebCore::RenderTableRow::clippedOverflowRectForRepaint(WebCore::RenderBoxModelObject*) const
STDERR: 1   0x7ffce8a9680a
STDERR: 2   0x7ffce89f97d3
STDERR: 3   0x7ffce89f984d
STDERR: 4   0x7ffce8a25706
STDERR: 5   0x7ffce8a257ab
STDERR: 6   0x7ffce8a0fe3f
STDERR: 7   0x7ffce8991ae7
STDERR: 8   0x7ffce89720cd
STDERR: 9   0x7ffce8a95e0e
STDERR: 10  0x7ffce8a6183c
STDERR: 11  0x7ffce8a610bd
STDERR: 12  0x7ffce8bf99db
STDERR: 13  0x7ffce8bf9c86
STDERR: 14  0x7ffce8be4859
STDERR: 15  0x7ffce8b9a1d3
STDERR: 16  0x7ffce9aee5c8
STDERR: 17  0x7ffce9aee821
STDERR: 18  0x7ffce9b11f61
STDERR: 19  0x7ffce9b11d7f
STDERR: 20  0x7ffce9af4be8
STDERR: 21  0x7ffce9af4630
STDERR: 22  0x7ffce9af520e
STDERR: 23  0x7ffce8b36cd8
STDERR: 24  0x7ffce9758fe0
STDERR: 25  0x7ffce9748f41
STDERR: 26  0x7ffce806900e
STDERR: 27  0x7ffce800c9b9
STDERR: 28  0x7ffce9748c78
STDERR: 29  0x7ffce9749171
STDERR: 30  0x7ffce9780f07
STDERR: 31  0x7ffce9795d67
STDERR: [20306:20306:461556515680:ERROR:process_util_posix.cc(143)] Received signal 11
STDERR: 	base::debug::StackTrace::StackTrace() [0x7ffcedd19326]
STDERR: 	base::(anonymous namespace)::StackDumpSignalHandler() [0x7ffcedd7e8cd]
STDERR: 	0x7ffce1dafaf0
STDERR: 	WebCore::RenderTableRow::clippedOverflowRectForRepaint() [0x7ffce8a96814]
STDERR: 	WebCore::RenderLayer::computeRepaintRects() [0x7ffce89f97d3]
STDERR: 	WebCore::RenderLayer::computeRepaintRectsIncludingDescendants() [0x7ffce89f984d]
STDERR: 	WebCore::RenderLayerCompositor::updateBacking() [0x7ffce8a25706]
STDERR: 	WebCore::RenderLayerCompositor::updateLayerCompositingState() [0x7ffce8a257ab]
STDERR: 	WebCore::RenderLayer::styleChanged() [0x7ffce8a0fe3f]
STDERR: 	WebCore::RenderBoxModelObject::styleDidChange() [0x7ffce8991ae7]
STDERR: 	WebCore::RenderBox::styleDidChange() [0x7ffce89720cd]
STDERR: 	WebCore::RenderTableRow::styleDidChange() [0x7ffce8a95e0e]
STDERR: 	WebCore::RenderObject::setStyle() [0x7ffce8a6183c]
STDERR: 	WebCore::RenderObject::setAnimatableStyle() [0x7ffce8a610bd]
STDERR: 	WebCore::NodeRendererFactory::createRenderer() [0x7ffce8bf99db]
STDERR: 	WebCore::NodeRendererFactory::createRendererIfNeeded() [0x7ffce8bf9c86]
STDERR: 	WebCore::Node::createRendererIfNeeded() [0x7ffce8be4859]
STDERR: 	WebCore::Element::attach() [0x7ffce8b9a1d3]
STDERR: 	WebCore::executeTask() [0x7ffce9aee5c8]
STDERR: 	WebCore::HTMLConstructionSite::executeQueuedTasks() [0x7ffce9aee821]
STDERR: 	WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken() [0x7ffce9b11f61]
STDERR: 	WebCore::HTMLTreeBuilder::constructTreeFromToken() [0x7ffce9b11d7f]
STDERR: 	WebCore::HTMLDocumentParser::pumpTokenizer() [0x7ffce9af4be8]
STDERR: 	WebCore::HTMLDocumentParser::pumpTokenizerIfPossible() [0x7ffce9af4630]
STDERR: 	WebCore::HTMLDocumentParser::append() [0x7ffce9af520e]
STDERR: 	WebCore::DecodedDataDocumentParser::appendBytes() [0x7ffce8b36cd8]
STDERR: 	WebCore::DocumentWriter::addData() [0x7ffce9758fe0]
STDERR: 	WebCore::DocumentLoader::commitData() [0x7ffce9748f41]
STDERR: 	WebKit::WebFrameImpl::commitDocumentData() [0x7ffce806900e]
STDERR: 	WebKit::FrameLoaderClientImpl::committedLoad() [0x7ffce800c9b9]
STDERR: 	WebCore::DocumentLoader::commitLoad() [0x7ffce9748c78]
STDERR: 	WebCore::DocumentLoader::receivedData() [0x7ffce9749171]
STDERR: 	WebCore::MainResourceLoader::addData() [0x7ffce9780f07]
STDERR: 	WebCore::ResourceLoader::didReceiveData() [0x7ffce9795d67]
STDERR: 	WebCore::MainResourceLoader::didReceiveData() [0x7ffce97823b4]
STDERR: 	WebCore::ResourceLoader::didReceiveData() [0x7ffce979664f]
STDERR: 	WebCore::ResourceHandleInternal::didReceiveData() [0x7ffce919795e]
STDERR: 	webkit_glue::WebURLLoaderImpl::Context::OnReceivedData() [0x7ffcee0de90c]
STDERR: 	(anonymous namespace)::RequestProxy::NotifyReceivedData() [0x5da6c7]
STDERR: 	base::internal::RunnableAdapter<>::Run() [0x5e0f79]
STDERR: 	base::internal::InvokeHelper<>::MakeItSo() [0x5e0942]
STDERR: 	base::internal::Invoker<>::Run() [0x5e01e0]
STDERR: 	base::Callback<>::Run() [0x7ffcedd114c9]
STDERR: 	MessageLoop::RunTask() [0x7ffcedd52fc6]
STDERR: 	MessageLoop::DeferOrRunPendingTask() [0x7ffcedd530de]
STDERR: 	MessageLoop::DoWork() [0x7ffcedd538f9]
STDERR: 	base::MessagePumpGlib::RunWithDispatcher() [0x7ffcedcf73ee]
STDERR: 	base::MessagePumpGlib::Run() [0x7ffcedcf77ce]
STDERR: 	MessageLoop::RunInternal() [0x7ffcedd52c8b]
STDERR: 	MessageLoop::RunHandler() [0x7ffcedd52b42]
STDERR: 	base::RunLoop::Run() [0x7ffcedd8651a]
STDERR: 	MessageLoop::Run() [0x7ffcedd52470]
STDERR: 	webkit_support::RunMessageLoop() [0x559503]
STDERR: 	TestShell::waitTestFinished() [0x4c1b82]
STDERR: 	TestShell::runFileTest() [0x4bac44]
STDERR: 	runTest() [0x486bf2]
STDERR: 	main [0x4875b3]
STDERR: 	0x7ffce1d9ac4d
STDERR: 	0x4853b9

[Florin Malita]

Created attachment 157309 [details]
newly-composited-on-scroll.html result for Chromium

Also, the CR result for the new test looks different (full repaint?) - see attached.

[Beth Dakin]

I removed a check for if (parent()) in RenderLayerCompositor because Simon wasn't sure why that was there. Maybe this assert is what it was there for.

[Simon Fraser (smfr)]

Oh, actually it's bogus to compute repaint rects inside of styleChanged because we haven't done layout yet.

[Beth Dakin]

Created attachment 157319 [details]
Crash fix

[Simon Fraser (smfr)]

I filed 93542 for a longer-term fix.

[Simon Fraser (smfr)]

Comment on attachment 157319 [details]
Crash fix

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

> Source/WebCore/ChangeLog:13
> +        My first patch to fix this bug removed an if (parent()) check that is 
> +        needed to prevent a table crash. This patch adds that check back, but 
> +        really we should delay the computation of repaint rects if layout has 

I think it would be good if this text referenced the test that was crashing, and changeset that added the origina if (parent()) check.

[Beth Dakin]

Crash fixed: http://trac.webkit.org/changeset/125104