Bug 87701 – [Qt] REGRESSION(r118616): It made all tests crash in debug mode

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r118616%20%2823100%29/results.html

Unfortunately r118616 made all tests crash on Qt (32/64 bit too) 
in debug mode with the following crash log:
crash log for DumpRenderTree (pid 7699):
STDOUT: <empty>
STDERR: 1   0x42e04c /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/bin/DumpRenderTree() [0x42e04c]
STDERR: 2   0x7f966309c230 /lib/libc.so.6(+0x32230) [0x7f966309c230]
STDERR: 3   0x7f966afe0e78 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::WriteBarrierBase<JSC::Structure>::unvalidatedGet() const+0xc) [0x7f966afe0e78]
STDERR: 4   0x7f966afdf87e /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::JSCell::unvalidatedStructure()+0x1c) [0x7f966afdf87e]
STDERR: 5   0x7f966c7678a9 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::slowValidateCell(JSC::JSCell*)+0x44) [0x7f966c7678a9]
STDERR: 6   0x7f966ae66a3b /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(void JSC::validateCell<JSC::JSCell*>(JSC::JSCell*)+0x18) [0x7f966ae66a3b]
STDERR: 7   0x7f966b00e303 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::WriteBarrierBase<JSC::JSObject>::get() const+0x27) [0x7f966b00e303]
STDERR: 8   0x7f966b0c5950 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::Bindings::QtInstance::removeCachedMethod(JSC::JSObject*)+0x4e) [0x7f966b0c5950]
STDERR: 9   0x7f966b0de75f /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::Bindings::QtRuntimeMethodData::finalize(JSC::Handle<JSC::Unknown>, void*)+0x4f) [0x7f966b0de75f]
STDERR: 10  0x7f966c583466 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::WeakBlock::finalize(JSC::WeakImpl*)+0xec) [0x7f966c583466]
STDERR: 11  0x7f966c582dfd /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::WeakBlock::sweep()+0x91) [0x7f966c582dfd]
STDERR: 12  0x7f966c582222 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::WeakSet::sweep()+0x76) [0x7f966c582222]
STDERR: 13  0x7f966c59949e /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::MarkedBlock::sweepWeakSet()+0x1e) [0x7f966c59949e]
STDERR: 14  0x7f966c599a5e /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::SweepWeakSet::operator()(JSC::MarkedBlock*)+0x1c) [0x7f966c599a5e]
STDERR: 15  0x7f966c59ab77 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(void JSC::MarkedAllocator::forEachBlock<JSC::SweepWeakSet>(JSC::SweepWeakSet&)+0x49) [0x7f966c59ab77]
STDERR: 16  0x7f966c59a757 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::SweepWeakSet::ReturnType JSC::MarkedSpace::forEachBlock<JSC::SweepWeakSet>(JSC::SweepWeakSet&)+0x65) [0x7f966c59a757]
STDERR: 17  0x7f966c59a1d9 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::SweepWeakSet::ReturnType JSC::MarkedSpace::forEachBlock<JSC::SweepWeakSet>()+0x1f) [0x7f966c59a1d9]
STDERR: 18  0x7f966c598ebc /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::MarkedSpace::sweepWeakSets()+0x18) [0x7f966c598ebc]
STDERR: 19  0x7f966c587e23 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::Heap::collect(JSC::Heap::SweepToggle)+0x19d) [0x7f966c587e23]
STDERR: 20  0x7f966c587c81 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(JSC::Heap::collectAllGarbage()+0x2f) [0x7f966c587c81]
STDERR: 21  0x7f966afe29d7 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(+0x20519d7) [0x7f966afe29d7]
STDERR: 22  0x7f966afe2c16 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::GCController::gcTimerFired(WebCore::Timer<WebCore::GCController>*)+0x1a) [0x7f966afe2c16]
STDERR: 23  0x7f966afe2e44 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::Timer<WebCore::GCController>::fired()+0x6e) [0x7f966afe2e44]
STDERR: 24  0x7f966b98bf14 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ThreadTimers::sharedTimerFiredInternal()+0xc6) [0x7f966b98bf14]
STDERR: 25  0x7f966b98be4b /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ThreadTimers::sharedTimerFired()+0x19) [0x7f966b98be4b]
STDERR: 26  0x7f966bc925e2 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::SharedTimerQt::timerEvent(QTimerEvent*)+0x6a) [0x7f966bc925e2]
STDERR: 27  0x7f966470f519 /usr/local/Trolltech/Qt-4.8.0/lib/libQtCore.so.4(QObject::event(QEvent*)+0xa9) [0x7f966470f519]
STDERR: 28  0x7f9664fcb19c /usr/local/Trolltech/Qt-4.8.0/lib/libQtGui.so.4(QApplicationPrivate::notify_helper(QObject*, QEvent*)+0xac) [0x7f9664fcb19c]
STDERR: 29  0x7f9664fd206d /usr/local/Trolltech/Qt-4.8.0/lib/libQtGui.so.4(QApplication::notify(QObject*, QEvent*)+0x13d) [0x7f9664fd206d]
STDERR: 30  0x7f96646fd0ec /usr/local/Trolltech/Qt-4.8.0/lib/libQtCore.so.4(QCoreApplication::notifyInternal(QObject*, QEvent*)+0x8c) [0x7f96646fd0ec]
STDERR: 31  0x7f966473152e /usr/local/Trolltech/Qt-4.8.0/lib/libQtCore.so.4(+0x1cf52e) [0x7f966473152e]
STDERR: LEAK: 68 WebCoreNode
STDERR: LEAK: 1 CachedResource
STDERR: LEAK: 1 SubresourceLoader
STDERR: LEAK: 1 Frame
STDERR: LEAK: 1 Page
STDERR: LEAK: 3 RenderObject

Could you check what happened?

[Geoffrey Garen]

QtInstance::m_methods needs to be a HashMap with a Weak<JSObject> value type. I would make this change myself but I'm not familiar with QHash and QByteArray, and the QtInstance object model has a lot of friend relationships and poor modularity, which makes it hard for me to have confidence in a code change just by reading a subsection of the Qt codebase.

[Zoltan Herczeg]

(In reply to comment #1)
> QtInstance::m_methods needs to be a HashMap with a Weak<JSObject> value type. I would make this change myself but I'm not familiar with QHash and QByteArray, and the QtInstance object model has a lot of friend relationships and poor modularity, which makes it hard for me to have confidence in a code change just by reading a subsection of the Qt codebase.

Hi Geoff, thanks for the help. I know the design is flawed there, but still I would like to fix it. However, I got all kinds of compiler errors (when playing with PassWeak), so please tell me how this thing should work.

Perhaps some different implementation for this could also be help:

void QtRuntimeMethodData::finalize(Handle<Unknown> value, void*)
{
    m_instance->removeCachedMethod(static_cast<JSObject*>(value.get().asCell()));
}

[Csaba Osztrogonác]

Maybe https://bugs.webkit.org/show_bug.cgi?id=87750 is related to this bug too.

[Geoffrey Garen]

Here's a rough cut at the changes needed:

> void QtInstance::removeCachedMethod(JSObject* method)

*it will be NULL at finalization time. To maintain current behavior, I guess you could just check for NULL instead of == method. Any item that is null is garbage and can be safely removed from the table.

It would be event better to avoid O(n) search here by removing by key. To do that, use the weakRemove helper function.

>    JSObject* val = qtinst->m_methods.value(name).get();

HashMap will automatically return NULL for dead Weak pointers, so no change needed here.

>             qtinst->m_methods.insert(name, WriteBarrier<JSObject>(exec->globalData(), qtinst->createRuntimeObject(exec), val));

You want HashMap::set here, to guarantee an overwrite of a NULL Weak pointer.

>             qtinst->m_methods.insert(name, WriteBarrier<JSObject>(exec->globalData(), qtinst->createRuntimeObject(exec), val));

Ditto.

>     if (qtinst->m_methods.contains(ascii))

This should be HashMap::get. contains is wrong because it will return true even if the contained item is NULL.

>     // clean up (unprotect from gc) the JSValues we've created
>    m_methods.clear();

This can go away, since it's automatic for HashMap.


>    for (QHash<QByteArray, WriteBarrier<JSObject> >::Iterator it = m_methods.begin(), end = m_methods.end(); it != end; ++it)
>        visitor.append(&it.value());

Iterating a HashMap of Weak pointers you need to test the iterator for NULL before dereferencing it.

Not sure what Kind of PassWeak errors you got. I might be able to help you if you paste them.

[Zoltan Herczeg]

Thanks for the long reply.

Now I am stuck here:

../../../../Source/WebCore/bridge/qt/qt_instance.cpp:188: error: no matching function for call to âJSC::SlotVisitor::append(JSC::Weak<JSC::JSObject>*)â
../../../../Source/JavaScriptCore/heap/MarkStack.h:212: note: candidates are: void JSC::MarkStack::append(JSC::ConservativeRoots&)
../../../../Source/JavaScriptCore/heap/MarkStack.h:412: note:                 void JSC::MarkStack::append(JSC::JSValue*)
../../../../Source/JavaScriptCore/heap/MarkStack.h:393: note:                 void JSC::MarkStack::append(JSC::JSValue*, size_t)
../../../../Source/JavaScriptCore/heap/MarkStack.h:418: note:                 void JSC::MarkStack::append(JSC::JSCell**)

SlotVisitor (I think used by GC) needs a JSC::JSCell**, but I only have a Weak<JSC::JSObject>. How can I squeeze the pointer out from it?

And I think we can leave with NULL pointers for those items which already have been freed (the number of possible names are fixed). Thus, can I just remove the removeCachedMethod at all?

[Geoffrey Garen]

(In reply to comment #5)
> Thanks for the long reply.
> 
> Now I am stuck here:
> 
> ../../../../Source/WebCore/bridge/qt/qt_instance.cpp:188: error: no matching function for call to âJSC::SlotVisitor::append(JSC::Weak<JSC::JSObject>*)â
> ../../../../Source/JavaScriptCore/heap/MarkStack.h:212: note: candidates are: void JSC::MarkStack::append(JSC::ConservativeRoots&)
> ../../../../Source/JavaScriptCore/heap/MarkStack.h:412: note:                 void JSC::MarkStack::append(JSC::JSValue*)
> ../../../../Source/JavaScriptCore/heap/MarkStack.h:393: note:                 void JSC::MarkStack::append(JSC::JSValue*, size_t)
> ../../../../Source/JavaScriptCore/heap/MarkStack.h:418: note:                 void JSC::MarkStack::append(JSC::JSCell**)
> 
> SlotVisitor (I think used by GC) needs a JSC::JSCell**, but I only have a Weak<JSC::JSObject>. How can I squeeze the pointer out from it?

Why do you want these pointers to be weak and also marked? The compiler is telling you that's not compatible with the GC's design. If the pointers are strong, the object that owns them can clear / destroy the table when it is destroyed. If they are weak, they should not be marked.

> And I think we can leave with NULL pointers for those items which already have been freed (the number of possible names are fixed). Thus, can I just remove the removeCachedMethod at all?

You can -- but NULL items in the table are a small memory leak. Not sure what your requirements are here.

[Zoltan Herczeg]

Created attachment 145074 [details]
I am here now

After playing with different combinations, this one seems working. Oszi, could you check it?

[Caio Marcelo de Oliveira Filho]

(In reply to comment #7)
> Created an attachment (id=145074) [details]
> I am here now
> 
> After playing with different combinations, this one seems working. Oszi, could you check it?

I have a couple of questions here: 

1) It's not clear to me that we need weak references for the methods, instead of keeping just references. Once the Instance is removed, we just need to tell the cached methods, if still around, that its associated instance is not there anymore.

2) Do we really need this method cache? Other bridges seems to handle without it.

3) Regardless we land this fix or not now. As proposed before by others before (including Oliver), I think a better long term solution here is to just rewrite the QtRuntimeMethod and subclasses with the JSC API. But in this case I don't see how to keep "Weak" references.

Once I understand (1), I can volunteer to work on (3).

[Csaba Osztrogonác]

Comment on attachment 145074 [details]
I am here now

View in context: https://bugs.webkit.org/attachment.cgi?id=145074&action=review

I tested it on 64 bit debug mode, and everything works. ;)
But a JSC and/or Qt expert should review the patch.

> Source/WebCore/bridge/qt/qt_instance.h:88
> +    class QTWeakObjectReference {

QTWeakObjectReference  -> QtWeakObjectReference

[Csaba Osztrogonác]

(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=145074) [details] [details]
> > I am here now
> > 
> > After playing with different combinations, this one seems working. Oszi, could you check it?
> 
> I have a couple of questions here: 
> 
> 1) It's not clear to me that we need weak references for the methods, instead of keeping just references. Once the Instance is removed, we just need to tell the cached methods, if still around, that its associated instance is not there anymore.
> 
> 2) Do we really need this method cache? Other bridges seems to handle without it.
> 
> 3) Regardless we land this fix or not now. As proposed before by others before (including Oliver), I think a better long term solution here is to just rewrite the QtRuntimeMethod and subclasses with the JSC API. But in this case I don't see how to keep "Weak" references.
> 
> Once I understand (1), I can volunteer to work on (3).

Here is the very very old bug report to fix (3): https://bugs.webkit.org/show_bug.cgi?id=60842 ... But it seems nobody is really interested in fixing it ...

[Zoltan Herczeg]

Ok, so shall we land this as a temporary fix? If so, I make it a nice patch.

[Zoltan Herczeg]

Created attachment 145988 [details]
patch

There was no objection, and the bots are still dead, so ...

[WebKit Review Bot]

Attachment 145988 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bridge/qt/qt_instance.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Csaba Osztrogonác]

Comment on attachment 145988 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145988&action=review

rs=me, we need this fix for broken debug tests.

> Source/WebCore/bridge/qt/qt_instance.h:88
> +    class QTWeakObjectReference {

s/QTWeakObjectReference /QtWeakObjectReference/g :)

[Zoltan Herczeg]

Landed as http://trac.webkit.org/changeset/119584