Bug 97036 – A lot of ERROR messages in FractionalLayoutUnits when opening fast/overflow/overflow-height-float-not-removed-crash3.html regression test

• Status: NEW
• Component: Layout and Rendering

[Alexey Proskuryakov]

18.09.12 12:47:02,850 WebProcess[15556]: ERROR: !(m_value >= 0)
18.09.12 12:47:02,850 WebProcess[15556]: /Users/ap/Safari/OpenSource/Source/WebCore/platform/FractionalLayoutUnit.h(166) : unsigned int WebCore::FractionalLayoutUnit::toUnsigned() const
18.09.12 12:47:02,850 WebProcess[15556]: ERROR: !(isInBounds(value))
18.09.12 12:47:02,850 WebProcess[15556]: /Users/ap/Safari/OpenSource/Source/WebCore/platform/FractionalLayoutUnit.h(101) : WebCore::FractionalLayoutUnit::FractionalLayoutUnit(unsigned int)
18.09.12 12:47:02,850 WebProcess[15556]: ERROR: !(m_value >= 0)
18.09.12 12:47:02,851 WebProcess[15556]: /Users/ap/Safari/OpenSource/Source/WebCore/platform/FractionalLayoutUnit.h(166) : unsigned int WebCore::FractionalLayoutUnit::toUnsigned() const
18.09.12 12:47:02,851 WebProcess[15556]: ERROR: !(isInBounds(value))
18.09.12 12:47:02,851 WebProcess[15556]: /Users/ap/Safari/OpenSource/Source/WebCore/platform/FractionalLayoutUnit.h(101) : WebCore::FractionalLayoutUnit::FractionalLayoutUnit(unsigned int)
18.09.12 12:47:02,851 WebProcess[15556]: ERROR: !(m_value >= 0)
18.09.12 12:47:02,851 WebProcess[15556]: /Users/ap/Safari/OpenSource/Source/WebCore/platform/FractionalLayoutUnit.h(166) : unsigned int WebCore::FractionalLayoutUnit::toUnsigned() const
18.09.12 12:47:02,851 WebProcess[15556]: ERROR: !(isInBounds(value))
18.09.12 12:47:02,851 WebProcess[15556]: /Users/ap/Safari/OpenSource/Source/WebCore/platform/FractionalLayoutUnit.h(101) : WebCore::FractionalLayoutUnit::FractionalLayoutUnit(unsigned int)
18.09.12 12:47:02,852 WebProcess[15556]: ERROR: !(m_value >= 0)
18.09.12 12:47:02,852 WebProcess[15556]: /Users/ap/Safari/OpenSource/Source/WebCore/platform/FractionalLayoutUnit.h(166) : unsigned int WebCore::FractionalLayoutUnit::toUnsigned() const
18.09.12 12:47:02,852 WebProcess[15556]: ERROR: !(isInBounds(value))
18.09.12 12:47:02,852 WebProcess[15556]: /Users/ap/Safari/OpenSource/Source/WebCore/platform/FractionalLayoutUnit.h(101) : WebCore::FractionalLayoutUnit::FractionalLayoutUnit(unsigned int)

[Levi Weintraub]

We display this information on Debug builds as a tool to notify developers that we're overflowing a LayoutUnit, as this is often a source of bugs and improperly handled conditions. This particular warning shows that a negative number is being cast into an unsigned, which is rarely what one wants.

Are you suggesting we remove these warnings (/suppress them by default) or do you have a suggestion of a way to present this data in a more helpful, less distracting way?

[Alexey Proskuryakov]

I think that this bug needs to be fixed by whoever uses LayoutUnits incorrectly.

Not sure if there is complete consensus on what LOG_ERROR should be used for. I think that it's really usually about things largely outside WebKit control, like corrupt sqlite databases. Broken HTML input should never cause any sort of console logging.

Using LOG_ERROR as a temporary solution before turning it into an ASSERT seems very reasonable to me, as long as we agree that making it an assertion is the goal.

[Levi Weintraub]

(In reply to comment #2)
> I think that this bug needs to be fixed by whoever uses LayoutUnits incorrectly.

Here's a possible culprit: https://bugs.webkit.org/show_bug.cgi?id=68744

I'll have to dig in when I have some time to positively identify this particular issue.

> Not sure if there is complete consensus on what LOG_ERROR should be used for. I think that it's really usually about things largely outside WebKit control, like corrupt sqlite databases. Broken HTML input should never cause any sort of console logging.
> 
> Using LOG_ERROR as a temporary solution before turning it into an ASSERT seems very reasonable to me, as long as we agree that making it an assertion is the goal.

To be clear, the logging only happens on Debug builds.

I agree in theory with what you're saying. It's currently possible to overflow rendering code any number of ways, and usually we handle it in some sort of graceful way, such as maxing with zero when we expect positive numbers. That and outstanding issues such as the one I mentioned prevented us from going with ASSERTS from the beginning (it was our initial implementation). Casting negative numbers to unsigned is almost never the right thing to do, so this particular check would likely be a great candidate for an ASSERT.

Longterm, Emil is experimenting with saturated math that would enable us to ASSERT for any actual overflows.

[Ahmad Saleem]

FractionalLayoutUnit.h got changed into 'LayoutUnit.h': https://github.com/WebKit/WebKit/commit/6acc3f9e4fbe34c3519d021ee6836f0dbabffc36

The following test is skipped on Mac [Debug]:

https://searchfox.org/wubkat/rev/da53e5336a9c4f2e056e219a184c4f908cda7f02/LayoutTests/platform/mac/TestExpectations#530

> # Overflowing LayoutUnits cause RenderGeometryMap assertions

With: webkit.org/b/67434

I think this is duplicate of bug 67434 or to fix in it or update the bug URL in Test Expectation.