[webkit-reviews] review granted: [Bug 20875] DRT crashes on animation tests : [Attachment 23459] Step 2: bug fix.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Simon Fraser
<simon.fraser at apple.com>'s request for review:
Bug 20875: DRT crashes on animation tests
https://bugs.webkit.org/show_bug.cgi?id=20875

Attachment 23459: Step 2: bug fix.
https://bugs.webkit.org/attachment.cgi?id=23459&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
It would be relatively easy to make this crash by moving the notifyDone object
onto something that wasn't the LayoutTestController and calling it.  But we
don't have to make DRT robust against "bad" tests.

Why would you print the notifyDone() message to both STDOUT and STDERR? 
Generally we just use STDERR For that.

Why do we need to null-check during finalize?

+static void layoutTestControllerObjectFinalize(JSObjectRef object)
+{
+    LayoutTestController* controller =
reinterpret_cast<LayoutTestController*>(JSObjectGetPrivate(object));
+    if (controller)
+	 controller->setJSObject(0);
+}

That seems wrong.  Cant' it just ASSERT?

Equally simple would be to just have notifyDone() null out the private variable
on the JSObject... since maybe notifyDone() is already destroying the
LayoutTestController?

Please add a ChangeLog which explains why this change makes sense. :)  (I feel
your pain re: git and ChangeLogs.)