Bug 171529 - ASSERTION FAILED: wasRemoved in WebCore::RealtimeMediaSourceCenter::removeDevicesChangedObserver(DevicesChangedObserverToken)
Summary: ASSERTION FAILED: wasRemoved in WebCore::RealtimeMediaSourceCenter::removeDev...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-01 17:50 PDT by Ryan Haddad
Modified: 2017-05-15 13:59 PDT (History)
7 users (show)

See Also:


Attachments
Crashlog (123.68 KB, text/plain)
2017-05-02 13:43 PDT, Ryan Haddad
no flags Details
Crashlog (103.88 KB, text/plain)
2017-05-02 13:49 PDT, Ryan Haddad
no flags Details
Proposed patch. (4.08 KB, patch)
2017-05-12 15:11 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing. (5.96 KB, patch)
2017-05-15 09:11 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing. (5.97 KB, patch)
2017-05-15 10:00 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2017-05-01 17:50:16 PDT
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
Comment 1 Ryan Haddad 2017-05-01 17:57:31 PDT
This assert was added in https://trac.webkit.org/changeset/215929/webkit
Comment 2 Ryan Haddad 2017-05-02 13:43:01 PDT
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
Comment 3 Ryan Haddad 2017-05-02 13:43:11 PDT
Created attachment 308848 [details]
Crashlog
Comment 4 Radar WebKit Bug Importer 2017-05-02 13:43:45 PDT
<rdar://problem/31945791>
Comment 5 Eric Carlson 2017-05-02 13:47:37 PDT
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)
Comment 6 Ryan Haddad 2017-05-02 13:48:55 PDT
(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.
Comment 7 Ryan Haddad 2017-05-02 13:49:37 PDT
Created attachment 308849 [details]
Crashlog
Comment 8 Alexey Proskuryakov 2017-05-06 20:48:31 PDT
EWS hits these assertions a lot, and is noticeably less reliable because of them. A quick fix would be appreciated.
Comment 9 Eric Carlson 2017-05-12 15:11:04 PDT
Created attachment 309952 [details]
Proposed patch.
Comment 10 Jer Noble 2017-05-12 15:28:32 PDT
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.
Comment 11 Eric Carlson 2017-05-15 09:11:55 PDT
Created attachment 310140 [details]
Patch for landing.
Comment 12 Eric Carlson 2017-05-15 10:00:11 PDT
Created attachment 310143 [details]
Patch for landing.
Comment 13 WebKit Commit Bot 2017-05-15 11:26:11 PDT
Comment on attachment 310143 [details]
Patch for landing.

Clearing flags on attachment: 310143

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