WeakPtr breaks vtables when upcasting to base classes

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Jer Noble]

WeakPtr implements upcasting by reinterpret_cast'ing it's WeakReference<Type> to the WeakReference<DerivedType>. This breaks vtables of derived objects.

[Geoffrey Garen]

Do you mean it breaks cases where a derived object has a vtable and a base object lacks a vtable?

[Jer Noble]

(In reply to Geoffrey Garen from comment #1)
> Do you mean it breaks cases where a derived object has a vtable and a base
> object lacks a vtable?

I didn't try that case. It occurs any time where reinterpret_cast<Base>(derived*) != static_cast<Base>(derived*); so any time you have a "inherit from abstract 'client' base class", you'll hit this.

[Jer Noble]

A quick first-pass fix for this would be to add an assert to weak_reference_upcast():

  ASSERT(static_cast<T*>(weakReference->get()) == reinterpret_cast<T*>(weakReference->get()));

Just having this assert in place in a debug builds yields a compilation error when trying to do a dangerous upcast. (I couldn't find a static_assert() that correctly generates a compile error.)

[Jer Noble]

Talked about this with Geoff, and came up with this basic sketch of the options:

Option 1) Make this kind of unsafe reinterpret_cast cause a compile-time error, and do nothing to WeakPtr to support this type of casting.

This would mean callers would have to create WeakPtrFactory<>s with the explicit template parameter of the destination type. When callers want to pass WeakPtr<>s of multiple types, they need multiple WeakPtrFactory<>s. There's a small (sizeof(Ref<>)) memory cost, and CanMakeWeakPtr<> & makeWeakPtr() won't work.

Option 2) Encode the casting math from A->B into WeakPtr<> itself.

One way to implement this would be, when you cast a WeakPtr<A> to WeakPtr<B> (assuming that A* -> B* is_convertible) in a way where reinterpret_cast(B)<ptr> != static_cast<B>(ptr), WeakPtr would store the ptrdiff_t value of static_cast<B>(ptr) - reinterpret_cast(B)<ptr>, and its .get() method would subtract the diff from reinterpret_cast(B)<ptr>. This would not require the caller to know the type A. This would increase the size of WeakPtr<> by sizeof(ptrdiff_t).

Another way to implement this would be to store 'ptr' directly in the WeakPtr<>, and use WeakReference<> only to say whether WeakPtr<> should return ptr or nullptr. This would increase the size of WeakPtr<> by sizeof(ptr) (which will likely be the same as sizeof(ptrdiff_t)), but would not require pointer arithmetic at dereference time.

Since the runtime cost of having multiple WeakPtrFatory<>s is so low, and the runtime cost of WeakPtr<>--multiplied across every WeakPtr<>--is relatively high, it seems like the best course of action at this time is to just make dangerous WeakPtr<> casts a compile-time error.

[Antti Koivisto]

Dangerous uses are probably rare in practice so option 1 seems reasonable.

[Geoffrey Garen]

> It occurs any time where
> reinterpret_cast<Base>(derived*) != static_cast<Base>(derived*); so any time
> you have a "inherit from abstract 'client' base class", you'll hit this.

I think the cases that trigger this pointer fixup issue are:

(a) Base class lacks vtable; subclass has vtable;

(b) Subclass inherits from multiple base classes.

A subclass that inherits from exactly one abstract base class is pointer equal to its abstract base class -- all of its extra data members just get appended to the end.

[Antti Koivisto]

> (b) Subclass inherits from multiple base classes.

Even in this case no fixup is needed for the first base class, right? That is, multiple inheritance is fine as long as the first base class provides the weak factory.

[Geoffrey Garen]

(In reply to Antti Koivisto from comment #7)
> > (b) Subclass inherits from multiple base classes.
> 
> Even in this case no fixup is needed for the first base class, right?

Right.

> That
> is, multiple inheritance is fine as long as the first base class provides
> the weak factory.

Partially right.

Multiple inheritance is fine as long as you're only going to cast to the first base class. (In that case, either the first base class or the subclass can provide the weak pointer factory.)

But if you need to be able to create a weak pointer to more than one base class, then you're stuck. If you provide a weak pointer factory in the subclass, then you need pointer fixup when you cast to the non-first base class. If you provide a weak pointer factory in the base classes, then you have multiple implementations of weakPtrFactory().

[Geoffrey Garen]

Something like this happened in https://bugs.webkit.org/show_bug.cgi?id=190449.

[Radar WebKit Bug Importer]

<rdar://problem/50952417>

[Geoffrey Garen]

Something like this happened in <rdar://problem/50871594>.

[Jer Noble]

Adding the ASSERT mentioned in Comment 3 definitely causes a compile-time error when casting between WeakPtr<MediaStreamTrack> and MediaProducer. I'll add it, and clean up all the compiler errors, and hopefully more situations like this can be avoided in the future.

[Jer Noble]

Created attachment 370317 [details]
Patch

[EWS Watchlist]

Comment on attachment 370317 [details]
Patch

Attachment 370317 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12246672

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 370321 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 370317 [details]
Patch

Attachment 370317 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12246694

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
apiTests

[youenn fablet]

(In reply to Jer Noble from comment #12)
> Adding the ASSERT mentioned in Comment 3 definitely causes a compile-time
> error when casting between WeakPtr<MediaStreamTrack> and MediaProducer. I'll
> add it, and clean up all the compiler errors, and hopefully more situations
> like this can be avoided in the future.

Sounds good as a first step.
I still wonder whether we could have WeakPtr<T> owning a WeakReference<U>, U being the base type having the weak pointer factory.
We would then no longer need reinterpret_cast, only static_cast at WeakPtr<T>.
That might have the downside of requiring more includes, as it might be needed to have a definition of T wherever we are using WeakPtr<T>.

[Geoffrey Garen]

> I still wonder whether we could have WeakPtr<T> owning a WeakReference<U>, U
> being the base type having the weak pointer factory.
> We would then no longer need reinterpret_cast, only static_cast at
> WeakPtr<T>.
> That might have the downside of requiring more includes, as it might be
> needed to have a definition of T wherever we are using WeakPtr<T>.

I think this is possible, so I wrote a little implementation of it. But it's incompatible with how our media code currently works. Our media code depends on being able to make a WeakPtr to a base type that doesn't support WeakPtr, based on the programmer knowledge that a subtype does support WeakPtr, and a reinterpret_cast will succeed. (For example, CDMInstanceSessionFairPlayStreamingAVFObjC has a WeakPtr<CDMInstanceSessionClient> m_client, even though CDMInstanceSessionClient does not support WeakPtr.)

[Geoffrey Garen]

Created attachment 370340 [details]
WIP patch to use static_cast instead of reinterpret_cast

Here's the WIP patch.

It breaks our media code.

I think we could avoid the header include problem by using SFINAE somehow. Not sure.

[youenn fablet]

> I think this is possible, so I wrote a little implementation of it. But it's
> incompatible with how our media code currently works. Our media code depends
> on being able to make a WeakPtr to a base type that doesn't support WeakPtr,
> based on the programmer knowledge that a subtype does support WeakPtr, and a
> reinterpret_cast will succeed. (For example,
> CDMInstanceSessionFairPlayStreamingAVFObjC has a
> WeakPtr<CDMInstanceSessionClient> m_client, even though
> CDMInstanceSessionClient does not support WeakPtr.)

We could try to update CDMInstanceSessionClient to be a CanMakeWeakPtr< CDMInstanceSessionClient>.
I am not sure we are loosing much in requesting CanMakeWeakPtr for WeakPtr.

> I think we could avoid the header include problem by using SFINAE somehow.
> Not sure.

If we provide a default through SFINAE, the default might be picked or not based on whether the definition is available or not. This might trigger some weird behavior.
Not simple...

[EWS Watchlist]

Comment on attachment 370317 [details]
Patch

Attachment 370317 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12254553

New failing tests:
imported/blink/fast/canvas/canvas-clip-stack-persistence.html
svg/text/textpath-reference-update.html
webanimations/accelerated-animation-with-delay-and-seek.html
http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html
storage/indexeddb/modern/gc-closes-database.html
storage/indexeddb/modern/get-keyrange.html

[EWS Watchlist]

Created attachment 370386 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Geoffrey Garen]

Created attachment 370776 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/WeakPtr.h:97:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 47 files


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

[Chris Dumez]

GTK is red.

[Geoffrey Garen]

Created attachment 370790 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/WeakPtr.h:97:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 49 files


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

[Geoffrey Garen]

Created attachment 370792 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/WeakPtr.h:97:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 49 files


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

[Geoffrey Garen]

Created attachment 370794 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/WeakPtr.h:97:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 49 files


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

[Geoffrey Garen]

Created attachment 370809 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/WeakPtr.h:97:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 50 files


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

[youenn fablet]

Comment on attachment 370809 [details]
Patch

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

> Source/WTF/wtf/WeakPtr.h:97
> +    WeakPtr(Ref<WeakReference>&& ref) : m_ref(std::forward<Ref<WeakReference>>(ref)) { }

Do we need std::forward here?
We could mark the constructor as explicit.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:411
> +    auto weakThis = makeWeakPtr(*this);

Can we remove this line?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:862
> +    auto weakThis = makeWeakPtr(*this);

Can we do weakThis = makeWeakPtr(*this) in the lambda below.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:923
> +            WeakPtr<SourceBufferPrivateAVFObjC> weakThis = makeWeakPtr(*this);

auto, but probably can be created at lambda creation time below.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1310
>      m_decompressionSession->requestMediaDataWhenReady([weakThis] {

weakThis = makeWeakPtr(*this)

[Geoffrey Garen]

> > Source/WTF/wtf/WeakPtr.h:97
> > +    WeakPtr(Ref<WeakReference>&& ref) : m_ref(std::forward<Ref<WeakReference>>(ref)) { }
> 
> Do we need std::forward here?

I think std::forward is there to maintain the rvalue reference, and avoid refcount churn when initializing m_ref.

> We could mark the constructor as explicit.

Sounds good.

Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:411
> > +    auto weakThis = makeWeakPtr(*this);
> 
> Can we remove this line?

Weird. Yes we can. :P

Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:862
> > +    auto weakThis = makeWeakPtr(*this);
> 
> Can we do weakThis = makeWeakPtr(*this) in the lambda below.

Will do.

Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:923
> > +            WeakPtr<SourceBufferPrivateAVFObjC> weakThis = makeWeakPtr(*this);
> 
> auto, but probably can be created at lambda creation time below.

Will do.

Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:1310
> >      m_decompressionSession->requestMediaDataWhenReady([weakThis] {
> 
> weakThis = makeWeakPtr(*this)

Will do.

[Geoffrey Garen]

> > Do we need std::forward here?
> 
> I think std::forward is there to maintain the rvalue reference, and avoid
> refcount churn when initializing m_ref.

Maybe your point was that we can use std::move instead, and then drop the explicit type argument? I'll do that.

[Geoffrey Garen]

Committed r245857: <https://trac.webkit.org/changeset/245857>

[Ryan Haddad]

Reverted r245857 for reason:

Breaks internal builds.

Committed r245863: <https://trac.webkit.org/changeset/245863>

[Geoffrey Garen]

Committed r245868: <https://trac.webkit.org/changeset/245868>