Bug 189040 – Remove WebRTC legacy API implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Remove WebRTC legacy API implementation

[youenn fablet]

Created attachment 348265 [details]
Patch

[youenn fablet]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/12715

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Created attachment 348298 [details]
Patch

[youenn fablet]

Created attachment 348307 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Created attachment 348327 [details]
Patch

[Darin Adler]

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?

[youenn fablet]

> 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.

[youenn fablet]

Created attachment 348393 [details]
Patch

[youenn fablet]

Created attachment 348402 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 348402 [details]
Patch

Clearing flags on attachment: 348402

Committed r235484: <https://trac.webkit.org/changeset/235484>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43861361>

[Truitt Savell]

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.

[youenn fablet]

(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

[Truitt Savell]

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.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 348531 [details]
Skipping test in iOS as well

[WebKit Commit Bot]

Comment on attachment 348531 [details]
Skipping test in iOS as well

Clearing flags on attachment: 348531

Committed r235519: <https://trac.webkit.org/changeset/235519>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Raymes Khoury]

@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?

[youenn fablet]

Fixed in upstream WPT

[Sam Sneddon [:gsnedders]]

*** Bug 164217 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

> 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?

Looks like it was not OK, see bug 270453.