Bug 106068 – Assert in RenderGeometryMap::mapToContainer

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

There's an assert in RenderGeometryMap::mapToContainer when opening the attached test case.

I've replaced the assert with the following code:

    if (enclosingIntRect(rendererMappedResult) != enclosingIntRect(FloatQuad(result).boundingBox())) {
        IntRect initialRect = enclosingIntRect(rect);
        IntRect rect1 = enclosingIntRect(rendererMappedResult);
        IntRect rect2 = enclosingIntRect(FloatQuad(result).boundingBox());
        fprintf(stderr, "Initial rects 1: %d %d %d %d\n", initialRect.x(), initialRect.y(), initialRect.width(), initialRect.height());
        fprintf(stderr, "Mismatched rects 1: %d %d %d %d\n            2: %d %d %d %d\n", rect1.x(), rect1.y(), rect1.width(), rect1.height(),
                rect2.x(), rect2.y(), rect2.width(), rect2.height());
        m_mapping.last().m_renderer->showRenderTreeForThis();
    }

and here is what I get:

Initial rects 1: 0 0 0 0
Mismatched rects 1: 123 123 0 0
            2: 0 0 0 0
RenderView 0x7fcfe4b06e98              	#document	0x7fcfe6002a00
  RenderBlock 0x7fcfe4b0da88           	HTML	0x7fcfe4b0b320
    RenderBody 0x7fcfe4b0ddc8          	BODY	0x7fcfe4b0d4a0
      RenderBlock (relative positioned) 0x7fcfe4b108d8	DIV	0x7fcfe4b0d510
*       RenderBlock 0x7fcfe4b0c9e8     	DIV	0x7fcfe4b0d620

[Alexandru Chiculita]

Created attachment 181252 [details]
testcase

[Alexandru Chiculita]

It looks like in RenderLayerCompositor::computeCompositingRequirements the absBounds are only computed if (overlapMap && !overlapMap->isEmpty() && compositingState.m_testingOverlap) is true.

However, the "empty" absBounds are later used to call addToOverlapMap and triggers that assert.

[Alexandru Chiculita]

Looks like the layer requirements are calculated before a layout was ever done. m_everHadLayout is 0 on the renderer that reproduces the problem. This should have no visual issues in practice as a layout will follow anyway and that will fix any eventual issues.

This is the stack I see when that happens:

#0  WebCore::RenderGeometryMap::mapToContainer (this=0x7fff58576cc0, rect=@0x7fff58576438, container=0x0) at Source/WebCore/rendering/RenderGeometryMap.cpp:127
#1  0x000000010ae3d6c9 in WebCore::RenderGeometryMap::absoluteRect (this=0x7fff58576cc0, rect=@0x7fff58576438) at RenderGeometryMap.h:89
#2  0x000000010ae7eec0 in WebCore::RenderLayerCompositor::addToOverlapMap (this=0x7fe89ea10690, overlapMap=@0x7fff58576c80, layer=0x7fe89ea177a8, layerBounds=@0x7fff58576610, boundsComputed=@0x7fff58576623) at Source/WebCore/rendering/RenderLayerCompositor.cpp:736
#3  0x000000010ae7caf5 in WebCore::RenderLayerCompositor::computeCompositingRequirements (this=0x7fe89ea10690, ancestorLayer=0x7fe89ea17128, layer=0x7fe89ea177a8, overlapMap=0x7fff58576c80, compositingState=@0x7fff58576788, layersChanged=@0x7fff58577107, descendantHas3DTransform=@0x7fff58576737) at Source/WebCore/rendering/RenderLayerCompositor.cpp:919
#4  0x000000010ae7c9c6 in WebCore::RenderLayerCompositor::computeCompositingRequirements (this=0x7fe89ea10690, ancestorLayer=0x7fe89ea15388, layer=0x7fe89ea17128, overlapMap=0x7fff58576c80, compositingState=@0x7fff58576958, layersChanged=@0x7fff58577107, descendantHas3DTransform=@0x7fff58576907) at Source/WebCore/rendering/RenderLayerCompositor.cpp:902
#5  0x000000010ae7c9c6 in WebCore::RenderLayerCompositor::computeCompositingRequirements (this=0x7fe89ea10690, ancestorLayer=0x7fe89ea10568, layer=0x7fe89ea15388, overlapMap=0x7fff58576c80, compositingState=@0x7fff58576b28, layersChanged=@0x7fff58577107, descendantHas3DTransform=@0x7fff58576ad7) at Source/WebCore/rendering/RenderLayerCompositor.cpp:902
#6  0x000000010ae7c9c6 in WebCore::RenderLayerCompositor::computeCompositingRequirements (this=0x7fe89ea10690, ancestorLayer=0x0, layer=0x7fe89ea10568, overlapMap=0x7fff58576c80, compositingState=@0x7fff58577108, layersChanged=@0x7fff58577107, descendantHas3DTransform=@0x7fff58577106) at Source/WebCore/rendering/RenderLayerCompositor.cpp:902
#7  0x000000010ae7bed8 in WebCore::RenderLayerCompositor::updateCompositingLayers (this=0x7fe89ea10690, updateType=WebCore::CompositingUpdateAfterLayout, updateRoot=0x7fe89ea10568) at Source/WebCore/rendering/RenderLayerCompositor.cpp:461
#8  0x000000010a1f66dd in WebCore::FrameView::updateCompositingLayersAfterLayout (this=0x7fe89ea0edf0) at Source/WebCore/page/FrameView.cpp:744
#9  0x0000000109ea9de0 in WebCore::Document::setVisualUpdatesAllowed (this=0x7fe89f801200, visualUpdatesAllowed=true) at Source/WebCore/dom/Document.cpp:1259
#10 0x0000000109ea9d01 in WebCore::Document::setVisualUpdatesAllowed (this=0x7fe89f801200, readyState=WebCore::Document::Complete) at Source/WebCore/dom/Document.cpp:1240
#11 0x0000000109ea9a4e in WebCore::Document::setReadyState (this=0x7fe89f801200, readyState=WebCore::Document::Complete) at Source/WebCore/dom/Document.cpp:1220
#12 0x000000010a1ce44a in WebCore::FrameLoader::checkCompleted (this=0x7fe8a0013c80) at Source/WebCore/loader/FrameLoader.cpp:773
#13 0x000000010a1cd123 in WebCore::FrameLoader::finishedParsing (this=0x7fe8a0013c80) at Source/WebCore/loader/FrameLoader.cpp:709
#14 0x0000000109eb7399 in WebCore::Document::finishedParsing (this=0x7fe89f801200) at Source/WebCore/dom/Document.cpp:4397
#15 0x000000010a3c3fa1 in WebCore::HTMLTreeBuilder::finished (this=0x7fe89ea141d0) at Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2906
#16 0x000000010a2f28dc in WebCore::HTMLDocumentParser::end (this=0x7fe89f80e400) at Source/WebCore/html/parser/HTMLDocumentParser.cpp:372
#17 0x000000010a2f1a1f in WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd (this=0x7fe89f80e400) at Source/WebCore/html/parser/HTMLDocumentParser.cpp:381
#18 0x000000010a2f1810 in WebCore::HTMLDocumentParser::prepareToStopParsing (this=0x7fe89f80e400) at Source/WebCore/html/parser/HTMLDocumentParser.cpp:149
#19 0x000000010a2f2933 in WebCore::HTMLDocumentParser::attemptToEnd (this=0x7fe89f80e400) at Source/WebCore/html/parser/HTMLDocumentParser.cpp:393
#20 0x000000010a2f2988 in WebCore::HTMLDocumentParser::finish (this=0x7fe89f80e400) at Source/WebCore/html/parser/HTMLDocumentParser.cpp:420
#21 0x0000000109f32228 in WebCore::DocumentWriter::end (this=0x7fe89b8190a8) at Source/WebCore/loader/DocumentWriter.cpp:244
#22 0x0000000109eff809 in WebCore::DocumentLoader::finishedLoading (this=0x7fe89b819000) at Source/WebCore/loader/DocumentLoader.cpp:295
#23 0x000000010ab878e0 in WebCore::MainResourceLoader::didFinishLoading (this=0x7fe89b46cec0, finishTime=0) at Source/WebCore/loader/MainResourceLoader.cpp:543
#24 0x000000010ab887c7 in WebCore::MainResourceLoader::notifyFinished (this=0x7fe89b46cec0, resource=0x7fe89b458660) at Source/WebCore/loader/MainResourceLoader.cpp:552
#25 0x0000000109be998d in WebCore::CachedResource::checkNotify (this=0x7fe89b458660) at Source/WebCore/loader/cache/CachedResource.cpp:336
#26 0x0000000109be99f5 in WebCore::CachedResource::data (this=0x7fe89b458660, allDataReceived=true) at Source/WebCore/loader/cache/CachedResource.cpp:345
#27 0x0000000109be4ed1 in WebCore::CachedRawResource::data (this=0x7fe89b458660, data=@0x7fff585776b0, allDataReceived=true) at Source/WebCore/loader/cache/CachedRawResource.cpp:72
#28 0x000000010b252d1b in WebCore::SubresourceLoader::didFinishLoading (this=0x7fe89b819a00, finishTime=0) at Source/WebCore/loader/SubresourceLoader.cpp:276
#29 0x000000010b010535 in WebCore::ResourceLoader::didFinishLoading (this=0x7fe89b819a00, finishTime=0) at Source/WebCore/loader/ResourceLoader.cpp:457
#30 0x000000010b00d05a in -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] (self=0x7fe89b41f710, _cmd=0x7fff882a73f4, connection=0x7fe89b463d20) at Source/WebCore/platform/network/mac/ResourceHandleMac.mm:835
#31 0x00007fff8b2cbf58 in __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke_0 ()
#32 0x00007fff8b2cbe9c in -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] ()
#33 0x00007fff8b2cbd98 in -[NSURLConnectionInternal _withActiveConnectionAndDelegate:] ()
#34 0x00007fff8d459f01 in ___delegate_didFinishLoading_block_invoke_0 ()
#35 0x00007fff8d44c3ca in ___withDelegateAsync_block_invoke_0 ()
#36 0x00007fff8d4dc56a in __block_global_1 ()
#37 0x00007fff8bb45724 in CFArrayApplyFunction ()
#38 0x00007fff8d43d554 in RunloopBlockContext::perform ()
#39 0x00007fff8d43d42b in MultiplexerSource::perform ()
#40 0x00007fff8bb27101 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ ()
#41 0x00007fff8bb26a25 in __CFRunLoopDoSources0 ()
#42 0x00007fff8bb49dc5 in __CFRunLoopRun ()
#43 0x00007fff8bb496b2 in CFRunLoopRunSpecific ()
#44 0x00007fff90e510a4 in RunCurrentEventLoopInMode ()
#45 0x00007fff90e50e42 in ReceiveNextEventCommon ()
#46 0x00007fff90e50cd3 in BlockUntilNextEventMatchingListInMode ()
#47 0x00007fff8c2a5613 in _DPSNextEvent ()
#48 0x00007fff8c2a4ed2 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#49 0x00007fff8c29c283 in -[NSApplication run] ()
#50 0x000000010b046bbc in WebCore::RunLoop::run () at Source/WebCore/platform/mac/RunLoopMac.mm:36
#51 0x0000000107a14cd3 in WebKit::WebProcessMain (commandLine=@0x7fff585795b8) at Source/WebKit2/WebProcess/mac/WebProcessMainMac.mm:187
#52 0x00000001078ff619 in WebKitMain (commandLine=@0x7fff585795b8) at Source/WebKit2/WebProcess/WebKitMain.cpp:58
#53 0x00000001078ff529 in WebKitMain (argc=12, argv=0x7fff58579660) at Source/WebKit2/WebProcess/WebKitMain.cpp:88
#54 0x0000000107686da2 in main (argc=12, argv=0x7fff58579660) at Source/WebKit2/mac/MainMacProcess.cpp:68

[Simon Fraser (smfr)]

(In reply to comment #3)
> Looks like the layer requirements are calculated before a layout was ever done. m_everHadLayout is 0 on the renderer that reproduces the problem. This should have no visual issues in practice as a layout will follow anyway and that will fix any eventual issues.

But it is crazy to rely on overlap rectangles when we haven't been laid out yet! We are in updateCompositingLayersAfterLayout() though, so someone is lying.

[Alexandru Chiculita]

(In reply to comment #4)
> (In reply to comment #3)
> > Looks like the layer requirements are calculated before a layout was ever done. m_everHadLayout is 0 on the renderer that reproduces the problem. This should have no visual issues in practice as a layout will follow anyway and that will fix any eventual issues.
> 
> But it is crazy to rely on overlap rectangles when we haven't been laid out yet! We are in updateCompositingLayersAfterLayout() though, so someone is lying.

I know, I guess for that one the solution is to trigger a layout if needed and only if there was no layout just go ahead update just the compositing layers. 

I've also added an ASSERT in updateCompositingLayers for !m_renderView->needsLayout() and after running the tests I've also found the following case:

ASSERTION FAILED: !m_renderView->needsLayout()
/Source/WebCore/rendering/RenderLayerCompositor.cpp(411) : void WebCore::RenderLayerCompositor::updateCompositingLayers(WebCore::CompositingUpdateType, WebCore::RenderLayer *)
1   0x10ae78bd3 WebCore::RenderLayerCompositor::updateCompositingLayers(WebCore::CompositingUpdateType, WebCore::RenderLayer*)
2   0x10ae4483e WebCore::RenderLayer::updateCompositingLayersAfterScroll()
3   0x10ae44346 WebCore::RenderLayer::scrollTo(int, int)
4   0x10ae46d1e WebCore::RenderLayer::setScrollOffset(WebCore::IntPoint const&)
5   0x10b093bb0 WebCore::ScrollableArea::scrollPositionChanged(WebCore::IntPoint const&)
6   0x10b093ea1 WebCore::ScrollableArea::setScrollOffsetFromAnimation(WebCore::IntPoint const&)
7   0x10b0963eb WebCore::ScrollAnimator::notifyPositionChanged()
8   0x10b09a759 WebCore::ScrollAnimatorMac::notifyPositionChanged()
9   0x10b09a2a2 WebCore::ScrollAnimatorMac::immediateScrollTo(WebCore::FloatPoint const&)
10  0x10b09a1c3 WebCore::ScrollAnimatorMac::scrollToOffsetWithoutAnimation(WebCore::FloatPoint const&)
11  0x10b0939fc WebCore::ScrollableArea::scrollToOffsetWithoutAnimation(WebCore::FloatPoint const&)
12  0x10ae43ba2 WebCore::RenderLayer::scrollToOffset(WebCore::IntSize const&, WebCore::RenderLayer::ScrollOffsetClamping)
13  0x10adbc433 WebCore::RenderLayer::scrollToXOffset(int, WebCore::RenderLayer::ScrollOffsetClamping)
14  0x10aea43ad WebCore::RenderMarquee::timerFired(WebCore::Timer<WebCore::RenderMarquee>*)
15  0x10aea5853 WebCore::Timer<WebCore::RenderMarquee>::fired()
16  0x10b3e37e6 WebCore::ThreadTimers::sharedTimerFiredInternal()
17  0x10b3e3579 WebCore::ThreadTimers::sharedTimerFired()
18  0x10b112543 WebCore::timerFired(__CFRunLoopTimer*, void*)
19  0x7fff8bb64da4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
20  0x7fff8bb648bd __CFRunLoopDoTimer
21  0x7fff8bb4a099 __CFRunLoopRun
22  0x7fff8bb496b2 CFRunLoopRunSpecific
23  0x7fff90e510a4 RunCurrentEventLoopInMode
24  0x7fff90e50e42 ReceiveNextEventCommon
25  0x7fff90e50cd3 BlockUntilNextEventMatchingListInMode
26  0x7fff8c2a5613 _DPSNextEvent
27  0x7fff8c2a4ed2 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
28  0x7fff8c29c283 -[NSApplication run]
29  0x10b043afc WebCore::RunLoop::run()
30  0x107a09563 WebKit::WebProcessMain(WebKit::CommandLine const&)
31  0x1078f3459 WebKitMain(WebKit::CommandLine const&)

[Alexandru Chiculita]

(In reply to comment #5)

BTW, that happens with fast/events/tabindex-focus-blur-all.html.

[Simon Fraser (smfr)]

Marquee has historically been a source of problems because of this.

[Alexandru Chiculita]

Created attachment 181385 [details]
test case for second assert

A simple way to reproduce the second case.

[Alexandru Chiculita]

(In reply to comment #8)
> Created an attachment (id=181385) [details]
> test case for second assert
> 
> A simple way to reproduce the second case.

Looks like replacing one line in RenderMarquee::timerFired fixes the problem. It actually makes more sense too, as having a layout pending on other elements might actually influence the size of this element, too.

- if (m_layer->renderer()->needsLayout())
+ if (m_layer->renderer()->view()->needsLayout())

That's been in there since 2003. Do you mind if I go ahead and make both changes in one patch? Adding that assert forces me to do both.

[Alexandru Chiculita]

Created attachment 181584 [details]
Patch V1

[Alexandru Chiculita]

Comment on attachment 181584 [details]
Patch V1

Looks like I forget to remove a change.

[Alexandru Chiculita]

Created attachment 181587 [details]
Patch V2

[Build Bot]

Comment on attachment 181587 [details]
Patch V2

Attachment 181587 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15731948

New failing tests:
compositing/geometry/assert-marquee-timer.html

[WebKit Review Bot]

Comment on attachment 181587 [details]
Patch V2

Attachment 181587 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15742798

New failing tests:
platform/chromium/virtual/softwarecompositing/geometry/assert-marquee-timer.html
compositing/geometry/assert-marquee-timer.html
platform/chromium/virtual/softwarecompositing/geometry/assert-layout-not-done.html
compositing/geometry/assert-layout-not-done.html

[Alexandru Chiculita]

Created attachment 181797 [details]
Patch V3

Fixed the test. Will set the CQ+ after EWS passes.

[WebKit Review Bot]

Comment on attachment 181797 [details]
Patch V3

Clearing flags on attachment: 181797

Committed r139146: <http://trac.webkit.org/changeset/139146>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Is it possible that this patch caused these flaky assertion failures?

http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r139159%20(5588)/dom/html/level2/html/HTMLDocument08-crash-log.txt

[Alexandru Chiculita]

(In reply to comment #18)
> Is it possible that this patch caused these flaky assertion failures?
> 
> http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r139159%20(5588)/dom/html/level2/html/HTMLDocument08-crash-log.txt

It's the ASSERT I've added in this patch. That test didn't crash on my machine, so that's why I didn't fix that case too.

I think that this ASSERT will catch other cases over time, so I think it is better to just log a different bug and fix that particular issue. I will investigate it today.

@Simon: Looking at the crashlog, I suppose the events are in following order: finished layout, scheduled for compositor update, did some DOM mutation that set needs layout, initial compositor timer fired.

A fix would be to cancel the compositor update timer when a layout is scheduled and reschedule the timer after the layout is done. What do you think?

[Simon Fraser (smfr)]

(In reply to comment #19)
> (In reply to comment #18)
> > Is it possible that this patch caused these flaky assertion failures?
> > 
> > http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r139159%20(5588)/dom/html/level2/html/HTMLDocument08-crash-log.txt
> 
> It's the ASSERT I've added in this patch. That test didn't crash on my machine, so that's why I didn't fix that case too.
> 
> I think that this ASSERT will catch other cases over time, so I think it is better to just log a different bug and fix that particular issue. I will investigate it today.
> 
> @Simon: Looking at the crashlog, I suppose the events are in following order: finished layout, scheduled for compositor update, did some DOM mutation that set needs layout, initial compositor timer fired.
> 
> A fix would be to cancel the compositor update timer when a layout is scheduled and reschedule the timer after the layout is done. What do you think?

Or maybe the compositor update should just bail if layout is dirty (as long as it will happen after the next layout).

[Simon Fraser (smfr)]

This new assertion is covered by bug 106419.