Bug 20875 – DRT crashes on animation tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Simon Fraser (smfr)]

DumpRenderTree often crashes on LayoutTests/animations. We tracked this down to a test that ends up calling notifyDone() twice (change-one-anim.html).

[Simon Fraser (smfr)]

Created attachment 23458 [details]
Step 1: rename layoutTestController to gLayoutTestController

[Simon Fraser (smfr)]

Created attachment 23459 [details]
Step 2: bug fix.

The crash was caused by LayoutTestController::notifyDone() getting called after the LayoutTestController had been deleted, so the setting of m_waitToDump was writing to random memory.

I fixed this by having the LayoutTestController keep a pointer to the JSObjectRef that refers to it, and nulling out the privateData when the LayoutTestController is deleted. I also added a finalize function for the JSObjectRef, to clear out the JSObjectRef on the LayoutTestController.

If a test tries to call notifyDone() twice now, then DRT will spew to stdout and stderr.

[Eric Seidel (no email)]

Comment on attachment 23458 [details]
Step 1: rename layoutTestController to gLayoutTestController

I'm OK with this change, but I'd really like to see the style guide updated at the same time if "g" is the new prefix for static variables.

http://webkit.org/coding/coding-style.html

Please update the style guide when landing this change, or make sure we have a style guide for this type of variable.  Right now we have no words on the subject in the guide.

Also, this needs a ChangeLog.

[Eric Seidel (no email)]

Comment on attachment 23459 [details]
Step 2: bug fix.

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.)

[Simon Fraser (smfr)]

(In reply to comment #4)
> (From update of attachment 23459 [details] [edit])
> 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.

Agreed, but neither do we want a "naive" test error to cause random memory
corruption that is very hard to identify.

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

I was following waitUntilDoneWatchdogFired(), which writes to both.

> Why do we need to null-check during finalize?

This was to protect against the case where the JSObjectRef outlived the layoutTestController.

> +static void layoutTestControllerObjectFinalize(JSObjectRef object)
> +{
> +    LayoutTestController* controller =
> reinterpret_cast<LayoutTestController*>(JSObjectGetPrivate(object));
> +    if (controller)
> +        controller->setJSObject(0);
> +}
> 
> That seems wrong.  Cant' it just ASSERT?

Now that ~ LayoutTestController clears the private data of the JSObject, this is the usual code path. Note that runTest() deletes the LayoutTestController before the JSObjectRef is cleaned up (I think).

Another approach would be to make the LayoutTestController RefCounted and have the JSObjectRef hold a ref.

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

This would prevent tests from calling other methods on window.layoutTestController after the notifyDone, but I'm not sure if they need to.

The lifetime/ownership model here is really poorly specified.

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

I will do changelogs; was waiting to see if these patches were the right approach before making them.

[Darin Adler]

(In reply to comment #3)
> I'm OK with this change, but I'd really like to see the style guide updated at
> the same time if "g" is the new prefix for static variables.

Elsewhere, WebKit code uses "s_" for this. I don't have a preference, really.

[Simon Fraser (smfr)]

First patch:
	M	WebKitTools/ChangeLog
	M	WebKitTools/DumpRenderTree/DumpRenderTree.h
	M	WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.cpp
	M	WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp
	M	WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm
	M	WebKitTools/DumpRenderTree/mac/DumpRenderTreeMac.h
	M	WebKitTools/DumpRenderTree/mac/DumpRenderTreeWindow.mm
	M	WebKitTools/DumpRenderTree/mac/EditingDelegate.mm
	M	WebKitTools/DumpRenderTree/mac/FrameLoadDelegate.mm
	M	WebKitTools/DumpRenderTree/mac/ResourceLoadDelegate.mm
	M	WebKitTools/DumpRenderTree/mac/UIDelegate.mm
	M	WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp
	M	WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp
	M	WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.cpp
	M	WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp
	M	WebKitTools/DumpRenderTree/mac/UIDelegate.mm
	M	WebKitTools/DumpRenderTree/mac/DumpRenderTreeWindow.mm
	M	WebKitTools/DumpRenderTree/mac/EditingDelegate.mm
	M	WebKitTools/DumpRenderTree/mac/ResourceLoadDelegate.mm
	M	WebKitTools/DumpRenderTree/mac/FrameLoadDelegate.mm
	M	WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm
	M	WebKitTools/DumpRenderTree/mac/DumpRenderTreeMac.h
	M	WebKitTools/DumpRenderTree/DumpRenderTree.h
	M	WebKitTools/ChangeLog
r36511 = f14a3a8f8d4a1cf3e0c15b1b673c533a91bda700 (trunk)

[Simon Fraser (smfr)]

Second patch:
Committed r36512
	M	WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm
	M	WebKitTools/DumpRenderTree/LayoutTestController.cpp
	M	WebKitTools/DumpRenderTree/LayoutTestController.h
	M	WebKitTools/ChangeLog
r36512 = 5e82be48463378cb1b97abd669c82fa741f53d65 (trunk)

[Simon Fraser (smfr)]

Filed bug 20884 to fix tests now revealed to be behaving badly.

[Simon Fraser (smfr)]

Note that this fix was changed in r36606