Bug 189040

Summary: Remove WebRTC legacy API implementation
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, eric.carlson, ews-watchlist, raymes, realdawei, rniwa, ryanhaddad, tsavell, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/12715
https://bugs.webkit.org/show_bug.cgi?id=189188
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Skipping test in iOS as well none

Description youenn fablet 2018-08-27 21:35:29 PDT
Remove WebRTC legacy API implementation
Comment 1 youenn fablet 2018-08-27 22:58:14 PDT
Created attachment 348265 [details]
Patch
Comment 2 youenn fablet 2018-08-27 23:00:32 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/12715
Comment 3 EWS Watchlist 2018-08-28 01:35:04 PDT
Comment on attachment 348265 [details]
Patch

Attachment 348265 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9007542

New failing tests:
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-default-feature-policy.https.html
fast/events/constructors/media-stream-event-constructor.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaDevices-IDL-enumerateDevices.html
imported/w3c/web-platform-tests/mediacapture-streams/MediaDevices-IDL-all.html
Comment 4 EWS Watchlist 2018-08-28 01:35:06 PDT
Created attachment 348277 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 5 youenn fablet 2018-08-28 09:32:23 PDT
Created attachment 348298 [details]
Patch
Comment 6 youenn fablet 2018-08-28 10:32:08 PDT
Created attachment 348307 [details]
Patch
Comment 7 EWS Watchlist 2018-08-28 12:39:05 PDT
Comment on attachment 348307 [details]
Patch

Attachment 348307 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9012557

New failing tests:
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/setRemoteDescription.html
Comment 8 EWS Watchlist 2018-08-28 12:39:07 PDT
Created attachment 348322 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 9 youenn fablet 2018-08-28 13:12:50 PDT
Created attachment 348327 [details]
Patch
Comment 10 Darin Adler 2018-08-28 20:46:33 PDT
Comment on attachment 348327 [details]
Patch

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

I am not sure I understand the situation here. Why is it OK to remove the legacy API? Is this a web-visible change or not?

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:220
> -    HashMap<webrtc::MediaStreamInterface*, MediaStream*> m_streams;
> +    HashMap<webrtc::MediaStreamInterface*, RefPtr<MediaStream>> m_streams;

This seems like a snuck-in unrelated improvement. Can we do this separately?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:4056
> +		CDE667A41E4BBF1500E8154A /* cocoa/WebAudioBufferList.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CDE667A21E4BBF1500E8154A /* cocoa/WebAudioBufferList.cpp */; };
> +		CDE667A51E4BBF1500E8154A /* cocoa/WebAudioBufferList.h in Headers */ = {isa = PBXBuildFile; fileRef = CDE667A31E4BBF1500E8154A /* cocoa/WebAudioBufferList.h */; settings = {ATTRIBUTES = (Private, ); }; };

This looks wrong.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:24534
> +				CDE667A21E4BBF1500E8154A /* cocoa/WebAudioBufferList.cpp */,
> +				CDE667A31E4BBF1500E8154A /* cocoa/WebAudioBufferList.h */,

This change looks wrong.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:31331
> +				CDE667A41E4BBF1500E8154A /* cocoa/WebAudioBufferList.cpp in Sources */,

This change looks wrong.

> LayoutTests/TestExpectations:1181
> +# Uses legacy WebRTC API.
> +imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/setRemoteDescription.html [ Skip ]

Can we instead fix this test first, *before* removing the thing it relies on?

> LayoutTests/fast/mediastream/RTCPeerConnection-statsSelector-expected.txt:8
> -PASS getUserMedia({audio:true, video:true}, gotStream) did not throw exception.
> +FAIL getUserMedia({audio:true, video:true}).then(gotStream) should not throw exception. Threw exception TypeError: undefined is not an object (evaluating 'getUserMedia({audio:true, video:true}).then').

Why are these new failures OK? Can we get a newer version of the test that assumes the right thing?

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-streams/MediaDevices-IDL-all-expected.txt:11
> -PASS Navigator interface: operation getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) 
> +FAIL Navigator interface: operation getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) assert_own_property: interface prototype object missing non-static operation expected property "getUserMedia" missing

Why are these new failures OK? Can we get a newer version of the test that assumes the right thing?

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-streams/MediaDevices-IDL-all-expected.txt:15
> -PASS Navigator interface: navigator must inherit property "getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback)" with the proper type 
> -PASS Navigator interface: calling getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) on navigator with too few arguments must throw TypeError 
> +FAIL Navigator interface: navigator must inherit property "getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback)" with the proper type assert_inherits: property "getUserMedia" not found in prototype chain
> +FAIL Navigator interface: calling getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) on navigator with too few arguments must throw TypeError assert_inherits: property "getUserMedia" not found in prototype chain

Why are these new failures OK? Can we get a newer version of the test that assumes the right thing?

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-streams/MediaDevices-IDL-enumerateDevices-expected.txt:11
> -PASS Navigator interface: operation getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) 
> +FAIL Navigator interface: operation getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) assert_own_property: interface prototype object missing non-static operation expected property "getUserMedia" missing

Why are these new failures OK? Can we get a newer version of the test that assumes the right thing?
Comment 11 youenn fablet 2018-08-28 21:40:46 PDT
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348327&action=review
> 
> I am not sure I understand the situation here. Why is it OK to remove the
> legacy API? Is this a web-visible change or not?

This legacy API is now turned off by default in WebKit.
We are in the process of updating our WebRTC implementation to be more spec compliant.
Removing this legacy API will help us going forward more quickly.

> > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:220
> > -    HashMap<webrtc::MediaStreamInterface*, MediaStream*> m_streams;
> > +    HashMap<webrtc::MediaStreamInterface*, RefPtr<MediaStream>> m_streams;
> 
> This seems like a snuck-in unrelated improvement. Can we do this separately?

We are removing LibWebRTCPeerConnectionBackend::m_remoteStreams which was the vector owning the MediaStream and is no longer useful since we stop exposing getRemoteStreams().
We therefore need somebody to keep a ref to these streams.
I can split that change in a separate patch if that helps the review.

> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:4056
> > +		CDE667A41E4BBF1500E8154A /* cocoa/WebAudioBufferList.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CDE667A21E4BBF1500E8154A /* cocoa/WebAudioBufferList.cpp */; };
> > +		CDE667A51E4BBF1500E8154A /* cocoa/WebAudioBufferList.h in Headers */ = {isa = PBXBuildFile; fileRef = CDE667A31E4BBF1500E8154A /* cocoa/WebAudioBufferList.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> This looks wrong.

Right, I will fix this.

> > LayoutTests/TestExpectations:1181
> > +# Uses legacy WebRTC API.
> > +imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/setRemoteDescription.html [ Skip ]
> 
> Can we instead fix this test first, *before* removing the thing it relies on?

The purpose of the test is to test the onaddstream legacy event for which we are removing support.
We cannot change the test to use onaddtrack since we would need to upstream it to WPT and other browsers supporting that event might find this test to be still useful.

> > LayoutTests/fast/mediastream/RTCPeerConnection-statsSelector-expected.txt:8
> > -PASS getUserMedia({audio:true, video:true}, gotStream) did not throw exception.
> > +FAIL getUserMedia({audio:true, video:true}).then(gotStream) should not throw exception. Threw exception TypeError: undefined is not an object (evaluating 'getUserMedia({audio:true, video:true}).then').
> 
> Why are these new failures OK? Can we get a newer version of the test that
> assumes the right thing?

Right, I will change the test.

> > LayoutTests/imported/w3c/web-platform-tests/mediacapture-streams/MediaDevices-IDL-all-expected.txt:11
> > -PASS Navigator interface: operation getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) 
> > +FAIL Navigator interface: operation getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) assert_own_property: interface prototype object missing non-static operation expected property "getUserMedia" missing
> 
> Why are these new failures OK? Can we get a newer version of the test that
> assumes the right thing?

These tests check the legacy API (use callbacks instead of promises) that other browsers might still support.
The plan here is to keep that like this for now and try to split legacy tests in a new folder webrtc/legacy that WebKit will skip altogether.
This is best done in WPT upstream.

> > LayoutTests/imported/w3c/web-platform-tests/mediacapture-streams/MediaDevices-IDL-all-expected.txt:15
> > -PASS Navigator interface: navigator must inherit property "getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback)" with the proper type 
> > -PASS Navigator interface: calling getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) on navigator with too few arguments must throw TypeError 
> > +FAIL Navigator interface: navigator must inherit property "getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback)" with the proper type assert_inherits: property "getUserMedia" not found in prototype chain
> > +FAIL Navigator interface: calling getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) on navigator with too few arguments must throw TypeError assert_inherits: property "getUserMedia" not found in prototype chain
> 
> Why are these new failures OK? Can we get a newer version of the test that
> assumes the right thing?

Ditto.

> > LayoutTests/imported/w3c/web-platform-tests/mediacapture-streams/MediaDevices-IDL-enumerateDevices-expected.txt:11
> > -PASS Navigator interface: operation getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) 
> > +FAIL Navigator interface: operation getUserMedia(MediaStreamConstraints, NavigatorUserMediaSuccessCallback, NavigatorUserMediaErrorCallback) assert_own_property: interface prototype object missing non-static operation expected property "getUserMedia" missing
> 
> Why are these new failures OK? Can we get a newer version of the test that
> assumes the right thing?

Ditto except that this test checks navigator.getUserMedia while we only support navigator.mediaDevices.getUserMedia now.
Other browsers might still want this test.
Comment 12 youenn fablet 2018-08-28 23:21:41 PDT
Created attachment 348393 [details]
Patch
Comment 13 youenn fablet 2018-08-29 09:43:15 PDT
Created attachment 348402 [details]
Patch
Comment 14 WebKit Commit Bot 2018-08-29 15:42:35 PDT
Comment on attachment 348402 [details]
Patch

Clearing flags on attachment: 348402

Committed r235484: <https://trac.webkit.org/changeset/235484>
Comment 15 WebKit Commit Bot 2018-08-29 15:42:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-08-29 15:43:18 PDT
<rdar://problem/43861361>
Comment 17 Truitt Savell 2018-08-30 08:40:49 PDT
Looks like https://trac.webkit.org/changeset/235484/webkit

has caused fast/mediastream/RTCPeerConnection-overloaded-operations.html

to be a highly flakey failure on Mac WK2. 

History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fmediastream%2FRTCPeerConnection-overloaded-operations.html

Diff:
--- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-expected.txt
+++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-actual.txt
@@ -1,3 +1,7 @@
+CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Argument 1 ('options') to RTCPeerConnection.createOffer must be a dictionary
+CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Argument 1 ('options') to RTCPeerConnection.createOffer must be a dictionary
+CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Argument 1 ('options') to RTCPeerConnection.createAnswer must be a dictionary
+CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Argument 1 ('options') to RTCPeerConnection.createAnswer must be a dictionary
 Test which overloaded RTCPeerConnection function that gets invoked (by return value)
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


I reproduced this by running this test 50 times on r235483 which had no failures and then 50 times on r235484 which failed often.
Comment 18 youenn fablet 2018-08-30 08:51:59 PDT
(In reply to Truitt Savell from comment #17)
> Looks like https://trac.webkit.org/changeset/235484/webkit
> 
> has caused fast/mediastream/RTCPeerConnection-overloaded-operations.html
> 
> to be a highly flakey failure on Mac WK2. 
> 
> History:
> http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=fast%2Fmediastream%2FRTCPeerConnection-
> overloaded-operations.html
> 
> Diff:
> ---
> /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/fast/
> mediastream/RTCPeerConnection-overloaded-operations-expected.txt
> +++
> /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/fast/
> mediastream/RTCPeerConnection-overloaded-operations-actual.txt
> @@ -1,3 +1,7 @@
> +CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Argument 1
> ('options') to RTCPeerConnection.createOffer must be a dictionary
> +CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Argument 1
> ('options') to RTCPeerConnection.createOffer must be a dictionary
> +CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Argument 1
> ('options') to RTCPeerConnection.createAnswer must be a dictionary
> +CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Argument 1
> ('options') to RTCPeerConnection.createAnswer must be a dictionary
>  Test which overloaded RTCPeerConnection function that gets invoked (by
> return value)
>  
>  On success, you will see a series of "PASS" messages, followed by "TEST
> COMPLETE".
> 
> 
> I reproduced this by running this test 50 times on r235483 which had no
> failures and then 50 times on r235484 which failed often.

Filed bug 189155
Comment 19 Truitt Savell 2018-08-30 11:27:25 PDT
Additionally found that imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/setRemoteDescription.html 

now has a missing test result as of this revision. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fwebrtc%2Frtcpeerconnection%2FsetRemoteDescription.html

Most likely just needs to be skipped in the right place.
Comment 20 youenn fablet 2018-08-30 12:39:31 PDT
Reopening to attach new patch.
Comment 21 youenn fablet 2018-08-30 12:39:32 PDT
Created attachment 348531 [details]
Skipping test in iOS as well
Comment 22 WebKit Commit Bot 2018-08-30 13:01:41 PDT
Comment on attachment 348531 [details]
Skipping test in iOS as well

Clearing flags on attachment: 348531

Committed r235519: <https://trac.webkit.org/changeset/235519>
Comment 23 WebKit Commit Bot 2018-08-30 13:01:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Raymes Khoury 2018-09-23 16:48:44 PDT
@youenn This change broke Chrome's web platform tests: https://bugs.chromium.org/p/chromium/issues/detail?id=879022#c4

I'm not sure that it's valid to expect navigator.getUserMedia to return a promise. It believe that it takes a callback as per https://developer.mozilla.org/en-US/docs/Web/API/Navigator/getUserMedia

navigator.mediaDevices.getUserMedia returns a promise and perhaps you intended to change it to that?
Comment 25 youenn fablet 2018-09-26 14:41:33 PDT
Fixed in upstream WPT