DumpRenderTree often crashes on LayoutTests/animations. We tracked this down to a test that ends up calling notifyDone() twice (change-one-anim.html).
Created attachment 23458 [details] Step 1: rename layoutTestController to gLayoutTestController
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.
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.
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.)
(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.
(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.
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)
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)
Filed bug 20884 to fix tests now revealed to be behaving badly.
Note that this fix was changed in r36606