Summary: | JSC::Bindings m_rootObject->isValid() assert fails when running layout tests | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihai Parparita <mihaip> | ||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, ap, aroben, darin, eric, ggaren, mario, mrobinson, msaboff, ossy, sam, webkit.review.bot | ||||
Priority: | P2 | Keywords: | InRadar, LayoutTestFailure | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Mihai Parparita
2011-02-03 14:09:33 PST
This is happening on Apple's Windows port, too: http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r77529%20(24884)/CrashLog_0d74_2011-02-03_13-15-39-812.txt Here's another stack trace from the 64-bit GTK+ debug bot: warning: Can't read pathname for load map: Input/output error. Core was generated by `/home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/Programs/DumpR'. Program terminated with signal 11, Segmentation fault. #0 0x00007f8a22fff8e0 in JSC::Bindings::Instance::willDestroyRuntimeObject (this=0x78513c0, object=0x7f8a146f5e40) at ../../Source/WebCore/bridge/jsc/BridgeJSC.cpp:109 109 ASSERT(m_rootObject->isValid()); #0 0x00007f8a22fff8e0 in JSC::Bindings::Instance::willDestroyRuntimeObject (this=0x78513c0, object=0x7f8a146f5e40) at ../../Source/WebCore/bridge/jsc/BridgeJSC.cpp:109 #1 0x00007f8a2300a014 in JSC::Bindings::RuntimeObject::~RuntimeObject (this=0x7f8a146f5e40, __in_chrg=<value optimized out>) at ../../Source/WebCore/bridge/runtime_object.cpp:58 #2 0x00007f8a22ff93c4 in JSC::Bindings::CRuntimeObject::~CRuntimeObject (this=0x7f8a146f5e40, __in_chrg=<value optimized out>) at ../../Source/WebCore/bridge/c/CRuntimeObject.cpp:45 #3 0x00007f8a23d263df in JSC::MarkedSpace::sweep (this=0x28df4e8) at ../../Source/JavaScriptCore/runtime/MarkedSpace.cpp:272 #4 0x00007f8a23d2821f in JSC::Heap::reset (this=0x28df4e0, sweepToggle=JSC::Heap::DoSweep) at ../../Source/JavaScriptCore/runtime/Heap.cpp:387 #5 0x00007f8a23d2816b in JSC::Heap::collectAllGarbage (this=0x28df4e0) at ../../Source/JavaScriptCore/runtime/Heap.cpp:371 #6 0x00007f8a22f4f4a1 in WebCore::collect () at ../../Source/WebCore/bindings/js/GCController.cpp:46 #7 0x00007f8a22f4f5f4 in WebCore::GCController::gcTimerFired (this=0x2909940) at ../../Source/WebCore/bindings/js/GCController.cpp:69 #8 0x00007f8a22f4f80e in WebCore::Timer<WebCore::GCController>::fired (this=0x2909940) at ../../Source/WebCore/platform/Timer.h:99 #9 0x00007f8a235f3e38 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x22fabb0) at ../../Source/WebCore/platform/ThreadTimers.cpp:112 #10 0x00007f8a235f3d6f in WebCore::ThreadTimers::sharedTimerFired () at ../../Source/WebCore/platform/ThreadTimers.cpp:90 #11 0x00007f8a22df83e6 in WebCore::timeout_cb () at ../../Source/WebCore/platform/gtk/SharedTimerGtk.cpp:49 #12 0x00007f8a20023dbb in g_timeout_dispatch (source=0x8cc0080, callback=0, user_data=0x7f8a1ee99e00) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3877 #13 0x00007f8a20023362 in g_main_dispatch (context=0x224d1e0) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:2440 #14 g_main_context_dispatch (context=0x224d1e0) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3013 #15 0x00007f8a20027a28 in g_main_context_iterate (context=0x224d1e0, block=<value optimized out>, dispatch=<value optimized out>, self=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3091 #16 0x00007f8a20027f35 in g_main_loop_run (loop=0x7f8a10377880) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3299 #17 0x00007f8a21f5c657 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0 #18 0x000000000041cd22 in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:654 #19 0x000000000041c3f2 in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:468 #20 0x000000000041e325 in main (argc=2, argv=0x7fff141a8c38) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1095 We should skip these tests for now. Committed r77747: <http://trac.webkit.org/changeset/77747> *** Bug 53991 has been marked as a duplicate of this bug. *** Created attachment 81683 [details]
Patch to change failing ASSERT to conditionally remove RuntimeObject
There is a race where a RuntimeObject has been marked for garbage collection but not reclaimed. When RootObject::invalidate() is called for a RootObject containing a GC'ed RuntimeObject, the invalidate call for the RuntimeObject will not be called (due to the m_runtimeObject.isValid(it) check). The m_runtimeObjects WeakGCMap will be cleared and the m_valid flag will be cleared. Later, when the RuntimeObject's dtor is called, it will clean up the related Instance object which was leading to the ASSERTion failure.
The code has been changed to make the removal of the runtime object conditional on m_rootObject being valid. If it isn't valid, the RuntimeObject has already been removed.
Comment on attachment 81683 [details] Patch to change failing ASSERT to conditionally remove RuntimeObject View in context: https://bugs.webkit.org/attachment.cgi?id=81683&action=review review+ but I would like the patch better if it didn’t do the same thing twice. > Source/WebCore/bridge/jsc/BridgeJSC.cpp:110 > - ASSERT(m_rootObject->isValid()); > - m_rootObject->removeRuntimeObject(object); > + if (m_rootObject->isValid()) > + m_rootObject->removeRuntimeObject(object); Good to remove the assertion, but why check isValid here since you already added the check to removeRuntimeObject? I added the isValid check in RootObject::removeRuntimeObject() so that a future caller of the method other than Instance:: willDestroyRuntimeObject() won't expose this race issue. The check in willDestroyRuntimeObject() makes it so that RootObject::removeRuntimeObject() won't be called. Therefore the check is only made once in the current case. (In reply to comment #9) > The check in willDestroyRuntimeObject() makes it so that RootObject::removeRuntimeObject() won't be called. Therefore the check is only made once in the current case. If invalid, the check is made once. If valid, the check is made twice. No need for double checks here. Made the change suggested by the reviewer before checking in. *** Bug 54022 has been marked as a duplicate of this bug. *** Committed r77979: <http://trac.webkit.org/changeset/77979> (In reply to comment #13) > Committed r77979: <http://trac.webkit.org/changeset/77979> From your commit: Index: trunk/LayoutTests/platform/gtk/Skipped =================================================================== --- a/trunk/LayoutTests/platform/gtk/Skipped +++ b/trunk/LayoutTests/platform/gtk/Skipped @@ -4643,6 +4643,4 @@ http/tests/websocket/tests/cross-origin.html -# These tests fail ASSERT(m_rootObject->isValid()); on the GTK+ debug bots. -# It should be unskipped when https://bugs.webkit.org/show_bug.cgi?id=53716 is fixed. -fast/frames/set-parent-src-synchronously.html -fast/frames/sandboxed-iframe-storage.html +# GTK+ DRT should support enabling and disabling the icon database +http/tests/inspector/resource-har-conversion.html I think you mistakenly reskipped a test I just unskipped. Looks like a merge issue. I'll unskip again. http://trac.webkit.org/changeset/77979 might have broken Qt Linux Release The following tests are not passing: fast/forms/input-maxlength-unsupported.html fast/forms/input-number-keyoperation.html fast/forms/input-number-unacceptable-style.html |