Bug 131280 – Rework CSS calc logic, fixing some reference count mistakes in Length

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Darin Adler]

Rework calculation logic, fixing some reference count mistakes in Length

[Darin Adler]

Created attachment 228701 [details]
Patch

[Darin Adler]

The following commit in Blink drew my attention to the problems in Length:

http://src.chromium.org/viewvc/blink?view=revision&revision=170739

That fixes one particular reference counting problem, but not the other I found.

[Andreas Kling]

Comment on attachment 228701 [details]
Patch

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

r=me

> Source/WebCore/css/CSSCalculationValue.h:94
> +    PassRef<CalculationValue> toCalcValue(const RenderStyle* currentStyle, const RenderStyle* rootStyle, double zoom = 1.0) const;

This name seems a bit evil, since doing someValue->toCalcValue().get().something(); would leak and assert.

> Source/WebCore/platform/Length.h:411
> +// FIXME: Is it really valuable to inline this?

I wonder if the compiler even bothers.

> Source/WebCore/rendering/style/RenderStyle.cpp:1357
>      case Auto:
>          fontWordSpacing = 0;
> -        FALLTHROUGH;
> +        break;

Huh.

[Darin Adler]

Committed r166860: <http://trac.webkit.org/changeset/166860>

[Benjamin Poulain]

It looks like this is causing crashes on the debug bots: http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r166860%20(3814)/results.html

[Brent Fulgham]

This patch introduced some test failures due to the new assertions triggering. Could you please take a look at them?

For example, see the four crash logs in <http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r166860%20(3814)/results.html>.

[Darin Adler]

(In reply to comment #6)
> This patch introduced some test failures due to the new assertions triggering. Could you please take a look at them?
> 
> For example, see the four crash logs in <http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r166860%20(3814)/results.html>.

Please remove the new assertions that are triggering. I have no doubt that they are showing us real pre-existing bugs, but they were not firing on my local machine, or I would have held off on adding them until the problems were fixed.

[Darin Adler]

Darn it, it looks like only one or two of the failures is due to a new assertion. The other failure seems to be due to hash table related assertions caused by the new code and I have no idea what the root cause is.

[Brent Fulgham]

There seems to be some dependency on prior tests. For example, running the test "fast/shapes/shape-outside-floats/shape-outside-boxes-001.html" by itself does not generate the crash due to a missing hash table.

However, if you run the "fast/shapes/shape-outside-floats/shape-outside-animation.html" first, then you will encounter the error.

[Darin Adler]

A fix for the isCalculated() assertion is in bug 131346.

[Darin Adler]

Now, fixes for all the assertions are in the patch attached to bug 131346.

[Alexey Proskuryakov]

One test is still crashing with this assertion, filed bug 131480.

[Brent Fulgham]

Part of the fix for CVE-2014-4410.