[Qt] Replace use of internal Weak smart pointer with JSWeakObjectMap
Created attachment 158057 [details] Patch
Comment on attachment 158057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158057&action=review > Source/WebCore/ChangeLog:16 > + The intent of replacing the use of the internal Weak smart pointer with > + public JSC C API is slightly more complex. JSWeakObjectMap is special > + in the sense that its life-time is tied to the life-time of the JS > + global object, which in turn is subject to garbage collection. In order > + to maximize re-use of the same map across different JSContextRef instances, > + we use the associated global context ref as owner of the weak maps. The key > + in the weak map is the identity (pointer) of the runtime method object itself. > + The iteration through the maps is tedious, but should usually not go beyond > + just a few. Would be nice if you wrote about the advantage of switching to the JSWeakObjectMap. Like what does it solve
Created attachment 158064 [details] Patch New version explaining the motivation :)
Comment on attachment 158064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158064&action=review > Source/WebCore/bridge/qt/qt_runtime.cpp:1320 > + if (obj) { > + JSObjectSetPrivate(obj, 0); > + JSWeakObjectMapRemove(it->first, it->second, this); Since we don't expect obj to be in more than one map, could we return here after finding one?
Comment on attachment 158064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158064&action=review > Source/WebCore/bridge/qt/qt_runtime.cpp:1303 > + > + double newline :-) > Source/WebCore/bridge/qt/qt_runtime.cpp:1316 > + for (WeakRuntimeMethodMap::const_iterator it = weakRuntimeMethodCache.begin(), end = weakRuntimeMethodCache.end(); > + it != end; ++it) { I know this looks nice, but it differs from how to normally do it. Apparently I was pointed out that the style guide says to not use iterators when being able to iterate knowing the size. I am not sure I agree with that, but just pointing out > Source/WebCore/bridge/qt/qt_runtime.cpp:1318 > + JSObjectRef obj = JSWeakObjectMapGet(it->first, it->second, this); > + if (obj) { merge to one line? > Source/WebCore/bridge/qt/qt_runtime.cpp:1374 > + JSObjectRef cachedMethod = JSWeakObjectMapGet(cache->first, cache->second, this); > + if (cachedMethod) Merge to one line? > Source/WebCore/bridge/qt/qt_runtime.cpp:1412 > + JSWeakObjectMapRef map = 0; why = 0 when you are setting it both places
(In reply to comment #4) > (From update of attachment 158064 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158064&action=review > > > Source/WebCore/bridge/qt/qt_runtime.cpp:1320 > > + if (obj) { > > + JSObjectSetPrivate(obj, 0); > > + JSWeakObjectMapRemove(it->first, it->second, this); > > Since we don't expect obj to be in more than one map, could we return here after finding one? Right now that is indeed not possible, given that the same QObject exposed in two different script environments leads to two different QtInstance objects and thus different RuntimeMethod objects. But in the future we're going to remove QtInstance.
Comment on attachment 158064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158064&action=review Thanks for the review :) >> Source/WebCore/bridge/qt/qt_runtime.cpp:1316 >> + it != end; ++it) { > > I know this looks nice, but it differs from how to normally do it. Apparently I was pointed out that the style guide says to not use iterators when being able to iterate knowing the size. I am not sure I agree with that, but just pointing out The data structure in question is a set, so the iterators are the only way to efficiently iterate. I agree that for vectors indexing is preferred. >> Source/WebCore/bridge/qt/qt_runtime.cpp:1318 >> + if (obj) { > > merge to one line? Good idea, I need to make that pattern a habit :) >> Source/WebCore/bridge/qt/qt_runtime.cpp:1412 >> + JSWeakObjectMapRef map = 0; > > why = 0 when you are setting it both places Unnecessary paranoia :) - I'll remove the initialization.
Created attachment 158073 [details] Patch
LGTM.
Committed r125444: <http://trac.webkit.org/changeset/125444>
Reverted r125444 for reason: Broke some tests Committed r125452: <http://trac.webkit.org/changeset/125452>
(In reply to comment #11) > Reverted r125444 for reason: > > Broke some tests > > Committed r125452: <http://trac.webkit.org/changeset/125452> Example crash log: http://build.webkit.org/results/Qt%20Linux%20Release/r125448%20(50754)/sputnik/Conformance/07_Lexical_Conventions/7.4_Comments/S7.4_A5-crash-log.txt from http://build.webkit.org/builders/Qt%20Linux%20Release/builds/50754 Will look into it tomorrow.
Another way to reproduce the crash: gdb --args /path/to/DRT `find LayoutTests/fast/js`
The crash is caused by accessing the weak map that is associated with a global object that's been deleted already. The solution appears to be to have a separate js context for the weak map that is created in the same context _group_. It is allowed to share objects across contexts in the same group. The weak map has no destructor, it is GC'ed when the context's global object is collected. Therefore we simply refcount on the context that "owns" the weak map. New patch coming up.
Created attachment 158805 [details] Patch
Comment on attachment 158805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158805&action=review r=me... but you already filled that in > Source/WebCore/bridge/qt/qt_instance.cpp:50 > + // Deleted by GC when m_context's globalObject gets collected Missed . though > Source/WebCore/bridge/qt/qt_instance.cpp:67 > + // Just us and the ref in the weakMaps left? Why is this a question?
Comment on attachment 158805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158805&action=review >> Source/WebCore/bridge/qt/qt_instance.cpp:50 >> + // Deleted by GC when m_context's globalObject gets collected > > Missed . though Oops, will fix. >> Source/WebCore/bridge/qt/qt_instance.cpp:67 >> + // Just us and the ref in the weakMaps left? > > Why is this a question? Ok, got the hint ;). Will be a bit more elaborate when landing. Basically if this is the last WeakMap instance left, then we should delete the context from the global weakMaps.
Comment on attachment 158805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158805&action=review > Source/WebCore/bridge/qt/qt_instance.cpp:56 > + if (m_context) Simon, it's not clear to me when this could happen. > Source/WebCore/bridge/qt/qt_instance.cpp:83 > + WeakMapSet::iterator cachedMap = weakMaps.find(group); > + if (cachedMap == weakMaps.end()) { > + m_impl = adoptRef(new WeakMapImpl(group)); > + weakMaps.add(group, m_impl); > + } else > + m_impl = cachedMap->second; Minor suggestion: you could use the HashMap::add idiom here. Instead of find() use add() passing 0 (or nullptr) as new value, if its a new entry you set directly in the value (second) of the iterator add() gives you back.
Comment on attachment 158805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158805&action=review >> Source/WebCore/bridge/qt/qt_instance.cpp:56 >> + if (m_context) > > Simon, it's not clear to me when this could happen. Sorry. I meant to say: when this couldn't happen. I don't see m_context being cleared and I don't think JSC clears it behind our back, so even if the context goes away I would expect this to evaluate to true... Could it even go away while we still hold a retained ref (implicitly retained by Create call)?
Comment on attachment 158805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158805&action=review >>> Source/WebCore/bridge/qt/qt_instance.cpp:56 >>> + if (m_context) >> >> Simon, it's not clear to me when this could happen. > > Sorry. I meant to say: when this couldn't happen. I don't see m_context being cleared and I don't think JSC clears it behind our back, so even if the context goes away I would expect this to evaluate to true... Could it even go away while we still hold a retained ref (implicitly retained by Create call)? You're right, this was a left-over from moving the code around a few iterations earlier. The if is indeed redundant, I'll remove it. Thanks :) >> Source/WebCore/bridge/qt/qt_instance.cpp:83 >> + m_impl = cachedMap->second; > > Minor suggestion: you could use the HashMap::add idiom here. Instead of find() use add() passing 0 (or nullptr) as new value, if its a new entry you set directly in the value (second) of the iterator add() gives you back. Lovely, I'll go for that.
Created attachment 159049 [details] Patch Updated patch for review, taking Caio's comments into account
Committed r125873: <http://trac.webkit.org/changeset/125873>
Comment on attachment 159049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159049&action=review > Source/WebCore/bridge/qt/qt_instance.cpp:80 > + entry.iterator->second = new WeakMapImpl(group); It seems to me that we still need the adoptRef() here. Optionally, we could have a WeakMapImpl::create that calls adoptRef(new ...) and returns a PassRefPtr.
Comment on attachment 159049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159049&action=review >> Source/WebCore/bridge/qt/qt_instance.cpp:80 >> + entry.iterator->second = new WeakMapImpl(group); > > It seems to me that we still need the adoptRef() here. > > Optionally, we could have a WeakMapImpl::create that calls adoptRef(new ...) and returns a PassRefPtr. Argh, good catch! I'll land a quick follow-up fix right away with adoptRef.
Hotfix landed in http://trac.webkit.org/changeset/125887