Bug 53716

Summary: JSC::Bindings m_rootObject->isValid() assert fails when running layout tests
Product: WebKit Reporter: Mihai Parparita <mihaip>
Component: JavaScriptCoreAssignee: 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 Flags
Patch to change failing ASSERT to conditionally remove RuntimeObject darin: review+

Mihai Parparita
Reported 2011-02-03 14:09:33 PST
To reproduce, with a debug build run: run-webkit-tests fast/events/recorded-keydown-event.html fast/events/tabindex-focus-blur-all.html Output is: FlateDecode: decoding error: incorrect data check. ASSERTION FAILED: m_rootObject->isValid() (/Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/Source/WebCore/bridge/jsc/BridgeJSC.cpp:109 void JSC::Bindings::Instance::willDestroyRuntimeObject(JSC::Bindings::RuntimeObject*)) -> JSC::Bindings::Instance::willDestroyRuntimeObject(JSC::Bindings::RuntimeObject*) -> JSC::Bindings::RuntimeObject::~RuntimeObject() -> WebKit::ProxyRuntimeObject::~ProxyRuntimeObject() -> JSC::MarkedSpace::allocate(unsigned long) -> JSC::Heap::allocate(unsigned long) -> JSC::JSCell::operator new(unsigned long, JSC::ExecState*) -> JSC::BooleanPrototype::BooleanPrototype(JSC::ExecState*, JSC::JSGlobalObject*, WTF::NonNullPassRefPtr<JSC::Structure>, JSC::Structure*) -> JSC::JSGlobalObject::reset(JSC::JSValue) -> JSC::JSGlobalObject::init(JSC::JSObject*) -> JSC::JSGlobalObject::JSGlobalObject(WTF::NonNullPassRefPtr<JSC::Structure>, JSC::JSGlobalObject::JSGlobalObjectData*, JSC::JSObject*) -> WebCore::JSDOMGlobalObject::JSDOMGlobalObject(WTF::NonNullPassRefPtr<JSC::Structure>, WebCore::JSDOMGlobalObject::JSDOMGlobalObjectData*, JSC::JSObject*) -> WebCore::JSDOMWindowBase::JSDOMWindowBase(WTF::NonNullPassRefPtr<JSC::Structure>, WTF::PassRefPtr<WebCore::DOMWindow>, WebCore::JSDOMWindowShell*) -> WebCore::JSDOMWindow::JSDOMWindow(WTF::NonNullPassRefPtr<JSC::Structure>, WTF::PassRefPtr<WebCore::DOMWindow>, WebCore::JSDOMWindowShell*) -> WebCore::JSDOMWindowShell::setWindow(WTF::PassRefPtr<WebCore::DOMWindow>) -> WebCore::ScriptController::clearWindowShell(bool) -> WebCore::Frame::pageDestroyed() -> WebCore::FrameLoader::closeAndRemoveChild(WebCore::Frame*) -> WebCore::FrameLoader::detachFromParent() -> WebCore::FrameLoader::detachChildren() -> WebCore::FrameLoader::detachFromParent() -> WebCore::FrameLoader::frameDetached() -> WebCore::HTMLFrameOwnerElement::willRemove() -> WebCore::HTMLFrameElementBase::willRemove() -> WebCore::ContainerNode::willRemove() -> WebCore::ContainerNode::willRemove() -> WebCore::willRemoveChildren(WebCore::ContainerNode*) -> WebCore::ContainerNode::removeChildren() -> WebCore::Document::implicitOpen() -> WebCore::Document::open(WebCore::Document*) -> WebCore::Document::write(WebCore::SegmentedString const&, WebCore::Document*) -> WebCore::documentWrite(JSC::ExecState*, WebCore::HTMLDocument*, WebCore::NewlineRequirement) (that assert was added by http://trac.webkit.org/changeset/48513)
Attachments
Patch to change failing ASSERT to conditionally remove RuntimeObject (3.48 KB, patch)
2011-02-08 13:35 PST, Michael Saboff 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Adam Roben (:aroben)
Comment 1 2011-02-03 14:16:28 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
Adam Roben (:aroben)
Comment 2 2011-02-03 14:19:34 PST
<rdar://problem/8955878>
Martin Robinson
Comment 3 2011-02-04 20:13:10 PST
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
Adam Roben (:aroben)
Comment 4 2011-02-05 07:20:14 PST
We should skip these tests for now.
Martin Robinson
Comment 5 2011-02-05 10:34:53 PST
Committed r77747: <http://trac.webkit.org/changeset/77747>
Martin Robinson
Comment 6 2011-02-08 07:39:35 PST
*** Bug 53991 has been marked as a duplicate of this bug. ***
Michael Saboff
Comment 7 2011-02-08 13:35:00 PST
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.
Darin Adler
Comment 8 2011-02-08 13:38:33 PST
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?
Michael Saboff
Comment 9 2011-02-08 14:03:47 PST
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.
Darin Adler
Comment 10 2011-02-08 14:06:06 PST
(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.
Michael Saboff
Comment 11 2011-02-08 15:03:38 PST
Made the change suggested by the reviewer before checking in.
Michael Saboff
Comment 12 2011-02-08 15:04:22 PST
*** Bug 54022 has been marked as a duplicate of this bug. ***
Michael Saboff
Comment 13 2011-02-08 15:10:32 PST
Committed r77979: <http://trac.webkit.org/changeset/77979>
Martin Robinson
Comment 14 2011-02-08 15:13:21 PST
(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.
WebKit Review Bot
Comment 15 2011-02-08 15:57:47 PST
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
Note You need to log in before you can comment on or make changes to this bug.