Bug 167816 – [WebRTC] LibWebRTCEndpoint should not own objects that should be destroyed on the main thread

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

LibWebRTCEndpoint can be destroyed on the network thread.

[youenn fablet]

Created attachment 300585 [details]
Patch

[youenn fablet]

Created attachment 300763 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

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
> [phone]  Youenn Fablet  <[e-mail]>

two ChangeLog entries.

[youenn fablet]

> 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
> > [phone]  Youenn Fablet  <[e-mail]>
> 
> two ChangeLog entries.

OK, I'll fix it.

[Alex Christensen]

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.

[youenn fablet]

Created attachment 300830 [details]
Patch for landing

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[youenn fablet]

Created attachment 300845 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 300845 [details]
Patch for landing

Clearing flags on attachment: 300845

Committed r211837: <http://trac.webkit.org/changeset/211837>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.