Bug 151030 – ASSERTION FAILED: roundedIntPoint(LayoutPoint(rendererMappedResult)) == result in WebCore::RenderGeometryMap::mapToContainer

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

[Renata Hodovan]

Created attachment 265056 [details]
Test

Load the attached test with debug MiniBrowser:

parent.appendChild(child)} catch (err) {}parent = document.getElementById('id_0')parent.removeChild(parent.childNodes[
<style>
* {
    overflow-y: auto;
    -webkit-margin-after : -4425511469ex;
}
</style>
<hgroup></hgroup>
<h1>a
    <map>
        <h3></h3>
    </map>
</h1>


OS: Ubuntu 15.04 x86_64
Checked build: debug EFL
Checked version: 009fb33


Backtrace:

ASSERTION FAILED: roundedIntPoint(rendererMappedResult) == roundedIntPoint(result)
../../Source/WebCore/rendering/RenderGeometryMap.cpp(118) : WebCore::FloatPoint WebCore::RenderGeometryMap::mapToContainer(const WebCore::FloatPoint&, const WebCore::RenderLayerModelObject*) const
1   0x7f98eb09df97 WTFCrash
2   0x7f98f1a72b1f WebCore::RenderGeometryMap::mapToContainer(WebCore::FloatPoint const&, WebCore::RenderLayerModelObject const*) const
3   0x7f98f1ac59b6 WebCore::RenderGeometryMap::absolutePoint(WebCore::FloatPoint const&) const
4   0x7f98f1aa4f52 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, unsigned int)
5   0x7f98f1aa5431 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, unsigned int)
6   0x7f98f1aa5431 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, unsigned int)
7   0x7f98f1aa5431 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, unsigned int)
8   0x7f98f1aa4e2b WebCore::RenderLayer::updateLayerPositionsAfterLayout(WebCore::RenderLayer const*, unsigned int)
9   0x7f98f177336c WebCore::FrameView::layout(bool)
10  0x7f98f11d35b8 WebCore::Document::implicitClose()
11  0x7f98f1636b1b WebCore::FrameLoader::checkCallImplicitClose()
12  0x7f98f1636852 WebCore::FrameLoader::checkCompleted()
13  0x7f98f16365c2 WebCore::FrameLoader::finishedParsing()
14  0x7f98f11dd5e8 WebCore::Document::finishedParsing()
15  0x7f98f2544799 WebCore::HTMLConstructionSite::finishedParsing()
16  0x7f98f1530210 WebCore::HTMLTreeBuilder::finished()
17  0x7f98f15008b4 WebCore::HTMLDocumentParser::end()
18  0x7f98f150098d WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
19  0x7f98f14ff66f WebCore::HTMLDocumentParser::prepareToStopParsing()
20  0x7f98f15009d0 WebCore::HTMLDocumentParser::attemptToEnd()
21  0x7f98f1500a87 WebCore::HTMLDocumentParser::finish()
22  0x7f98f16219f6 WebCore::DocumentWriter::end()
23  0x7f98f160b2cc WebCore::DocumentLoader::finishedLoading(double)
24  0x7f98f160b02a WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource*)
25  0x7f98f16b6437 WebCore::CachedResource::checkNotify()
26  0x7f98f16b6546 WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*)
27  0x7f98f16b273a WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*)
28  0x7f98f167b168 WebCore::SubresourceLoader::didFinishLoading(double)
29  0x7f98f1675c47 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double)
30  0x7f98f1e67560
31  0x7f98e768d5b6
Aborted (core dumped)

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007f98eb09df9c in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321
321     *(int *)(uintptr_t)0xbbadbeef = 0;
#0  0x00007f98eb09df9c in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321
#1  0x00007f98f1a72b1f in WebCore::RenderGeometryMap::mapToContainer (this=0x7fff98ccb400, p=..., container=0x0) at ../../Source/WebCore/rendering/RenderGeometryMap.cpp:118
#2  0x00007f98f1ac59b6 in WebCore::RenderGeometryMap::absolutePoint (this=0x7fff98ccb400, p=...) at ../../Source/WebCore/rendering/RenderGeometryMap.h:85
#3  0x00007f98f1aa4f52 in WebCore::RenderLayer::updateLayerPositions (this=0x7f98d26a45a0, geometryMap=0x7fff98ccb400, flags=10) at ../../Source/WebCore/rendering/RenderLayer.cpp:477
#4  0x00007f98f1aa5431 in WebCore::RenderLayer::updateLayerPositions (this=0x7f98d26feb40, geometryMap=0x7fff98ccb400, flags=10) at ../../Source/WebCore/rendering/RenderLayer.cpp:557
#5  0x00007f98f1aa5431 in WebCore::RenderLayer::updateLayerPositions (this=0x7f98d26fea20, geometryMap=0x7fff98ccb400, flags=10) at ../../Source/WebCore/rendering/RenderLayer.cpp:557
#6  0x00007f98f1aa5431 in WebCore::RenderLayer::updateLayerPositions (this=0x7f98d26fe5a0, geometryMap=0x7fff98ccb400, flags=10) at ../../Source/WebCore/rendering/RenderLayer.cpp:557
#7  0x00007f98f1aa4e2b in WebCore::RenderLayer::updateLayerPositionsAfterLayout (this=0x7f98d26fe5a0, rootLayer=0x7f98d26fe5a0, flags=14) at ../../Source/WebCore/rendering/RenderLayer.cpp:460
#8  0x00007f98f177336c in WebCore::FrameView::layout (this=0x7f98d2425540, allowSubtree=true) at ../../Source/WebCore/page/FrameView.cpp:1455
#9  0x00007f98f11d35b8 in WebCore::Document::implicitClose (this=0x7f98d2426a40) at ../../Source/WebCore/dom/Document.cpp:2704
#10 0x00007f98f1636b1b in WebCore::FrameLoader::checkCallImplicitClose (this=0x7f98d26e4098) at ../../Source/WebCore/loader/FrameLoader.cpp:889
#11 0x00007f98f1636852 in WebCore::FrameLoader::checkCompleted (this=0x7f98d26e4098) at ../../Source/WebCore/loader/FrameLoader.cpp:835
#12 0x00007f98f16365c2 in WebCore::FrameLoader::finishedParsing (this=0x7f98d26e4098) at ../../Source/WebCore/loader/FrameLoader.cpp:756
#13 0x00007f98f11dd5e8 in WebCore::Document::finishedParsing (this=0x7f98d2426a40) at ../../Source/WebCore/dom/Document.cpp:4897
#14 0x00007f98f2544799 in WebCore::HTMLConstructionSite::finishedParsing (this=0x7f98d26fe6e0) at ../../Source/WebCore/html/parser/HTMLConstructionSite.cpp:403
#15 0x00007f98f1530210 in WebCore::HTMLTreeBuilder::finished (this=0x7f98d26fe6c0) at ../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2937
#16 0x00007f98f15008b4 in WebCore::HTMLDocumentParser::end (this=0x7f98d242e840) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:393
#17 0x00007f98f150098d in WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd (this=0x7f98d242e840) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:402
#18 0x00007f98f14ff66f in WebCore::HTMLDocumentParser::prepareToStopParsing (this=0x7f98d242e840) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:132
#19 0x00007f98f15009d0 in WebCore::HTMLDocumentParser::attemptToEnd (this=0x7f98d242e840) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:414
#20 0x00007f98f1500a87 in WebCore::HTMLDocumentParser::finish (this=0x7f98d242e840) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:442
#21 0x00007f98f16219f6 in WebCore::DocumentWriter::end (this=0x7f98d24249e0) at ../../Source/WebCore/loader/DocumentWriter.cpp:247
#22 0x00007f98f160b2cc in WebCore::DocumentLoader::finishedLoading (this=0x7f98d2424940, finishTime=0) at ../../Source/WebCore/loader/DocumentLoader.cpp:437
#23 0x00007f98f160b02a in WebCore::DocumentLoader::notifyFinished (this=0x7f98d2424940, resource=0x7f98d2436000) at ../../Source/WebCore/loader/DocumentLoader.cpp:384
#24 0x00007f98f16b6437 in WebCore::CachedResource::checkNotify (this=0x7f98d2436000) at ../../Source/WebCore/loader/cache/CachedResource.cpp:297
#25 0x00007f98f16b6546 in WebCore::CachedResource::finishLoading (this=0x7f98d2436000) at ../../Source/WebCore/loader/cache/CachedResource.cpp:313
#26 0x00007f98f16b273a in WebCore::CachedRawResource::finishLoading (this=0x7f98d2436000, data=0x7f98d27bb680) at ../../Source/WebCore/loader/cache/CachedRawResource.cpp:103
#27 0x00007f98f167b168 in WebCore::SubresourceLoader::didFinishLoading (this=0x7f98d242fa80, finishTime=0) at ../../Source/WebCore/loader/SubresourceLoader.cpp:372
#28 0x00007f98f1675c47 in WebCore::ResourceLoader::didFinishLoading (this=0x7f98d242fa80, finishTime=0) at ../../Source/WebCore/loader/ResourceLoader.cpp:638
#29 0x00007f98f1e67560 in WebCore::readCallback (asyncResult=0x19021a0, data=0x7f98d27bd660) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1341
#30 0x00007f98e768d5b6 in async_ready_callback_wrapper (source_object=0x180d5b0, res=0x19021a0, user_data=0x7f98d27bd660) at ginputstream.c:523
#31 0x00007f98e76b3b84 in g_task_return_now (task=0x19021a0) at gtask.c:1077
#32 0x00007f98e76b3ba9 in complete_in_idle_cb (task=0x19021a0) at gtask.c:1086
#33 0x00007f98e70ebadd in g_main_dispatch (context=0x18078d0) at gmain.c:3064
#34 g_main_context_dispatch (context=context@entry=0x18078d0) at gmain.c:3663
#35 0x00007f98e8a57e58 in _ecore_glib_select__locked (ecore_timeout=<optimized out>, efds=0x7fff98ccc250, wfds=0x7fff98ccc1d0, rfds=0x7fff98ccc150, ecore_fds=<optimized out>, ctx=<optimized out>) at lib/ecore/ecore_glib.c:172
#36 _ecore_glib_select (ecore_fds=<optimized out>, rfds=0x7fff98ccc150, wfds=0x7fff98ccc1d0, efds=0x7fff98ccc250, ecore_timeout=<optimized out>) at lib/ecore/ecore_glib.c:204
#37 0x00007f98e8a5b4a4 in _ecore_main_select (timeout=9.532824124368238e-130) at lib/ecore/ecore_main.c:1459
#38 0x00007f98e8a5bed4 in _ecore_main_loop_iterate_internal (once_only=once_only@entry=0) at lib/ecore/ecore_main.c:1893
#39 0x00007f98e8a5bfc7 in ecore_main_loop_begin () at lib/ecore/ecore_main.c:983
#40 0x00007f98eb0f8e8d in WTF::RunLoop::run () at ../../Source/WTF/wtf/efl/RunLoopEfl.cpp:49
#41 0x00007f98f0f58e1b in WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> (argc=2, argv=0x7fff98ccc688) at ../../Source/WebKit2/Shared/unix/ChildProcessMain.h:61
#42 0x00007f98f0f58a29 in WebKit::WebProcessMainUnix (argc=2, argv=0x7fff98ccc688) at ../../Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:161
#43 0x00000000004008ea in main (argc=2, argv=0x7fff98ccc688) at ../../Source/WebKit2/WebProcess/EntryPoint/unix/WebProcessMain.cpp:44

[Renata Hodovan]

Created attachment 274415 [details]
Test case

[Said Abou-Hallawa]

*** Bug 155580 has been marked as a duplicate of this bug. ***

[Fujii Hironori]

The function saturatedAddition (Source/WTF/wtf/SaturatedArithmetic.h) overflows when opening this test case (attachment 265056 [details]).

Is there a good way to assert something in case of overflow?

[Fujii Hironori]

Created attachment 274898 [details]
another test case

I create a new test case by slightly changing the original test case.
In this test, two values are far different:

> +		rendererMappedResult	{m_x=164.000000 m_y=9.00000000 }	WebCore::FloatPoint
> +		result	{m_x=164.000000 m_y=-2.00000000 }	WebCore::FloatPoint

Tested with trunk@198375 AppleWin port, Debug build.

[Brent Fulgham]

This reproduces in r204037.

[Radar WebKit Bug Importer]

<rdar://problem/27711142>

[Radar WebKit Bug Importer]

<rdar://problem/27711147>

[Jack]

Root cause of descrepency when m_accumulatedOffset is saturated:
In pushMappingsToAncestor, the offset of each layer is accumulated and stored in m_accumulatedOffset, and clamped by saturated value: 0x7FFFFFFF. On the other hand, in popMappingsToAncestor, offset of each layer is substracted from m_accumulatedOffset. However, when m_accumulatedOffset is saturated the substraction does not set m_accumulatedOffset back to the original offset of each layer, resulting in the assertion.

Verified root cause by workaround: Keep a snapshot of m_accumulatedOffset at each call to pushMappingsToAncestor, and restore the offset from the snapshot when popMappingsToAncestor is called.

[Jack]

workaround:

-void RenderGeometryMap::stepInserted(const RenderGeometryMapStep& step)
+void RenderGeometryMap::stepInserted(RenderGeometryMapStep& step)
 {
     // RenderView's offset, is only applied when we have fixed-positions.
-    if (!step.m_renderer->isRenderView())
+    if (!step.m_renderer->isRenderView()) {
+        step.m_accumulatedOffsetCache = m_accumulatedOffset;
         m_accumulatedOffset += step.m_offset;
+    }

 void RenderGeometryMap::stepRemoved(const RenderGeometryMapStep& step)
 {
     // RenderView's offset, is only applied when we have fixed-positions.
     if (!step.m_renderer->isRenderView())
-        m_accumulatedOffset -= step.m_offset;
+        m_accumulatedOffset = step.m_accumulatedOffsetCache;

[Jack]

However, this work around adds LayoutSize for each RenderGeometryMapStep. Another approach can be saving the reference of RenderGeometryMapStep that overflows m_accumulatedOffset, and later only subtract offset starting at the overflowing RenderGeometryMapStep. Or, if it rarely happens in real world, we can remove the assertion, so it does not block other issues?

[Simon Fraser (smfr)]

(In reply to Jack from comment #10)
> However, this work around adds LayoutSize for each RenderGeometryMapStep.
> Another approach can be saving the reference of RenderGeometryMapStep that
> overflows m_accumulatedOffset, and later only subtract offset starting at
> the overflowing RenderGeometryMapStep. Or, if it rarely happens in real
> world, we can remove the assertion, so it does not block other issues?

Can we change the assertion to detect the numeric overflow cases?

[Jack]

In some test cases, m_accumulatedOffset is used after subtracting step offset. And if the subtraction happens when it is saturated, the result is a random value (not max int32). Therefore, asserting overflow m_accumulatedOffset would still fail in those cases.

(In reply to Simon Fraser (smfr) from comment #11)
> Can we change the assertion to detect the numeric overflow cases?

[Jack]

Created attachment 391107 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 391107 [details]
Patch

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

> Source/WebCore/rendering/RenderGeometryMap.cpp:112
> +        ASSERT(roundedIntPoint(LayoutPoint(rendererMappedResult)) == result || m_accumulatedOffsetSaturated);

I think this is a good place for ASSERT_IMPLIES:
ASSERT_IMPLIES(!m_accumulatedOffsetSaturated, LayoutPoint(rendererMappedResult)) == result)

> Source/WebCore/rendering/RenderGeometryMap.cpp:272
> +        if (m_accumulatedOffset.width().mightBeSaturated() || m_accumulatedOffset.height().mightBeSaturated())
> +            m_accumulatedOffsetSaturated = true;

Why don't we add:
LayoutSize::mightBeSaturated() which returns true if width or height are saturated, and change this to:
m_accumulatedOffsetSaturated |= m_accumulatedOffset.mightBeSaturated();

[Jack]

Created attachment 391128 [details]
Patch

[Jack]

Thanks for the great suggestions!

[Darin Adler]

Comment on attachment 391128 [details]
Patch

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

Looks OK. I’m frustrated that we need the data member and can’t just check m_accumulatedOffset.mightBeSaturated(), but I assume we do need it.

> Source/WebCore/rendering/RenderGeometryMap.cpp:112
> +        ASSERT_IMPLIES(!m_accumulatedOffsetSaturated, roundedIntPoint(LayoutPoint(rendererMappedResult)) == result);

I think just adding the expression "m_accumulatedOffsetSaturated &&" to the assertion would be clearer than this use of ASSERT_IMPLIES. Maybe I’m biased, because I never heard of ASSERT_IMPLIES before today.

> Source/WebCore/rendering/RenderGeometryMap.h:134
> +    bool m_accumulatedOffsetSaturated { false };

m_accumulatedOffsetMightBeSaturated

[Darin Adler]

Comment on attachment 391128 [details]
Patch

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

>> Source/WebCore/rendering/RenderGeometryMap.cpp:112
>> +        ASSERT_IMPLIES(!m_accumulatedOffsetSaturated, roundedIntPoint(LayoutPoint(rendererMappedResult)) == result);
> 
> I think just adding the expression "m_accumulatedOffsetSaturated &&" to the assertion would be clearer than this use of ASSERT_IMPLIES. Maybe I’m biased, because I never heard of ASSERT_IMPLIES before today.

I guess Simon suggested using ASSERT_IMPLIES. But I still think this would be easier to read for me:

     ASSERT(m_accumulatedOffsetMightBeSaturated || roundedIntPoint(LayoutPoint(rendererMappedResult)) == result);

[Jack]

Created attachment 391139 [details]
Patch

[Jack]

Yeah, m_accumulatedOffset does not stay saturated. It's doable if we make the subtraction aware of saturated condition. However, that will change the behavior.

(In reply to Darin Adler from comment #17)
> Looks OK. I’m frustrated that we need the data member and can’t just check
> m_accumulatedOffset.mightBeSaturated(), but I assume we do need it.

[Simon Fraser (smfr)]

Comment on attachment 391139 [details]
Patch

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

> Source/WebCore/platform/graphics/LayoutSize.h:139
> +#if ASSERT_ENABLED

No need to wrap this in #if ASSERT_ENABLED.  LayoutUnit::mightBeSaturated() isn't.

> LayoutTests/fast/layers/geometry-map-saturated-offset-assert.html:10
> +        -webkit-appearance:media-volume-slider-mute-button;
> +        vertical-align:-53772756.9in;
> +        overflow-y:hidden;
> +        -webkit-padding-before:+.8mm;

Is this really the minimal test case? Can we do without  media-volume-slider-mute-button and the -webkit prefixes?

[Jack]

Created attachment 391185 [details]
Patch

[Jack]

Yeah, interestingly -webkit-appearance is needed. I changed it to button so it looks more general. Tried radio but the problem doesn't reproduce.

[WebKit Commit Bot]

Comment on attachment 391185 [details]
Patch

Clearing flags on attachment: 391185

Committed r257046: <https://trac.webkit.org/changeset/257046>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.