Bug 171529 – ASSERTION FAILED: wasRemoved in WebCore::RealtimeMediaSourceCenter::removeDevicesChangedObserver(DevicesChangedObserverToken)

• Status: ASSIGNED
• Component: New Bugs

[Ryan Haddad]

This is a flaky assertion failure seen with LayoutTest fast/dom/Window/remove-timeout-crash.html

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r216041%20(909)/results.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fdom%2FWindow%2Fremove-timeout-crash.html

ASSERTION FAILED: wasRemoved
/Volumes/Data/slave/sierra-debug/build/Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp(171) : void WebCore::RealtimeMediaSourceCenter::removeDevicesChangedObserver(DevicesChangedObserverToken)
1   0x10494fa0d WTFCrash
2   0x114e1c2d6 WebCore::RealtimeMediaSourceCenter::removeDevicesChangedObserver(unsigned int)
3   0x114a79861 WebCore::MediaDevices::~MediaDevices()
4   0x114a798e5 WebCore::MediaDevices::~MediaDevices()
5   0x114a79929 WebCore::MediaDevices::~MediaDevices()
6   0x11435b4bf WTF::RefCounted<WebCore::MediaDevices>::deref() const
7   0x114c72ff5 void WTF::derefIfNotNull<WebCore::MediaDevices>(WebCore::MediaDevices*)
8   0x114c72fb3 WTF::RefPtr<WebCore::MediaDevices>::~RefPtr()
9   0x114c72695 WTF::RefPtr<WebCore::MediaDevices>::~RefPtr()
10  0x114c7265c WebCore::NavigatorMediaDevices::~NavigatorMediaDevices()
11  0x114c726b5 WebCore::NavigatorMediaDevices::~NavigatorMediaDevices()
12  0x114c726f9 WebCore::NavigatorMediaDevices::~NavigatorMediaDevices()
13  0x114c6ccb3 WTF::KeyValuePair<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > >::~KeyValuePair()
14  0x114c6cbe5 WTF::KeyValuePair<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > >::~KeyValuePair()
15  0x114c6cb74 WTF::HashTable<char const*, WTF::KeyValuePair<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > > >, WTF::PtrHash<char const*>, WTF::HashMap<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > >, WTF::PtrHash<char const*>, WTF::HashTraits<char const*>, WTF::HashTraits<std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > > >::KeyValuePairTraits, WTF::HashTraits<char const*> >::deallocateTable(WTF::KeyValuePair<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > >*, unsigned int)
16  0x114c6c969 WTF::HashTable<char const*, WTF::KeyValuePair<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > > >, WTF::PtrHash<char const*>, WTF::HashMap<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > >, WTF::PtrHash<char const*>, WTF::HashTraits<char const*>, WTF::HashTraits<std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > > >::KeyValuePairTraits, WTF::HashTraits<char const*> >::~HashTable()
17  0x114c6c925 WTF::HashTable<char const*, WTF::KeyValuePair<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > > >, WTF::PtrHash<char const*>, WTF::HashMap<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > >, WTF::PtrHash<char const*>, WTF::HashTraits<char const*>, WTF::HashTraits<std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > > >::KeyValuePairTraits, WTF::HashTraits<char const*> >::~HashTable()
18  0x114c6c905 WTF::HashMap<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > >, WTF::PtrHash<char const*>, WTF::HashTraits<char const*>, WTF::HashTraits<std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > > >::~HashMap()
19  0x114c6c8e5 WTF::HashMap<char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > >, WTF::PtrHash<char const*>, WTF::HashTraits<char const*>, WTF::HashTraits<std::__1::unique_ptr<WebCore::Supplement<WebCore::Navigator>, std::__1::default_delete<WebCore::Supplement<WebCore::Navigator> > > > >::~HashMap()
20  0x114c6c0d5 WebCore::Supplementable<WebCore::Navigator>::~Supplementable()
21  0x114c6c04c WebCore::Navigator::~Navigator()
22  0x114c6c0f5 WebCore::Navigator::~Navigator()
23  0x114c6c139 WebCore::Navigator::~Navigator()
24  0x113495ccf WTF::RefCounted<WebCore::NavigatorBase>::deref() const
25  0x113495c71 WTF::Ref<WebCore::Navigator>::~Ref()
26  0x113486e55 WTF::Ref<WebCore::Navigator>::~Ref()
27  0x1143cdd29 WebCore::JSDOMWrapper<WebCore::Navigator>::~JSDOMWrapper()
28  0x1143cdd05 WebCore::JSNavigator::~JSNavigator()
29  0x1143ca7d5 WebCore::JSNavigator::~JSNavigator()
30  0x1143ca42d WebCore::JSNavigator::destroy(JSC::JSCell*)
31  0x104307b5a JSC::(anonymous namespace)::DestroyFunc::operator()(JSC::VM&, JSC::JSCell*) const

[Ryan Haddad]

This assert was added in https://trac.webkit.org/changeset/215929/webkit

[Ryan Haddad]

Also saw this with fast/dom/Window/timeout-callback-scope.html

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r216079%20(926)/results.html

[Ryan Haddad]

Created attachment 308848 [details]
Crashlog

[Radar WebKit Bug Importer]

<rdar://problem/31945791>

[Eric Carlson]

The attached crashlog doesn't seem to be related:

Thread 24 Crashed:: WTF::AutomaticThread
0   com.apple.JavaScriptCore      	0x000000010c61cb8c JSC::MarkedBlock::vm() const + 12 (MarkedBlock.h:416)
1   com.apple.JavaScriptCore      	0x000000010c6a27d1 JSC::HeapCell::vm() const + 81 (HeapCellInlines.h:67)
2   com.apple.JavaScriptCore      	0x000000010c6a67af JSC::JSCell::structure() const + 31 (JSCellInlines.h:110)

[Ryan Haddad]

(In reply to Eric Carlson from comment #5)
> The attached crashlog doesn't seem to be related:
> 
> Thread 24 Crashed:: WTF::AutomaticThread
> 0   com.apple.JavaScriptCore      	0x000000010c61cb8c JSC::MarkedBlock::vm()
> const + 12 (MarkedBlock.h:416)
> 1   com.apple.JavaScriptCore      	0x000000010c6a27d1 JSC::HeapCell::vm()
> const + 81 (HeapCellInlines.h:67)
> 2   com.apple.JavaScriptCore      	0x000000010c6a67af
> JSC::JSCell::structure() const + 31 (JSCellInlines.h:110)

Attached the wrong one, sorry. Will fix.

[Ryan Haddad]

Created attachment 308849 [details]
Crashlog

[Alexey Proskuryakov]

EWS hits these assertions a lot, and is noticeably less reliable because of them. A quick fix would be appreciated.

[Eric Carlson]

Created attachment 309952 [details]
Proposed patch.

[Jer Noble]

Comment on attachment 309952 [details]
Proposed patch.

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

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:205
> -    auto callbacks = m_devicesChangedObservers;
> -    for (auto it : callbacks)
> +    // Copy the hash map because the observer callback may call back in and modify the map.
> +    auto callbacks = observerMap();
> +    for (auto& it : callbacks)

This solves a certain class of problems, but with this change, it's still possible for a callback to call a freshly deleted object. So please make sure that the classes registering for observers use a strong- or weak-pointer to protect against being called back after destruction time.

[Eric Carlson]

Created attachment 310140 [details]
Patch for landing.

[Eric Carlson]

Created attachment 310143 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 310143 [details]
Patch for landing.

Clearing flags on attachment: 310143

Committed r216866: <http://trac.webkit.org/changeset/216866>