Bug 93872 – [Qt] Replace use of internal Weak smart pointer with JSWeakObjectMap

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Hausmann]

[Qt] Replace use of internal Weak smart pointer with JSWeakObjectMap

[Simon Hausmann]

Created attachment 158057 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Simon Hausmann]

Created attachment 158064 [details]
Patch

New version explaining the motivation :)

[Caio Marcelo de Oliveira Filho]

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?

[Kenneth Rohde Christiansen]

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

[Simon Hausmann]

(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.

[Simon Hausmann]

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.

[Simon Hausmann]

Created attachment 158073 [details]
Patch

[Caio Marcelo de Oliveira Filho]

LGTM.

[Simon Hausmann]

Committed r125444: <http://trac.webkit.org/changeset/125444>

[Simon Hausmann]

Reverted r125444 for reason:

Broke some tests

Committed r125452: <http://trac.webkit.org/changeset/125452>

[Simon Hausmann]

(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.

[Simon Hausmann]

Another way to reproduce the crash:

gdb --args /path/to/DRT `find LayoutTests/fast/js`

[Simon Hausmann]

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.

[Simon Hausmann]

Created attachment 158805 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Simon Hausmann]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

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)?

[Simon Hausmann]

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.

[Simon Hausmann]

Created attachment 159049 [details]
Patch

Updated patch for review, taking Caio's comments into account

[Simon Hausmann]

Committed r125873: <http://trac.webkit.org/changeset/125873>

[Caio Marcelo de Oliveira Filho]

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.

[Simon Hausmann]

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.

[Simon Hausmann]

Hotfix landed in http://trac.webkit.org/changeset/125887