Bug 93393

Summary: Overflow regions sometimes repaint incorrectly after going into or coming out of compositing mode
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: Layout and RenderingAssignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, enne, eric, fmalita, jamesr, simon.fraser, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch to make style bot happy
simon.fraser: review-
Test case
none
Patch with test
simon.fraser: review+
newly-composited-on-scroll.html result for Chromium
none
Crash fix simon.fraser: review+

Description Beth Dakin 2012-08-07 14:11:09 PDT
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>
Comment 1 Beth Dakin 2012-08-07 14:26:11 PDT
Created attachment 157005 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-07 14:29:30 PDT
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.
Comment 3 Beth Dakin 2012-08-07 14:33:07 PDT
Created attachment 157006 [details]
Patch to make style bot happy
Comment 4 Eric Seidel (no email) 2012-08-07 14:39:43 PDT
Seems reasonable, but Simon's your man here.
Comment 5 Adrienne Walker 2012-08-07 14:45:55 PDT
Can you extend compositing/repaint/newly-composited-repaint-rect.html (or make a new test) to cover this issue?
Comment 6 Beth Dakin 2012-08-07 15:29:55 PDT
So far I have been unsuccessful at creating a test for this bug, but I will try a few more things.
Comment 7 Simon Fraser (smfr) 2012-08-07 16:07:08 PDT
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.
Comment 8 Simon Fraser (smfr) 2012-08-07 16:12:01 PDT
Comment on attachment 157006 [details]
Patch to make style bot happy

Ooops, r- for lack of a test.
Comment 9 Beth Dakin 2012-08-07 22:45:52 PDT
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.
Comment 10 Beth Dakin 2012-08-08 14:01:26 PDT
Created attachment 157287 [details]
Patch with test
Comment 11 Beth Dakin 2012-08-08 14:10:29 PDT
Thanks Simon! http://trac.webkit.org/changeset/125086
Comment 12 Florin Malita 2012-08-08 15:26:31 PDT
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
Comment 13 Florin Malita 2012-08-08 15:28:13 PDT
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.
Comment 14 Beth Dakin 2012-08-08 15:32:35 PDT
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.
Comment 15 Simon Fraser (smfr) 2012-08-08 15:33:34 PDT
Oh, actually it's bogus to compute repaint rects inside of styleChanged because we haven't done layout yet.
Comment 16 Beth Dakin 2012-08-08 16:04:11 PDT
Created attachment 157319 [details]
Crash fix
Comment 17 Simon Fraser (smfr) 2012-08-08 16:05:13 PDT
I filed 93542 for a longer-term fix.
Comment 18 Simon Fraser (smfr) 2012-08-08 16:06:11 PDT
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.
Comment 19 Beth Dakin 2012-08-08 16:10:09 PDT
Crash fixed: http://trac.webkit.org/changeset/125104