Bug 53912 – [Qt] Fix crashes in QMetaObject::metacall

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Csaba Osztrogonác]

After r77705 there are many crashes on Qt buildbot.

[Csaba Osztrogonác]

http/tests/websocket/tests/websocket-pending-activity.html crashes intermittently

I'm going to find which changeset caused it. (r77705 is innocent)

[Csaba Osztrogonác]

Adding http/tests/websocket/tests/websocket-pending-activity.html to the Skipped list (http://trac.webkit.org/changeset/77855) made the following test http/tests/websocket/tests/websocket-protocol-ignored.html crash. I added it to the Skipped list too. (http://trac.webkit.org/changeset/77911)

[Csaba Osztrogonác]

Created attachment 81601 [details]
crash log of http/tests/websocket/tests/websocket-protocol-ignored.html (r77906)

[Csaba Osztrogonác]

Created attachment 81614 [details]
crash log of fast/text/international/text-combine-parser-test.html (r77156) on 64 bit

[Csaba Osztrogonác]

The first occurence of crashes on 64 bit appeared after http://trac.webkit.org/changeset/77151 and its buildfix http://trac.webkit.org/changeset/77156.

[Csaba Osztrogonác]

Created attachment 81618 [details]
crash log of fast/frames/sandboxed-iframe-noscript.html (r77159) on 32 bit in debug mode

fast/frames/sandboxed-iframe-noscript.html started to crash from r77159 in debug mode

[Csaba Osztrogonác]

I modified the importance to P1/blocker, because this bug cause flakey crashes frequently on our "Qt Linux Release" core builder bot. This flakiness made sheriffbot spamming many bugs with false positive comments like:
"http://trac.webkit.org/changeset/XXXXX might have broken Qt Linux Release The following tests are not passing: ..."

We don't want to rightly angry people remove Qt bots from core builders list, because after that nobody would care about Qt bots, and the build of QtWebKit would be always broken. But we can't avoid this catastrophic consequence if none of Qt/JSC expert cares about this bug as soon as possible.

Any volunteer? Zoltan? Kent?

[Csaba Osztrogonác]

you can reproduce crashing of fast/frames/sandboxed-iframe-noscript.html easily:

$ Tools/Scripts/run-webkit-tests --debug fast/frames/sandboxed-iframe-navigation-top.html fast/frames/sandboxed-iframe-navigation-windowopen.html fast/frames/sandboxed-iframe-noscript.html

[Zoltan Herczeg]

gdb commands:
  break Source/WebCore/bridge/runtime_root.cpp:88
  ignore 1 20

This RootObject will cause the crash, when it is invalidated by:

#0  JSC::Bindings::RootObject::invalidate (this=0x8374e08)
    at ../../../Source/WebCore/bridge/runtime_root.cpp:115
#1  0xb65f6ce3 in ~ScriptController (this=0x836a4ec)
    at ../../../Source/WebCore/bindings/js/ScriptController.cpp:89
#2  0xb6bcc3e4 in ~Frame (this=0x836a1d0) at ../../../Source/WebCore/page/Frame.cpp:245
#3  0xb658ba71 in WTF::RefCounted<WebCore::Frame>::deref (this=0x836a1d4)
    at ../../../Source/JavaScriptCore/wtf/RefCounted.h:141
#4  0xb658bab3 in WTF::derefIfNotNull<WebCore::Frame> (ptr=0x836a1d0)
    at ../../../Source/JavaScriptCore/wtf/PassRefPtr.h:59
#5  0xb658bad9 in ~RefPtr (this=0xb270bc64) at ../../../Source/JavaScriptCore/wtf/RefPtr.h:58
#6  0xb6bfe1fe in ~Page (this=0xb270bc38) at ../../../Source/WebCore/page/Page.cpp:207
#7  0xb6f3d8ac in ~QWebPagePrivate (this=0xb270b308) at ../../../Source/WebKit/qt/Api/qwebpage.cpp:354
#8  0xb6f3dd27 in ~QWebPage (this=0xb270b128) at ../../../Source/WebKit/qt/Api/qwebpage.cpp:1945
#9  0x08068618 in ~WebPage (this=0xb270b128)
    at ../../../../Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:168
#10 0xb4267676 in QObjectPrivate::deleteChildren ()
   from /usr/local/Trolltech/Qt-4.7.1/lib/libQtCore.so.4
#11 0xb426e5df in QObject::~QObject () from /usr/local/Trolltech/Qt-4.7.1/lib/libQtCore.so.4

[Zoltan Herczeg]

Source/WebCore/bindings/js/ScriptController.cpp

ScriptController::~ScriptController()
{
    disconnectPlatformScriptObjects();

    if (m_cacheableBindingRootObject) {
        m_cacheableBindingRootObject->invalidate();
        m_cacheableBindingRootObject = 0;
    }

    // It's likely that destroying m_windowShells will create a lot of garbage.
    if (!m_windowShells.isEmpty()) {
        while (!m_windowShells.isEmpty())
            destroyWindowShell(m_windowShells.begin()->first.get());
        gcController().garbageCollectSoon();
    }
}

Actually we call the invalidate(); here manually, and during the gcController().garbageCollectSoon() it seems we need that m_cacheableBindingRootObject must be valid. This is the issu, I have no idea how to fix it, because I don't understand the mechanics here. Could we reorder the two ifs?

[Csaba Osztrogonác]

http://trac.webkit.org/changeset/61062 (Robert)
http://trac.webkit.org/changeset/57533 (Geoffrey)

Geoffrey and Robert, you implemented the code which Zoltan mentioned in Comment #10 . Have you got any idea for fixing this bug?

Or Michael, could you give us a hint? fast/frames/sandboxed-iframe-noscript.html started to crash after your patch: htttp://trac.webkit.org/changeset/76969 (Potentially Unsafe HashSet of RuntimeObject* in RootObject definition )

[Geoffrey Garen]

It may be reasonable to remove that ASSERT. It's not clear to me how you could maintain the invariant that the root object is valid throughout the lifetime of a garbage collected object, since such an object can have unpredictable lifetime.

[Michael Saboff]

I believe the change I'm going to make for https://bugs.webkit.org/show_bug.cgi?id=53716 will fix this as well.  Basically changing Source/WebCore/bridge/jsc/BridgeJSC.cpp

From:
 void Instance::willDestroyRuntimeObject(RuntimeObject* object)
 {
     ASSERT(m_rootObject);
    ASSERT(m_rootObject->isValid());
    m_rootObject->removeRuntimeObject(object);
     m_runtimeObject.clear(object);
 }

To: 
 void Instance::willDestroyRuntimeObject(RuntimeObject* object)
 {
     ASSERT(m_rootObject);
    if (m_rootObject->isValid())
        m_rootObject->removeRuntimeObject(object);
     m_runtimeObject.clear(object);
 }

[Csaba Osztrogonác]

(In reply to comment #13)
> I believe the change I'm going to make for https://bugs.webkit.org/show_bug.cgi?id=53716 will fix this as well.  Basically changing Source/WebCore/bridge/jsc/BridgeJSC.cpp
Your fix resolved the "ASSERTION FAILED: m_rootObject->isValid()", thanks for it.

But unfortunately the other crashes haven't been fixed yet.

[Csaba Osztrogonác]

Comment on attachment 81618 [details]
crash log of fast/frames/sandboxed-iframe-noscript.html (r77159) on 32 bit in debug mode

fixed

[Csaba Osztrogonác]

I made Qt Linux Release core builder happy with skipping http/tests/websocket/tests directory: http://trac.webkit.org/changeset/78039

But it is paper overing the problem, we should find the real fix.
I'm going find how we can reproduce crashes on a smaller example.

[Andreas Kling]

Relaxing this one a bit.
Also, I can't reproduce any of the issues supposedly still pending. Ossy? :3

[Csaba Osztrogonác]

I can't reproduce it now, so let's try to unskip websocket tests: http://trac.webkit.org/changeset/85743