Bug 53912 - [Qt] Fix crashes in QMetaObject::metacall
Summary: [Qt] Fix crashes in QMetaObject::metacall
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 53488
  Show dependency treegraph
 
Reported: 2011-02-07 04:16 PST by Csaba Osztrogonác
Modified: 2011-05-04 06:48 PDT (History)
11 users (show)

See Also:


Attachments
crash log of http/tests/websocket/tests/websocket-protocol-ignored.html (r77906) (2.88 KB, text/plain)
2011-02-07 23:39 PST, Csaba Osztrogonác
no flags Details
crash log of fast/text/international/text-combine-parser-test.html (r77156) on 64 bit (6.23 KB, text/plain)
2011-02-08 02:09 PST, Csaba Osztrogonác
no flags Details
crash log of fast/frames/sandboxed-iframe-noscript.html (r77159) on 32 bit in debug mode (10.44 KB, text/plain)
2011-02-08 03:30 PST, Csaba Osztrogonác
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2011-02-07 04:16:10 PST
After r77705 there are many crashes on Qt buildbot.
Comment 1 Csaba Osztrogonác 2011-02-07 14:44:13 PST
http/tests/websocket/tests/websocket-pending-activity.html crashes intermittently

I'm going to find which changeset caused it. (r77705 is innocent)
Comment 2 Csaba Osztrogonác 2011-02-07 23:32:25 PST
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)
Comment 3 Csaba Osztrogonác 2011-02-07 23:39:15 PST
Created attachment 81601 [details]
crash log of http/tests/websocket/tests/websocket-protocol-ignored.html (r77906)
Comment 4 Csaba Osztrogonác 2011-02-08 02:09:02 PST
Created attachment 81614 [details]
crash log of fast/text/international/text-combine-parser-test.html (r77156) on 64 bit
Comment 5 Csaba Osztrogonác 2011-02-08 02:15:43 PST
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.
Comment 6 Csaba Osztrogonác 2011-02-08 03:30:00 PST
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
Comment 7 Csaba Osztrogonác 2011-02-08 04:05:27 PST
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?
Comment 8 Csaba Osztrogonác 2011-02-08 04:22:59 PST
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
Comment 9 Zoltan Herczeg 2011-02-08 06:22:52 PST
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
Comment 10 Zoltan Herczeg 2011-02-08 06:43:16 PST
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?
Comment 11 Csaba Osztrogonác 2011-02-08 07:08:57 PST
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 )
Comment 12 Geoffrey Garen 2011-02-08 10:42:26 PST
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.
Comment 13 Michael Saboff 2011-02-08 12:25:10 PST
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);
 }
Comment 14 Csaba Osztrogonác 2011-02-09 00:14:10 PST
(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.
Comment 15 Csaba Osztrogonác 2011-02-09 02:54:22 PST
Comment on attachment 81618 [details]
crash log of fast/frames/sandboxed-iframe-noscript.html (r77159) on 32 bit in debug mode

fixed
Comment 16 Csaba Osztrogonác 2011-02-09 05:30:48 PST
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.
Comment 17 Andreas Kling 2011-03-29 06:39:43 PDT
Relaxing this one a bit.
Also, I can't reproduce any of the issues supposedly still pending. Ossy? :3
Comment 18 Csaba Osztrogonác 2011-05-04 06:48:15 PDT
I can't reproduce it now, so let's try to unskip websocket tests: http://trac.webkit.org/changeset/85743