Bug 188799 - WeakPtr breaks vtables when upcasting to base classes
Summary: WeakPtr breaks vtables when upcasting to base classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-21 10:46 PDT by Jer Noble
Modified: 2019-05-29 14:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.36 KB, patch)
2019-05-21 07:47 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (693.11 KB, application/zip)
2019-05-21 09:15 PDT, Build Bot
no flags Details
WIP patch to use static_cast instead of reinterpret_cast (6.51 KB, text/plain)
2019-05-21 14:07 PDT, Geoffrey Garen
no flags Details
Archive of layout-test-results from ews211 for win-future (13.73 MB, application/zip)
2019-05-22 01:17 PDT, Build Bot
no flags Details
Patch (66.99 KB, patch)
2019-05-28 12:44 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (68.02 KB, patch)
2019-05-28 15:20 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (68.24 KB, patch)
2019-05-28 15:24 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (68.65 KB, patch)
2019-05-28 15:53 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (69.15 KB, patch)
2019-05-28 17:22 PDT, Geoffrey Garen
youennf: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2018-08-21 10:46:09 PDT
WeakPtr implements upcasting by reinterpret_cast'ing it's WeakReference<Type> to the WeakReference<DerivedType>. This breaks vtables of derived objects.
Comment 1 Geoffrey Garen 2018-08-21 13:23:25 PDT
Do you mean it breaks cases where a derived object has a vtable and a base object lacks a vtable?
Comment 2 Jer Noble 2018-08-21 16:49:03 PDT
(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.
Comment 3 Jer Noble 2018-08-21 16:54:49 PDT
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.)
Comment 4 Jer Noble 2018-08-22 10:23:21 PDT
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.
Comment 5 Antti Koivisto 2018-08-22 10:56:47 PDT
Dangerous uses are probably rare in practice so option 1 seems reasonable.
Comment 6 Geoffrey Garen 2018-08-22 14:47:10 PDT
> 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.
Comment 7 Antti Koivisto 2018-08-23 02:38:15 PDT
> (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.
Comment 8 Geoffrey Garen 2018-08-23 09:58:12 PDT
(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().
Comment 9 Geoffrey Garen 2018-10-10 20:17:25 PDT
Something like this happened in https://bugs.webkit.org/show_bug.cgi?id=190449.
Comment 10 Radar WebKit Bug Importer 2019-05-20 11:02:07 PDT
<rdar://problem/50952417>
Comment 11 Geoffrey Garen 2019-05-20 16:19:03 PDT
Something like this happened in <rdar://problem/50871594>.
Comment 12 Jer Noble 2019-05-21 07:32:32 PDT
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.
Comment 13 Jer Noble 2019-05-21 07:47:36 PDT
Created attachment 370317 [details]
Patch
Comment 14 Build Bot 2019-05-21 09:15:04 PDT
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.
Comment 15 Build Bot 2019-05-21 09:15:06 PDT
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
Comment 16 Build Bot 2019-05-21 09:24:05 PDT
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
Comment 17 youenn fablet 2019-05-21 09:40:00 PDT
(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>.
Comment 18 Geoffrey Garen 2019-05-21 14:02:50 PDT
> 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.)
Comment 19 Geoffrey Garen 2019-05-21 14:07:02 PDT
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.
Comment 20 youenn fablet 2019-05-21 14:44:11 PDT
> 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...
Comment 21 Build Bot 2019-05-22 01:17:11 PDT
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
Comment 22 Build Bot 2019-05-22 01:17:14 PDT
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
Comment 23 Geoffrey Garen 2019-05-28 12:44:36 PDT
Created attachment 370776 [details]
Patch
Comment 24 Build Bot 2019-05-28 12:47:03 PDT
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.
Comment 25 Chris Dumez 2019-05-28 12:49:40 PDT
GTK is red.
Comment 26 Geoffrey Garen 2019-05-28 15:20:57 PDT
Created attachment 370790 [details]
Patch
Comment 27 Build Bot 2019-05-28 15:22:51 PDT
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.
Comment 28 Geoffrey Garen 2019-05-28 15:24:42 PDT
Created attachment 370792 [details]
Patch
Comment 29 Build Bot 2019-05-28 15:29:10 PDT
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.
Comment 30 Geoffrey Garen 2019-05-28 15:53:34 PDT
Created attachment 370794 [details]
Patch
Comment 31 Build Bot 2019-05-28 15:55:17 PDT
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.
Comment 32 Geoffrey Garen 2019-05-28 17:22:08 PDT
Created attachment 370809 [details]
Patch
Comment 33 Build Bot 2019-05-28 17:23:49 PDT
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.
Comment 34 youenn fablet 2019-05-28 20:55:40 PDT
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)
Comment 35 Geoffrey Garen 2019-05-29 10:45:20 PDT
> > 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.
Comment 36 Geoffrey Garen 2019-05-29 10:51:22 PDT
> > 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.
Comment 37 Geoffrey Garen 2019-05-29 11:22:02 PDT
Committed r245857: <https://trac.webkit.org/changeset/245857>
Comment 38 Ryan Haddad 2019-05-29 13:08:05 PDT
Reverted r245857 for reason:

Breaks internal builds.

Committed r245863: <https://trac.webkit.org/changeset/245863>
Comment 39 Geoffrey Garen 2019-05-29 14:15:56 PDT
Committed r245868: <https://trac.webkit.org/changeset/245868>