Bug 159145 - REGRESSION(r202337) [WebRTC] Crash when loading html5test.com
Summary: REGRESSION(r202337) [WebRTC] Crash when loading html5test.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 143211 158832
  Show dependency treegraph
 
Reported: 2016-06-27 06:07 PDT by Carlos Alberto Lopez Perez
Modified: 2016-06-29 09:11 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (4.78 KB, patch)
2016-06-28 01:43 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff
Updated patch (4.78 KB, patch)
2016-06-28 22:29 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2016-06-27 06:07:33 PDT
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
Comment 1 Carlos Garcia Campos 2016-06-27 06:14:04 PDT
I don't see anything specific to GTK+ in the bt. This looks like a crash in WebRTC implementation.
Comment 2 Carlos Alberto Lopez Perez 2016-06-27 07:23:12 PDT
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.
Comment 3 Adam Bergkvist 2016-06-27 22:34:20 PDT
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.
Comment 4 Adam Bergkvist 2016-06-28 01:43:39 PDT
Created attachment 282224 [details]
Proposed patch
Comment 5 Alex Christensen 2016-06-28 11:18:36 PDT
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.
Comment 6 Adam Bergkvist 2016-06-28 14:36:53 PDT
(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.
Comment 7 Adam Bergkvist 2016-06-28 22:29:59 PDT
Created attachment 282321 [details]
Updated patch
Comment 8 Adam Bergkvist 2016-06-29 08:50:02 PDT
Comment on attachment 282321 [details]
Updated patch

Thanks Eric and Alex
Comment 9 WebKit Commit Bot 2016-06-29 09:11:06 PDT
Comment on attachment 282321 [details]
Updated patch

Clearing flags on attachment: 282321

Committed r202624: <http://trac.webkit.org/changeset/202624>
Comment 10 WebKit Commit Bot 2016-06-29 09:11:11 PDT
All reviewed patches have been landed.  Closing bug.