Bug 157596 – Remove all uses of PassRefPtr in WTF

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Keith Rollin]

Remove all uses of PassRefPtr in WTF.

[Radar WebKit Bug Importer]

<rdar://problem/26234391>

[Keith Rollin]

Created attachment 279796 [details]
Patch

This removes most uses of PassRefPtr in WTF, but leaves some behind for compatibility with modules that have not yet been converted.

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/Functional.h:701:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/Functional.h:706:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 24 files


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

[Chris Dumez]

Comment on attachment 279796 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSString.cpp:255
>          if (RefPtr<StringImpl> newImpl = StringImpl::tryCreateUninitialized(m_length, buffer)) {

Could use auto

> Source/JavaScriptCore/runtime/JSString.cpp:269
>      if (RefPtr<StringImpl> newImpl = StringImpl::tryCreateUninitialized(m_length, buffer)) {

Could use auto

> Source/JavaScriptCore/runtime/JSString.h:96
> +        , m_value(adoptRef(*value.leakRef()))

How is this better? Why is JSString still taking a PassRefPtr in? If it is too much refactoring to update this constructor declaration then I think we should not make this change either.
Note that based on the call sites, it looks like this constructor should eventually take a Ref<StringImpl>&&

> Source/JavaScriptCore/runtime/StringConstructor.cpp:78
> +    Ref<StringImpl> impl = StringImpl::createUninitialized(length, buf);

should use auto

> Source/JavaScriptCore/runtime/StringPrototype.cpp:-327
> -        return jsString(exec, impl.release());

Could use auto for impl earlier in this function.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:345
> +    return jsString(exec, WTFMove(impl));

Could use auto for impl earlier in this function.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:-400
> -        return jsString(exec, impl.release());

Could use auto for impl earlier in this function.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:431
> +    return jsString(exec, WTFMove(impl));

Could use auto for impl earlier in this function.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:765
> +    return jsString(exec, WTFMove(impl));

Could use auto for impl earlier in this function.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:-778
> -    return jsString(&exec, impl.release());

Could use auto for impl earlier in this function.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1580
> +        RefPtr<StringImpl> impl = StringImpl::tryCreateUninitialized(bufferSize, buffer);

Should use auto

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1649
> +    RefPtr<StringImpl> impl = StringImpl::tryCreateUninitialized(bufferSize, buffer);

should use auto

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1895
> +    return jsString(exec, WTFMove(impl));

Could use auto for impl earlier in this function.

> Source/WTF/wtf/Functional.h:27
>  #define WTF_Functional_h

This whole header is unused and about to get removed by Anders so I don't think we should modify it in this patch.

> Source/WTF/wtf/ParallelJobsGeneric.h:69
> +        static RefPtr<ThreadPrivate> create()

Should return a Ref<>, not a RefPtr<>.

> Source/WTF/wtf/text/StringImpl.cpp:207
> +inline Ref<StringImpl> StringImpl::reallocateInternal(RefPtr<StringImpl>&& originalString, unsigned length, CharType*& data)

Looks like this should take a Ref<>&& in, not a RefPtr<>&&.

> Source/WTF/wtf/text/StringImpl.cpp:228
> +Ref<StringImpl> StringImpl::reallocate(RefPtr<StringImpl>&& originalString, unsigned length, LChar*& data)

Looks like this should take a Ref<>&& in, not a RefPtr<>&&.

> Source/WTF/wtf/text/StringImpl.cpp:235
> +Ref<StringImpl> StringImpl::reallocate(RefPtr<StringImpl>&& originalString, unsigned length, UChar*& data)

Looks like this should take a Ref<>&& in, not a RefPtr<>&&.

> Source/WTF/wtf/text/StringImpl.cpp:296
> +Ref<SymbolImpl> StringImpl::createSymbol(StringImpl* rep)

Looks like this should take a StringImpl& in as it cannot be null.

> Source/WTF/wtf/text/StringImpl.h:268
> +    StringImpl(const LChar* characters, unsigned length, StringImpl* base)

Should probably take a Ref<>&& in? Looks like base cannot be null, also, we do transfer ownership.

> Source/WTF/wtf/text/StringImpl.h:279
> +        substringBuffer() = &Ref<StringImpl>(*base).leakRef();

:S

> Source/WTF/wtf/text/StringImpl.h:285
> +    StringImpl(const UChar* characters, unsigned length, StringImpl* base)

ditto.

> Source/WTF/wtf/text/StringImpl.h:296
> +        substringBuffer() = &Ref<StringImpl>(*base).leakRef();

:S

> Source/WTF/wtf/text/StringImpl.h:303
> +    StringImpl(CreateSymbolTag, const LChar* characters, unsigned length, StringImpl* base)

ditto, I'll stop commenting on these in this file.

> Source/WTF/wtf/text/StringImpl.h:313
> +        substringBuffer() = &Ref<StringImpl>(*base).leakRef();

:S

> Tools/TestWebKitAPI/Tests/WTF/Functional.cpp:154
>  

This whole file is about to get dropped.

[Keith Rollin]

(In reply to comment #4)
> Comment on attachment 279796 [details]

All comments accepted unless otherwise noted below.

> > Source/JavaScriptCore/runtime/JSString.h:96
> > +        , m_value(adoptRef(*value.leakRef()))
> 
> How is this better? Why is JSString still taking a PassRefPtr in? If it is
> too much refactoring to update this constructor declaration then I think we
> should not make this change either.
> Note that based on the call sites, it looks like this constructor should
> eventually take a Ref<StringImpl>&&

This is better because it compiles. :-)  m_value is a String. The previous code was initializing it with a PassRefPtr. I removed that constructor from String, and so had to munge types in order to invoke a constructor that worked.

I think you're suggesting that JSString should be changed to no longer accept a PassRefPtr? If so, that's outside the scope of this change. Making such a change would propagate out through JSC and make this patch larger (if not outright huge).

If you're suggesting that String should continue to have a constructor that takes a PassRefPtr, I'm not sure I agree with that. It's not apparent to me that we need to keep that constructor for backward compatibility when we can instead make the call sites conform to the lack of the c'tor that takes the PassRefPtr. The result is a clean String interface.

> > Source/WTF/wtf/ParallelJobsGeneric.h:69
> > +        static RefPtr<ThreadPrivate> create()
> 
> Should return a Ref<>, not a RefPtr<>.

The result of this function is stored in containers of type Vector<RefPtr<>> (s_threadPool and m_threads), so I thought it a good idea to keep the types the same and reduce churn, as well as avoid changes to other code that references these containers expecting a RefPtr and not a Ref.

> > Source/WTF/wtf/text/StringImpl.cpp:207
> > +inline Ref<StringImpl> StringImpl::reallocateInternal(RefPtr<StringImpl>&& originalString, unsigned length, CharType*& data)
> 
> Looks like this should take a Ref<>&& in, not a RefPtr<>&&.
> 
> > Source/WTF/wtf/text/StringImpl.cpp:228
> > +Ref<StringImpl> StringImpl::reallocate(RefPtr<StringImpl>&& originalString, unsigned length, LChar*& data)
> 
> Looks like this should take a Ref<>&& in, not a RefPtr<>&&.
> 
> > Source/WTF/wtf/text/StringImpl.cpp:235
> > +Ref<StringImpl> StringImpl::reallocate(RefPtr<StringImpl>&& originalString, unsigned length, UChar*& data)
> 
> Looks like this should take a Ref<>&& in, not a RefPtr<>&&.

I'm not sure. For instance, StringBuilder calls reallocate() passing in the StringImpl in its m_buffer data member. m_buffer is a RefPtr, meaning that its StringImpl can be nullptr, which I don't believe is OK to put into a Ref<>.

> > Source/WTF/wtf/text/StringImpl.h:268
> > +    StringImpl(const LChar* characters, unsigned length, StringImpl* base)
> 
> Should probably take a Ref<>&& in? Looks like base cannot be null, also, we
> do transfer ownership.
> 
> > Source/WTF/wtf/text/StringImpl.h:279
> > +        substringBuffer() = &Ref<StringImpl>(*base).leakRef();
> 
> :S
> 
> > Source/WTF/wtf/text/StringImpl.h:285
> > +    StringImpl(const UChar* characters, unsigned length, StringImpl* base)
> 
> ditto.
> 
> > Source/WTF/wtf/text/StringImpl.h:296
> > +        substringBuffer() = &Ref<StringImpl>(*base).leakRef();
> 
> :S
> 
> > Source/WTF/wtf/text/StringImpl.h:303
> > +    StringImpl(CreateSymbolTag, const LChar* characters, unsigned length, StringImpl* base)
> 
> ditto, I'll stop commenting on these in this file.
> 
> > Source/WTF/wtf/text/StringImpl.h:313
> > +        substringBuffer() = &Ref<StringImpl>(*base).leakRef();
> 
> :S

While StringImpl assumes ownership, it's not apparent to me that ownership is transferred. That is, it seems to me that ownership can be shared. See, for example, createSubstringSharingImpl, where it looks like ownerRep can continue to be owned by something else. In that case, my understanding is that Ref<>&& is not correct here.

[Keith Rollin]

Created attachment 279843 [details]
Patch

Updated for Chris's comments.

[Chris Dumez]

Comment on attachment 279796 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSString.h:96
>>> +        , m_value(adoptRef(*value.leakRef()))
>> 
>> How is this better? Why is JSString still taking a PassRefPtr in? If it is too much refactoring to update this constructor declaration then I think we should not make this change either.
>> Note that based on the call sites, it looks like this constructor should eventually take a Ref<StringImpl>&&
> 
> This is better because it compiles. :-)  m_value is a String. The previous code was initializing it with a PassRefPtr. I removed that constructor from String, and so had to munge types in order to invoke a constructor that worked.
> 
> I think you're suggesting that JSString should be changed to no longer accept a PassRefPtr? If so, that's outside the scope of this change. Making such a change would propagate out through JSC and make this patch larger (if not outright huge).
> 
> If you're suggesting that String should continue to have a constructor that takes a PassRefPtr, I'm not sure I agree with that. It's not apparent to me that we need to keep that constructor for backward compatibility when we can instead make the call sites conform to the lack of the c'tor that takes the PassRefPtr. The result is a clean String interface.

Then I would rather we construct a RefPtr<> from the PassRefPtr<> and call the String(RefPtr<>&&) constructor. PassRefPtr does not guarantee this is never called with nullptr.

>>> Source/WTF/wtf/ParallelJobsGeneric.h:69
>>> +        static RefPtr<ThreadPrivate> create()
>> 
>> Should return a Ref<>, not a RefPtr<>.
> 
> The result of this function is stored in containers of type Vector<RefPtr<>> (s_threadPool and m_threads), so I thought it a good idea to keep the types the same and reduce churn, as well as avoid changes to other code that references these containers expecting a RefPtr and not a Ref.

I still think this should return a Ref<>. There is no churn when converting a Ref<>&& into a RefPtr<> and the conversion is implicit.

>>> Source/WTF/wtf/text/StringImpl.cpp:207
>>> +inline Ref<StringImpl> StringImpl::reallocateInternal(RefPtr<StringImpl>&& originalString, unsigned length, CharType*& data)
>> 
>> Looks like this should take a Ref<>&& in, not a RefPtr<>&&.
> 
> I'm not sure. For instance, StringBuilder calls reallocate() passing in the StringImpl in its m_buffer data member. m_buffer is a RefPtr, meaning that its StringImpl can be nullptr, which I don't believe is OK to put into a Ref<>.

The first thing this function does is dereference originalString. If it could be null, we would already crash.

>>> Source/WTF/wtf/text/StringImpl.h:268
>>> +    StringImpl(const LChar* characters, unsigned length, StringImpl* base)
>> 
>> Should probably take a Ref<>&& in? Looks like base cannot be null, also, we do transfer ownership.
> 
> While StringImpl assumes ownership, it's not apparent to me that ownership is transferred. That is, it seems to me that ownership can be shared. See, for example, createSubstringSharingImpl, where it looks like ownerRep can continue to be owned by something else. In that case, my understanding is that Ref<>&& is not correct here.

Ownership is transferred in the sense that the implementation is calling leakRef() on it and is therefore holding a reference to the StringImpl. If a call site needs to keep holding a Ref<>, then you can call copyRef().
Note that passing a raw pointer here is a bit sad because:
1. It can never be null (The implementation dereferences it without null check)
2. You anyway construct a Ref<> below, so your not avoiding any refcounting churn. The fact is that the constructor wants to increment the refcount.

[Chris Dumez]

Comment on attachment 279843 [details]
Patch

See comments on previous patch.

[Chris Dumez]

Comment on attachment 279843 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:529
> +    RefPtr<StringImpl> reference = StringImpl::createSymbol(*original.get());

Here and below: *original is sufficient. You don't need the .get().

[Chris Dumez]

Comment on attachment 279843 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.h:355
> +    static ALWAYS_INLINE Ref<StringImpl> createSubstringSharingImpl(StringImpl* rep, unsigned offset, unsigned length)

I apparently failed to mention it on the previous patch but this should not be a raw pointer as it cannot be null (the implementation dereferences it without null check). Should probably be a StringImpl&.

> Source/WTF/wtf/text/StringImpl.h:363
> +        auto ownerRep = ((rep->bufferOwnership() == BufferSubstring) ? rep->substringBuffer() : rep);

I believe we prefer auto* when it is a raw pointer.

[Chris Dumez]

I think moving the std::functions around does not help because I believe a copy is made anyway when initially constructing the std::function from the lambda, which causes the captured variables to be copied.

From http://en.cppreference.com/w/cpp/utility/functional/function/function:
template< class F > function( F f );
-> Initializes the target with a copy of f.

I think we need to use something else than an std::function for RunLoop / WorkQueue's dispatch() parameter to avoid problems.

[Chris Dumez]

(In reply to comment #11)
> I think moving the std::functions around does not help because I believe a
> copy is made anyway when initially constructing the std::function from the
> lambda, which causes the captured variables to be copied.
> 
> From http://en.cppreference.com/w/cpp/utility/functional/function/function:
> template< class F > function( F f );
> -> Initializes the target with a copy of f.
> 
> I think we need to use something else than an std::function for RunLoop /
> WorkQueue's dispatch() parameter to avoid problems.

Oops wrong bug.

[Keith Rollin]

Created attachment 279971 [details]
Patch

Implemented all suggested changes.

[Chris Dumez]

Comment on attachment 279971 [details]
Patch

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

> Source/WTF/wtf/text/StringBuilder.cpp:62
> +        m_string = StringImpl::createSubstringSharingImpl(*m_buffer.get(), 0, m_length);

The .get() here is redundant.

> Source/WTF/wtf/text/StringImpl.cpp:305
> +        return adoptRef(static_cast<SymbolImpl&>(*new (NotNull, stringImpl) StringImpl(CreateSymbol, rep.m_data8, rep.length(), Ref<StringImpl>(*ownerRep))));

To we really need the explicit Ref<StringImpl>() ? Doesn't *ownerRep just work?

> Source/WTF/wtf/text/StringImpl.cpp:306
> +    return adoptRef(static_cast<SymbolImpl&>(*new (NotNull, stringImpl) StringImpl(CreateSymbol, rep.m_data16, rep.length(), Ref<StringImpl>(*ownerRep))));

Ditto.

> Source/WTF/wtf/text/StringImpl.h:367
> +            return adoptRef(*new (NotNull, stringImpl) StringImpl(rep.m_data8 + offset, length, Ref<StringImpl>(*ownerRep)));

Explicit Ref<>() really needed?

> Source/WTF/wtf/text/StringImpl.h:368
> +        return adoptRef(*new (NotNull, stringImpl) StringImpl(rep.m_data16 + offset, length, Ref<StringImpl>(*ownerRep)));

Ditto.

> Source/WTF/wtf/text/StringImpl.h:413
> +    // Reallocate the StringImpl. The originalString must be only owned by the RefPtr,

Ref?

> Source/WTF/wtf/text/WTFString.cpp:335
> +    return String(StringImpl::createSubstringSharingImpl(*m_impl.get(), offset, length));

.get() is redundant.

> Source/WebCore/xml/XPathGrammar.y:180
> +        String nametest = adoptRef(*$1);

Why are this needed? Constructing a String from a RefPtr<StringImpl>&& should be fine, no?

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:157
> +static Ref<StringImpl> stringFromUTF8(const char* characters)

Please use auto at the call sites.

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:159
> +    return adoptRef(*String::fromUTF8(characters).releaseImpl());

Return String::fromUTF8(characters).releaseImpl().releaseNonNull() may be nicer

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:528
>      RefPtr<StringImpl> original = stringFromUTF8("original");

Should use auto

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:529
> +    RefPtr<StringImpl> reference = StringImpl::createSymbol(*original);

You will not need the star after using auto above.

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:538
>      RefPtr<StringImpl> empty = stringFromUTF8("");

Should use auto.

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:539
> +    RefPtr<StringImpl> emptyReference = StringImpl::createSymbol(*empty);

You will not need the star after using auto above.

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:552
>      RefPtr<StringImpl> original = stringFromUTF8("original");

Should use auto

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:553
> +    RefPtr<StringImpl> reference = StringImpl::createSymbol(*original);

You will not need the star after using auto above.

[Keith Rollin]

Created attachment 280454 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/RefPtr.h:107:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 32 files


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

[Chris Dumez]

Comment on attachment 280454 [details]
Patch

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

R=me with nits.

> Source/WTF/wtf/MetaAllocator.cpp:189
> +    auto handle = adoptRef(new MetaAllocatorHandle(this, start, sizeInBytes, ownerUID));

adoptRef(*new ...);

> Source/WTF/wtf/MetaAllocator.cpp:192
> +        m_tracker->notify(handle.get());

Handle.ptr()

> Source/WTF/wtf/MetaAllocator.cpp:194
> +    return handle;

Return WTFMove(handle);

> Source/WTF/wtf/PassRefPtr.h:145
> +        return PassRefPtr<T>(static_cast<T*>(p.get()));

This is a potential perf regression. This should keep calling leakRef() and adopting. Just keep the constructor that takes an AdoptTag and call it here.

> Source/WTF/wtf/RefPtr.h:68
> +    RefPtr<T> release() { RefPtr<T> tmp = adoptRef(m_ptr); m_ptr = nullptr; return tmp; }

Can you please add a FIXME comment to say that we should eventually drop this and update the call sites to use WTFMove()?

[Keith Rollin]

Created attachment 280751 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/RefPtr.h:108:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 32 files


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

[WebKit Commit Bot]

Comment on attachment 280751 [details]
Patch

Clearing flags on attachment: 280751

Committed r201782: <http://trac.webkit.org/changeset/201782>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.