Summary: | Overflow regions sometimes repaint incorrectly after going into or coming out of compositing mode | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | 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
Beth Dakin
2012-08-07 14:11:09 PDT
Created attachment 157005 [details]
Patch
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.
Created attachment 157006 [details]
Patch to make style bot happy
Seems reasonable, but Simon's your man here. Can you extend compositing/repaint/newly-composited-repaint-rect.html (or make a new test) to cover this issue? So far I have been unsuccessful at creating a test for this bug, but I will try a few more things. 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 on attachment 157006 [details]
Patch to make style bot happy
Ooops, r- for lack of a test.
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.
Created attachment 157287 [details]
Patch with test
Thanks Simon! http://trac.webkit.org/changeset/125086 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 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.
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. Oh, actually it's bogus to compute repaint rects inside of styleChanged because we haven't done layout yet. Created attachment 157319 [details]
Crash fix
I filed 93542 for a longer-term fix. 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. Crash fixed: http://trac.webkit.org/changeset/125104 |