[webkit-reviews] review denied: [Bug 22834] Mismatched memory free in the new CSSSelectorList : [Attachment 46376] Patch for comment #23 (retry)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Benbuck Nason <bnason at netflix.com>'s
request for review:
Bug 22834: Mismatched memory free in the new CSSSelectorList
https://bugs.webkit.org/show_bug.cgi?id=22834

Attachment 46376: Patch for comment #23 (retry)
https://bugs.webkit.org/attachment.cgi?id=46376&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
The right fix is to change CSSParser.cpp to call "new CSSSelector" instead of
"fastNew<CSSSelector>()".

> -		   delete m_data.m_tagHistory;
> +		   fastDelete(m_data.m_tagHistory);

This isn't right. When m_hasRareData is true, then the tag history will be
deleted as part of the destruction of OwnPtr. This will call delete, not
fastDelete. So changing the code in ~CSSSelector to call fastDelete will not
fix the mismatch. But since CSSSelector inherits from Noncopyable, there is no
reason to explicitly use fastNew on a CSSSelector -- Noncopyable inherits from
FastAllocBase, and so new should automatically use fastNew. This code change
seems both incorrect and unnecessary.

> -	   delete s;
> +	   fastDelete(s);

Same comment here. Changing these call sites is incompatible with using
OwnPtr<CSSSelector> elsewhere.