REGRESSION(r202337) [WebRTC] Crash when loading html5test.com

[Carlos Alberto Lopez Perez]

With current trunk (r202484) when I try to load html5test.com with the minibrowser:

$ Tools/Scripts/run-minibrowser --gtk http://html5test.com

I get a crash.


The backtrace: http://sprunge.us/REVR

[Carlos Garcia Campos]

I don't see anything specific to GTK+ in the bt. This looks like a crash in WebRTC implementation.

[Carlos Alberto Lopez Perez]

I have identified r202337 <http://trac.webkit.org/r202337> as the one causing this regression.

I manually reverted r202337 locally, rebuilt webkit and the crash is gone.

[Adam Bergkvist]

I see the problem. For testing, we create a MockMediaEndpoint (WebRTC backend), but when the mini-browser is run, we have a nullptr MediaEndpoint. The changes introduced in r202337 doesn't handle that properly.

[Adam Bergkvist]

Created attachment 282224 [details]
Proposed patch

[Alex Christensen]

Comment on attachment 282224 [details]
Proposed patch

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

> Source/WebCore/platform/mediastream/MediaEndpoint.cpp:56
> +class NullMediaEndpoint : public MediaEndpoint {

I think EmptyMediaEndpoint would be a better name.

> Source/WebCore/platform/mediastream/MediaEndpoint.cpp:80
> +    return std::unique_ptr<MediaEndpoint>(new NullMediaEndpoint(client));

make_unique.  Also, this function is not necessary.  Just use std::make_unique instead of this.

[Adam Bergkvist]

(In reply to comment #5)

Thanks for reviewing, Alex

> Comment on attachment 282224 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282224&action=review
> 
> > Source/WebCore/platform/mediastream/MediaEndpoint.cpp:56
> > +class NullMediaEndpoint : public MediaEndpoint {
> 
> I think EmptyMediaEndpoint would be a better name.

EmptyMediaEndpoint and EmptyRealtimeMediaSource works for me.

> > Source/WebCore/platform/mediastream/MediaEndpoint.cpp:80
> > +    return std::unique_ptr<MediaEndpoint>(new NullMediaEndpoint(client));
> 
> make_unique.  Also, this function is not necessary.  Just use
> std::make_unique instead of this.

Are you referring to the createMediaEndpoint() function here? The MediaEndpointPeerConnection constructor calls createMediaEndpoint() to create its MediaEndpoint - in this case an "empty" one. When a port has a "real" MediaEndponit implementation it provides its own createMediaEndpoint() that's called instead.

[Adam Bergkvist]

Created attachment 282321 [details]
Updated patch

[Adam Bergkvist]

Comment on attachment 282321 [details]
Updated patch

Thanks Eric and Alex

[WebKit Commit Bot]

Comment on attachment 282321 [details]
Updated patch

Clearing flags on attachment: 282321

Committed r202624: <http://trac.webkit.org/changeset/202624>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.