| Summary: | [Qt] REGRESSION(r118616): It made all tests crash in debug mode | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Blocker | CC: | cmarcelo, ggaren, loki, noam, oliver, ossy, webkit.review.bot, zherczeg | ||||||
| Priority: | P1 | Keywords: | Qt, QtTriaged | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 79668, 87581 | ||||||||
| Attachments: |
|
||||||||
|
Description
Csaba Osztrogonác
2012-05-28 23:49:30 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. (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())); } Maybe https://bugs.webkit.org/show_bug.cgi?id=87750 is related to this bug too. 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. 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? (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. Created attachment 145074 [details]
I am here now
After playing with different combinations, this one seems working. Oszi, could you check it?
(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). 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 (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 ... Ok, so shall we land this as a temporary fix? If so, I make it a nice patch. Created attachment 145988 [details]
patch
There was no objection, and the bots are still dead, so ...
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.
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 :) Landed as http://trac.webkit.org/changeset/119584 |