[webkit-reviews] review granted: [Bug 172086] Crash in libxml2.2.dylib: xmlDictReference : [Attachment 310062] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 172086: Crash in libxml2.2.dylib: xmlDictReference
https://bugs.webkit.org/show_bug.cgi?id=172086

Attachment 310062: Patch

https://bugs.webkit.org/attachment.cgi?id=310062&action=review




--- Comment #4 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 310062
  --> https://bugs.webkit.org/attachment.cgi?id=310062
Patch

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

> Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:125
> +	   if (m_stylesheetDoc)

I take it that it is not safe to call xmlFreeDoc() with a nullptr.

>>> Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:129
>>> +	 m_stylesheetDoc = nullptr;
>> 
>> This function is an improvement over the current code. Could it simplify
this function as well as the rest of the code if we made m_stylesheetDoc a
std::unique_ptr<> with a custom deleter that calls xmlFreeDoc()? If we made
this change then would we be able to remove m_stylesheetDocTaken? One example
of a std::unique_ptr<> with a custom deleter is OpenTypeMathData::m_mathFont
whose custom deleter is a function object called HbFontDeleter,
<http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/opentype
/OpenTypeMathData.h?rev=216800#L136>.
> 
> So here's the challenge with m_stylesheetDoc:  It exists in three different
states in this class.  Those states are:
> 
> 1. nullptr
> 2. Allocated and owned by XSLStyleSheet class.
> 3. Allocated but owned by libxslt data structure, so just a raw pointer in
XSLStyleSheet class.
> 
> The m_stylesheetDocTaken variable differentiates between states #2 and #3. 
When m_stylesheetDocTaken is false, we're in state #2.	When
m_stylesheetDocTaken is true, we're in state #3.  (Yes, m_stylesheetDocTaken
could probably be changed to something like m_stylesheetDocOwned and the
true/false values reversed, but I wanted to keep things "simple" for this
patch.)
> 
> Now if I could encode the m_stylesheetDocTaken state inside the
std::unique_ptr<>, that would be great!  I need to do more research to know,
though.  (Seems like that would violate the main goal of std::unique_ptr<>, in
which case I'd have to have two variables:  m_stylesheetDoc as
std::unique_ptr<>, and m_stylesheetDocRaw as a raw pointer, which logic to
return the correct one based on how they're set.  That doesn't seem much better
than m_stylesheetDoc and m_stylesheetDocTaken, though.)

I see.

> Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:186
> +    return !!m_stylesheetDoc;

I take it you feel this reads better.