Bug 151030 - ASSERTION FAILED: roundedIntPoint(LayoutPoint(rendererMappedResult)) == result in WebCore::RenderGeometryMap::mapToContainer
Summary: ASSERTION FAILED: roundedIntPoint(LayoutPoint(rendererMappedResult)) == resul...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 155580 (view as bug list)
Depends on:
Blocks: 116980
  Show dependency treegraph
 
Reported: 2015-11-09 09:03 PST by Renata Hodovan
Modified: 2020-02-19 22:41 PST (History)
15 users (show)

See Also:


Attachments
Test (274 bytes, text/html)
2015-11-09 09:03 PST, Renata Hodovan
no flags Details
Test case (222 bytes, text/html)
2016-03-18 08:47 PDT, Renata Hodovan
no flags Details
another test case (246 bytes, text/html)
2016-03-25 04:13 PDT, Fujii Hironori
no flags Details
Patch (6.18 KB, patch)
2020-02-18 15:29 PST, Jack
no flags Details | Formatted Diff | Diff
Patch (6.76 KB, patch)
2020-02-18 19:17 PST, Jack
darin: review+
Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2020-02-18 20:59 PST, Jack
no flags Details | Formatted Diff | Diff
Patch (6.73 KB, patch)
2020-02-19 11:57 PST, Jack
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Renata Hodovan 2015-11-09 09:03:59 PST
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
Comment 1 Renata Hodovan 2016-03-18 08:47:18 PDT
Created attachment 274415 [details]
Test case
Comment 2 Said Abou-Hallawa 2016-03-18 11:11:19 PDT
*** Bug 155580 has been marked as a duplicate of this bug. ***
Comment 3 Fujii Hironori 2016-03-21 21:44:55 PDT
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?
Comment 4 Fujii Hironori 2016-03-25 04:13:32 PDT
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.
Comment 5 Brent Fulgham 2016-08-04 17:42:22 PDT
This reproduces in r204037.
Comment 6 Radar WebKit Bug Importer 2016-08-04 17:43:05 PDT
<rdar://problem/27711142>
Comment 7 Radar WebKit Bug Importer 2016-08-04 17:43:30 PDT
<rdar://problem/27711147>
Comment 8 Jack 2020-02-17 17:27:50 PST
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.
Comment 9 Jack 2020-02-17 17:37:17 PST
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;
Comment 10 Jack 2020-02-17 17:52:30 PST
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?
Comment 11 Simon Fraser (smfr) 2020-02-17 18:17:38 PST
(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?
Comment 12 Jack 2020-02-17 19:33:09 PST
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?
Comment 13 Jack 2020-02-18 15:29:26 PST
Created attachment 391107 [details]
Patch
Comment 14 Simon Fraser (smfr) 2020-02-18 15:38:18 PST
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();
Comment 15 Jack 2020-02-18 19:17:08 PST
Created attachment 391128 [details]
Patch
Comment 16 Jack 2020-02-18 19:18:44 PST
Thanks for the great suggestions!
Comment 17 Darin Adler 2020-02-18 19:54:51 PST
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
Comment 18 Darin Adler 2020-02-18 19:56:32 PST
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);
Comment 19 Jack 2020-02-18 20:59:43 PST
Created attachment 391139 [details]
Patch
Comment 20 Jack 2020-02-18 21:10:13 PST
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.
Comment 21 Simon Fraser (smfr) 2020-02-19 11:18:52 PST
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?
Comment 22 Jack 2020-02-19 11:57:51 PST
Created attachment 391185 [details]
Patch
Comment 23 Jack 2020-02-19 12:33:06 PST
Yeah, interestingly -webkit-appearance is needed. I changed it to button so it looks more general. Tried radio but the problem doesn't reproduce.
Comment 24 WebKit Commit Bot 2020-02-19 22:41:25 PST
Comment on attachment 391185 [details]
Patch

Clearing flags on attachment: 391185

Committed r257046: <https://trac.webkit.org/changeset/257046>
Comment 25 WebKit Commit Bot 2020-02-19 22:41:27 PST
All reviewed patches have been landed.  Closing bug.