[webkit-reviews] review granted: [Bug 9787] fast/frames tests failing (bad pointer to owner element) under MallocScribble : [Attachment 9277] Patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 9787: fast/frames tests failing (bad pointer to owner element) under
MallocScribble
http://bugzilla.opendarwin.org/show_bug.cgi?id=9787

Attachment 9277: Patch
http://bugzilla.opendarwin.org/attachment.cgi?id=9277&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The line of code in Frame::~Frame that says d->m_ownerElement = 0 doesn't seem
necessary. We're destroying the object and we don't need to nil out fields in
it.

The code should not use renderer->element() -- it should use the more efficient
and more sensibly named node() function.

Are you certain there's no code path where the frame element gets destroyed
without a close call? For example, what about when a whole page and frame tree
goes away at once. Same question about the other elements.

It seems a little dangerous to me that the frame has a pointer to the owner
element, and it's not ref'd. And in the other direction, the owner element has
only an indirect memory of the frame -- a frame name. Can these two ever get
out of sync? Because if they do, then the Frame has a bad pointer in it.

The patch looks fine to me, but I'm still concerned that the object ownership
relationship is too fragile.

I'm going to say review+ because this is definitely an improvement.