Rework calculation logic, fixing some reference count mistakes in Length
Created attachment 228701 [details] Patch
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.
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.
Committed r166860: <http://trac.webkit.org/changeset/166860>
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
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>.
(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.
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.
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.
A fix for the isCalculated() assertion is in bug 131346.
Now, fixes for all the assertions are in the patch attached to bug 131346.
One test is still crashing with this assertion, filed bug 131480.
Part of the fix for CVE-2014-4410.