[webkit-reviews] review granted: [Bug 98158] REGRESSION(r130160): It made 3 tests crash : [Attachment 166883] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Stephen Chenney
<schenney at chromium.org>'s request for review:
Bug 98158: REGRESSION(r130160): It made 3 tests crash
https://bugs.webkit.org/show_bug.cgi?id=98158

Attachment 166883: Patch
https://bugs.webkit.org/attachment.cgi?id=166883&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166883&action=review


I like the idea.  It hink there are some nits you might chose to fix yet.

> Source/WebCore/css/CSSSegmentedFontFace.cpp:111
> +    if (m_fontDataTable.contains(hashKey))
> +	   return m_fontDataTable.get(hashKey);

OK.  So seems we could at least do one lookup instead of two. 
RefPtr<SegmentedFontData> fontData = m_fontDataTable.get(); if (fontData)
return fontData;

>>> Source/WebCore/css/CSSSegmentedFontFace.cpp:113
>>> +	 RefPtr<SegmentedFontData> fontData = SegmentedFontData::create();
>> 
>> I'm confused.  This looks identical to the code you're removing, just
slower. :)
> 
> Previous code always added the key to the cache. If the key was already in
the map, the add would return the FontData and this method would return it.
When the key was absent, we would go ahead and create the FontData, which at
the time of creation has empty m_ranges. Because we're holding a reference to
the RefPtr value from the hash map, that also puts the created FontData in the
map.
> 
> If the attempt to populate the range data failed, we return 0 from this
method, but that leaves the newly created FontData in the map, with empty
m_ranges. Later, when another caller asks for the FontData, it's there in the
map and FontData with empty m_ranges is returned, which violates an assumption
of SegmentedFontData.
> 
> There are at least two other potential fixes that leave the "add" in place.
We can check for null ranges before returning the cached result and, if empty,
try again to create them. That avoids a tiny bit of ref pointer thrashing. In
hindsight this is probably a better solution.
> 
> Or, when we fail to create range data we can remove the FontData from the
cache before returning 0.

I see.	That makes sense, thank you.

> Source/WebCore/css/CSSSegmentedFontFace.cpp:130
> +	       m_fontDataTable.add(hashKey, fontData);

I might add a comment here that said something like:
// Onyl add our font to the table if we succeeded in creating ranges for it.
(Or something nicer.)