Full Text Bug Listing

Csaba Osztrogonác

Reported 2012-05-28 23:49:30 PDT

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

Comment 1 2012-05-29 00:49:42 PDT

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

Comment 2 2012-05-29 06:20:17 PDT

(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

Comment 3 2012-05-29 08:56:51 PDT

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

Geoffrey Garen

Comment 4 2012-05-29 11:02:54 PDT

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

Comment 5 2012-05-30 07:08:40 PDT

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

Comment 6 2012-05-30 10:17:18 PDT

(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

Comment 7 2012-05-31 06:17:49 PDT

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

Comment 8 2012-05-31 08:21:59 PDT

(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 9 2012-05-31 08:48:13 PDT

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

Comment 10 2012-05-31 08:52:09 PDT

(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

Comment 11 2012-06-01 02:29:29 PDT

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

Zoltan Herczeg

Comment 12 2012-06-06 04:25:56 PDT

Created attachment 145988 [details] patch There was no objection, and the bots are still dead, so ...

WebKit Review Bot

Comment 13 2012-06-06 04:27:26 PDT

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 14 2012-06-06 04:28:45 PDT

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

Comment 15 2012-06-06 04:35:54 PDT

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

Attachments
I am here now (7.38 KB, patch) 2012-05-31 06:17 PDT, Zoltan Herczeg	no flags	Details Formatted Diff Diff
patch (8.87 KB, patch) 2012-06-06 04:25 PDT, Zoltan Herczeg	ossy: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Bug 87701