Bug 53716 – JSC::Bindings m_rootObject->isValid() assert fails when running layout tests

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mihai Parparita]

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)

[Adam Roben (:aroben)]

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

<rdar://problem/8955878>

[Martin Robinson]

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

We should skip these tests for now.

[Martin Robinson]

Committed r77747: <http://trac.webkit.org/changeset/77747>

[Martin Robinson]

*** Bug 53991 has been marked as a duplicate of this bug. ***

[Michael Saboff]

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 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]

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]

(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]

Made the change suggested by the reviewer before checking in.

[Michael Saboff]

*** Bug 54022 has been marked as a duplicate of this bug. ***

[Michael Saboff]

Committed r77979: <http://trac.webkit.org/changeset/77979>

[Martin Robinson]

(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]

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