Bug 189040

Created attachment 348265 [details] Patch

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

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

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

Created attachment 348298 [details] Patch

Created attachment 348307 [details] Patch

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

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

Created attachment 348327 [details] Patch

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?

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

Created attachment 348393 [details] Patch

Created attachment 348402 [details] Patch

Comment on attachment 348402 [details] Patch Clearing flags on attachment: 348402 Committed r235484: <https://trac.webkit.org/changeset/235484>

All reviewed patches have been landed. Closing bug.

<rdar://problem/43861361>

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.

(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

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.

Reopening to attach new patch.

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

Comment on attachment 348531 [details] Skipping test in iOS as well Clearing flags on attachment: 348531 Committed r235519: <https://trac.webkit.org/changeset/235519>

All reviewed patches have been landed. Closing bug.

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

Fixed in upstream WPT

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

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