Bug 166407 – NULL-deref CRASH in WebCore::PlatformMediaSession::mediaType

• Component: New Bugs
• Status: NEW

[Jer Noble]

NULL-deref CRASH in WebCore::PlatformMediaSession::mediaType

[Jer Noble]

Created attachment 297646 [details]
Patch

[Darin Adler]

Comment on attachment 297646 [details]
Patch

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

Looks great.

Not sure when we use std::function, when we use WTF::Function, and when we use const& or && rather than passing by value. I’d like to clarify that in our style guide.

> Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:420
> +PlatformMediaSession* PlatformMediaSessionManager::findSession(std::function<bool(PlatformMediaSession&, size_t)> func) const

I would prefer the word "function" or "predicate" for the name of the argument, since we try to steer away from abbreviations in WebKit.

> Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:447
> +bool PlatformMediaSessionManager::anyOfSessions(std::function<bool(PlatformMediaSession&, size_t)> func) const
> +{
> +    return findSession(func);
>  }

Seems like this should be inlined in the header.

[Jer Noble]

(In reply to comment #2)
> Comment on attachment 297646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297646&action=review
> 
> Looks great.
> 
> Not sure when we use std::function, when we use WTF::Function, and when we
> use const& or && rather than passing by value. I’d like to clarify that in
> our style guide.

I didn't know WTF::Function was a thing!

> > Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:420
> > +PlatformMediaSession* PlatformMediaSessionManager::findSession(std::function<bool(PlatformMediaSession&, size_t)> func) const
> 
> I would prefer the word "function" or "predicate" for the name of the
> argument, since we try to steer away from abbreviations in WebKit.

Sure thing.

> > Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:447
> > +bool PlatformMediaSessionManager::anyOfSessions(std::function<bool(PlatformMediaSession&, size_t)> func) const
> > +{
> > +    return findSession(func);
> >  }
> 
> Seems like this should be inlined in the header.

Ok.

[Jer Noble]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 297646 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=297646&action=review
> > 
> > Looks great.
> > 
> > Not sure when we use std::function, when we use WTF::Function, and when we
> > use const& or && rather than passing by value. I’d like to clarify that in
> > our style guide.
> 
> I didn't know WTF::Function was a thing!

I went with WTF::Function and passing by const& (since ownership doesn't need to change).

[Jer Noble]

Created attachment 297659 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 297659 [details]
Patch for landing

Clearing flags on attachment: 297659

Committed r210104: <http://trac.webkit.org/changeset/210104>

[Jer Noble]

<rdar://problem/29139619>