Bug 53716 - JSC::Bindings m_rootObject->isValid() assert fails when running layout tests
Summary: JSC::Bindings m_rootObject->isValid() assert fails when running layout tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, LayoutTestFailure
: 53991 54022 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-02-03 14:09 PST by Mihai Parparita
Modified: 2011-02-08 15:57 PST (History)
12 users (show)

See Also:


Attachments
Patch to change failing ASSERT to conditionally remove RuntimeObject (3.48 KB, patch)
2011-02-08 13:35 PST, Michael Saboff
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 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)
Comment 1 Adam Roben (:aroben) 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
Comment 2 Adam Roben (:aroben) 2011-02-03 14:19:34 PST
<rdar://problem/8955878>
Comment 3 Martin Robinson 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
Comment 4 Adam Roben (:aroben) 2011-02-05 07:20:14 PST
We should skip these tests for now.
Comment 5 Martin Robinson 2011-02-05 10:34:53 PST
Committed r77747: <http://trac.webkit.org/changeset/77747>
Comment 6 Martin Robinson 2011-02-08 07:39:35 PST
*** Bug 53991 has been marked as a duplicate of this bug. ***
Comment 7 Michael Saboff 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.
Comment 8 Darin Adler 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?
Comment 9 Michael Saboff 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.
Comment 10 Darin Adler 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.
Comment 11 Michael Saboff 2011-02-08 15:03:38 PST
Made the change suggested by the reviewer before checking in.
Comment 12 Michael Saboff 2011-02-08 15:04:22 PST
*** Bug 54022 has been marked as a duplicate of this bug. ***
Comment 13 Michael Saboff 2011-02-08 15:10:32 PST
Committed r77979: <http://trac.webkit.org/changeset/77979>
Comment 14 Martin Robinson 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.
Comment 15 WebKit Review Bot 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