Bug 156648

Summary: REGRESSION( r199603): Pandora crashes WebKit in WebPlaybackSessionManager::removeClientForContext
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
bdakin: review+
Follow up patch darin: review+

Description Jer Noble 2016-04-15 16:57:41 PDT 
REGRESSION( r199603): Pandora crashes WebKit in WebPlaybackSessionManager::removeClientForContext
Comment 1 Jer Noble 2016-04-15 16:58:14 PDT 
rdar://problem/25758117
Comment 2 Jer Noble 2016-04-15 16:59:30 PDT 
Created attachment 276526 [details]
Patch
Comment 3 Jer Noble 2016-04-15 17:01:52 PDT 
Committed r199615: <http://trac.webkit.org/changeset/199615>
Comment 4 Darin Adler 2016-04-15 22:41:14 PDT 
Comment on attachment 276526 [details]
Patch

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

> Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:262
> +    if (!m_mediaElements.contains(&mediaElement))
>          return;
>  
>      uint64_t contextId = m_mediaElements.get(&mediaElement);

This is an inefficient coding pattern. It’s a double hash table lookup. Preferred style is to take advantage of the empty value, and so know that a contextId of 0 means that mediaElement is not in m_mediaElement, or to use find and thus be able to do both the contains (iterator != m_mediaElement.end()) and get (iterator->value) operations and share that single hash table lookup.
Comment 5 Jer Noble 2016-04-18 08:33:06 PDT 
Reopening to attach new patch.
Comment 6 Jer Noble 2016-04-18 08:33:14 PDT 
Created attachment 276641 [details]
Follow up patch
Comment 7 Darin Adler 2016-04-18 08:52:40 PDT 
Comment on attachment 276641 [details]
Follow up patch

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

> Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:236
> +    auto foundIter = m_mediaElements.find(&mediaElement);

Normally best not to abbreviate "iterator" as "iter". I would probably have named this just "iterator", but "foundIterator" is OK.
Comment 8 Jer Noble 2016-04-18 09:04:02 PDT 
Committed r199671: <http://trac.webkit.org/changeset/199671>