Bug 167816 - [WebRTC] LibWebRTCEndpoint should not own objects that should be destroyed on the main thread
Summary: [WebRTC] LibWebRTCEndpoint should not own objects that should be destroyed on...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-03 14:56 PST by youenn fablet
Modified: 2017-02-07 13:58 PST (History)
2 users (show)

See Also:


Attachments
Patch (38.98 KB, patch)
2017-02-03 18:15 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (38.64 KB, patch)
2017-02-06 15:47 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (36.71 KB, patch)
2017-02-07 11:50 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-01 for mac-elcapitan (878.17 KB, application/zip)
2017-02-07 12:42 PST, WebKit Commit Bot
no flags Details
Archive of layout-test-results from webkit-cq-03 for mac-elcapitan (956.51 KB, application/zip)
2017-02-07 13:33 PST, WebKit Commit Bot
no flags Details
Patch for landing (35.92 KB, patch)
2017-02-07 13:37 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-02-03 14:56:15 PST
LibWebRTCEndpoint can be destroyed on the network thread.
Comment 1 youenn fablet 2017-02-03 18:15:24 PST
Created attachment 300585 [details]
Patch
Comment 2 youenn fablet 2017-02-06 15:47:06 PST
Created attachment 300763 [details]
Patch
Comment 3 WebKit Commit Bot 2017-02-06 15:48:20 PST
Attachment 300763 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:93:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:119:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:134:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/testing/MockLibWebRTCPeerConnection.h:48:  local_streams is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/testing/MockLibWebRTCPeerConnection.h:49:  remote_streams is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/testing/MockLibWebRTCPeerConnection.h:51:  local_description is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/testing/MockLibWebRTCPeerConnection.h:52:  remote_description is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/testing/MockLibWebRTCPeerConnection.h:55:  signaling_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/testing/MockLibWebRTCPeerConnection.h:56:  ice_connection_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/testing/MockLibWebRTCPeerConnection.h:57:  ice_gathering_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 10 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Alex Christensen 2017-02-06 16:07:00 PST
Comment on attachment 300763 [details]
Patch

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

What problem did you see that motivated this change?

> LayoutTests/ChangeLog:15
> +2017-02-03  Youenn Fablet  <youenn@apple.com>

two ChangeLog entries.
Comment 5 youenn fablet 2017-02-06 17:31:26 PST
> What problem did you see that motivated this change?

I haven't noticed any crash yet.
But libwebrtc network process may well keep the last reference to LibWebRTCMediaEndpoint, thus deleting objects that are supposed to be deleted in the main thread.

> 
> > LayoutTests/ChangeLog:15
> > +2017-02-03  Youenn Fablet  <youenn@apple.com>
> 
> two ChangeLog entries.

OK, I'll fix it.
Comment 6 Alex Christensen 2017-02-07 00:10:42 PST
Comment on attachment 300763 [details]
Patch

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

> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:93
> +    explicit MockLibWebRTCPeerConnectionReleasedInNetworkThreadWhileCreatingOffer(webrtc::PeerConnectionObserver& observer) : MockLibWebRTCPeerConnection(observer) { }

We should fix the style checker if we really want to allow one initializer on the same line.

> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:142
> +void MockLibWebRTCPeerConnectionReleasedInNetworkThreadWhileSettingDescription::SetLocalDescription(webrtc::SetSessionDescriptionObserver* observer, webrtc::SessionDescriptionInterface*)

This shouldn't have its own line.
Comment 7 youenn fablet 2017-02-07 11:50:25 PST
Created attachment 300830 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2017-02-07 12:42:36 PST
Comment on attachment 300830 [details]
Patch for landing

Rejecting attachment 300830 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: http://webkit-queues.webkit.org/results/3021081
Comment 9 WebKit Commit Bot 2017-02-07 12:42:38 PST
Created attachment 300835 [details]
Archive of layout-test-results from webkit-cq-01 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 WebKit Commit Bot 2017-02-07 13:33:42 PST
Comment on attachment 300830 [details]
Patch for landing

Rejecting attachment 300830 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: http://webkit-queues.webkit.org/results/3021306
Comment 11 WebKit Commit Bot 2017-02-07 13:33:44 PST
Created attachment 300844 [details]
Archive of layout-test-results from webkit-cq-03 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 youenn fablet 2017-02-07 13:37:59 PST
Created attachment 300845 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2017-02-07 13:58:14 PST
Comment on attachment 300845 [details]
Patch for landing

Clearing flags on attachment: 300845

Committed r211837: <http://trac.webkit.org/changeset/211837>
Comment 14 WebKit Commit Bot 2017-02-07 13:58:18 PST
All reviewed patches have been landed.  Closing bug.