[webkit-reviews] review denied: [Bug 15715] Nested XSL stylesheets can produce memory corruption : [Attachment 22944] latest patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jonathan Haas <myrdred at gmail.com>'s
request for review:
Bug 15715: Nested XSL stylesheets can produce memory corruption
https://bugs.webkit.org/show_bug.cgi?id=15715

Attachment 22944: latest patch
https://bugs.webkit.org/attachment.cgi?id=22944&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks OK. I have a few thoughts and questions.

Mark Rowe requested a regression test; I'd like to see the patch include one.

Since parentStyleSheet() is never used outside the XSLStyleSheet class, I don't
think we need to define a function. If later we decide it's needed, that's OK,
but it's slightly nicer to have tighter encapsulation and know no one outside
the class looks at m_parentStyleSheet.

What guarantees that m_parentStyleSheet is not left pointing to a deleted
object?

+    void setParentStyleSheet(XSLStyleSheet* parent) {
+	m_parentStyleSheet = parent; 
+	if (parent)
+	   setOwnerDocument(parent->ownerDocument());
+    }

The opening brace is supposed to go on a separate line. This function is also
getting long enough that it might be better to not put it inline. Our normal
approach is to only use inlines for completely trivial functions unless there's
a specific performance motivation for doing so.

Are there any callers left for XSLStyleSheet::setOwnerDocument()? If not,
perhaps we should remove it.

I'm going to say review- since I'm hoping for a patch with a regression test.
In a pinch I suppose we can land this without one. Let me know if you'd like to
do that -- you should even feel re-mark this patch for review if you think this
should go in as-is even considering my comments.