WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
170146
Fix addIceCandidate after
r214441
https://bugs.webkit.org/show_bug.cgi?id=170146
Summary
Fix addIceCandidate after r214441
youenn fablet
Reported
2017-03-27 16:56:30 PDT
We should throw if no parameter is passed.
Attachments
Patch
(6.49 KB, patch)
2017-03-27 16:57 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
(1.04 MB, application/zip)
2017-03-27 18:24 PDT
,
Build Bot
no flags
Details
Patch
(5.64 KB, patch)
2017-03-27 18:39 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(5.75 KB, patch)
2017-03-28 12:59 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2017-03-27 16:57:27 PDT
Created
attachment 305526
[details]
Patch
Chris Dumez
Comment 2
2017-03-27 16:59:30 PDT
Comment on
attachment 305526
[details]
Patch r=me if the bots are happy.
youenn fablet
Comment 3
2017-03-27 17:02:37 PDT
(In reply to Chris Dumez from
comment #2
)
> Comment on
attachment 305526
[details]
> Patch > > r=me if the bots are happy.
Right, I updated the test expectation by hand...
Chris Dumez
Comment 4
2017-03-27 18:14:47 PDT
(In reply to youenn fablet from
comment #3
)
> (In reply to Chris Dumez from
comment #2
) > > Comment on
attachment 305526
[details]
> > Patch > > > > r=me if the bots are happy. > > Right, I updated the test expectation by hand...
lol
Build Bot
Comment 5
2017-03-27 18:24:06 PDT
Comment on
attachment 305526
[details]
Patch
Attachment 305526
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/3420597
New failing tests: imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl.html
Build Bot
Comment 6
2017-03-27 18:24:09 PDT
Created
attachment 305536
[details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Chris Dumez
Comment 7
2017-03-27 18:26:21 PDT
--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/retries/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt +++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/retries/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl-actual.txt @@ -23,7 +23,7 @@ PASS RTCPeerConnection interface: attribute remoteDescription PASS RTCPeerConnection interface: attribute currentRemoteDescription PASS RTCPeerConnection interface: attribute pendingRemoteDescription -PASS RTCPeerConnection interface: operation addIceCandidate([object Object],[object Object]) +PASS RTCPeerConnection interface: operation addIceCandidate([object Object],[object Object]) PASS RTCPeerConnection interface: attribute signalingState PASS RTCPeerConnection interface: attribute iceGatheringState PASS RTCPeerConnection interface: attribute iceConnectionState @@ -44,7 +44,9 @@ fn.apply(obj, args); }" did not throw FAIL RTCPeerConnection interface: operation setRemoteDescription(RTCSessionDescription,VoidFunction,RTCPeerConnectionErrorCallback) assert_equals: property has wrong .length expected 1 but got 0 -PASS RTCPeerConnection interface: operation addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) +FAIL RTCPeerConnection interface: operation addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) assert_throws: calling operation with this = null didn't throw TypeError function "function () { + fn.apply(obj, args); + }" did not throw FAIL RTCPeerConnection interface: operation getStats(MediaStreamTrack,RTCStatsCallback,RTCPeerConnectionErrorCallback) assert_throws: calling operation with this = null didn't throw TypeError function "function () { fn.apply(obj, args); }" did not throw @@ -79,7 +81,7 @@ PASS RTCPeerConnection interface: pc must inherit property "currentRemoteDescription" with the proper type (8) PASS RTCPeerConnection interface: pc must inherit property "pendingRemoteDescription" with the proper type (9) PASS RTCPeerConnection interface: pc must inherit property "addIceCandidate" with the proper type (10) -PASS RTCPeerConnection interface: calling addIceCandidate([object Object],[object Object]) on pc with too few arguments must throw TypeError +PASS RTCPeerConnection interface: calling addIceCandidate([object Object],[object Object]) on pc with too few arguments must throw TypeError FAIL RTCPeerConnection interface: pc must inherit property "signalingState" with the proper type (11) Unrecognized type RTCSignalingState FAIL RTCPeerConnection interface: pc must inherit property "iceGatheringState" with the proper type (12) Unrecognized type RTCIceGatheringState FAIL RTCPeerConnection interface: pc must inherit property "iceConnectionState" with the proper type (13) Unrecognized type RTCIceConnectionState @@ -106,7 +108,7 @@ fn.apply(obj, args); }" did not throw PASS RTCPeerConnection interface: pc must inherit property "addIceCandidate" with the proper type (27) -PASS RTCPeerConnection interface: calling addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) on pc with too few arguments must throw TypeError +FAIL RTCPeerConnection interface: calling addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) on pc with too few arguments must throw TypeError assert_throws: Called with 0 arguments function "function () { fn.apply(obj, args); }" did not throw PASS RTCPeerConnection interface: pc must inherit property "getStats" with the proper type (28)
youenn fablet
Comment 8
2017-03-27 18:39:15 PDT
Created
attachment 305538
[details]
Patch
youenn fablet
Comment 9
2017-03-27 18:39:34 PDT
(In reply to youenn fablet from
comment #8
)
> Created
attachment 305538
[details]
> Patch
This one should have a better expectation...
youenn fablet
Comment 10
2017-03-28 12:59:09 PDT
Created
attachment 305626
[details]
Patch for landing
youenn fablet
Comment 11
2017-03-28 12:59:36 PDT
(In reply to youenn fablet from
comment #10
)
> Created
attachment 305626
[details]
> Patch for landing
Retrying EWS since it was submitted at a bad time apparently.
Carlos Alberto Lopez Perez
Comment 12
2017-03-28 13:01:35 PDT
r214441
<
https://trac.webkit.org/r214441
> Also caused this failure on the WebKitGTK port (OpenWebRTC) on the test fast/mediastream/RTCPeerConnection-overloaded-operations-params.html --- /home/clopez/webkit/webkit/layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-params-expected.txt +++ /home/clopez/webkit/webkit/layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-params-actual.txt @@ -80,10 +80,10 @@ PASS promise pc.addIceCandidate(candidate) did not reject with TypeError. PASS promise pc.addIceCandidate(candidate, emptyFunc, emptyFunc) did not reject with TypeError. *** candidate is not optional -PASS promise pc.addIceCandidate() rejected with TypeError: Not enough arguments +FAIL promise pc.addIceCandidate() fulfilled unexpectedly. *** candidate is not nullable -PASS promise pc.addIceCandidate(null) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate -PASS promise pc.addIceCandidate(null, emptyFunc, emptyFunc) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate +FAIL promise pc.addIceCandidate(null) fulfilled unexpectedly. +FAIL promise pc.addIceCandidate(null, emptyFunc, emptyFunc) fulfilled unexpectedly. PASS promise pc.addIceCandidate(1) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate *** Bad input as candidate PASS promise pc.addIceCandidate('foo') rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate Note that the test fast/mediastream/RTCPeerConnection-overloaded-operations-params.html is Skipped on all ports but the WebKitGTK+ one.
Eric Carlson
Comment 13
2017-03-28 13:03:31 PDT
Comment on
attachment 305626
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=305626&action=review
> LayoutTests/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt:49 > +FAIL RTCPeerConnection interface: operation addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) assert_throws: calling operation with this = null didn't throw TypeError function "function () { > + fn.apply(obj, args); > + }" did not throw
Does the test need to be updated or is this correct?
Carlos Alberto Lopez Perez
Comment 14
2017-03-28 13:04:41 PDT
The patch that is about to land reduces the test diff to this: $ cat layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-params-diff.txt --- /home/clopez/webkit/webkit/layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-params-expected.txt +++ /home/clopez/webkit/webkit/layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-params-actual.txt @@ -82,8 +82,8 @@ *** candidate is not optional PASS promise pc.addIceCandidate() rejected with TypeError: Not enough arguments *** candidate is not nullable -PASS promise pc.addIceCandidate(null) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate -PASS promise pc.addIceCandidate(null, emptyFunc, emptyFunc) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate +FAIL promise pc.addIceCandidate(null) fulfilled unexpectedly. +FAIL promise pc.addIceCandidate(null, emptyFunc, emptyFunc) fulfilled unexpectedly. PASS promise pc.addIceCandidate(1) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate *** Bad input as candidate PASS promise pc.addIceCandidate('foo') rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate
Chris Dumez
Comment 15
2017-03-28 13:06:39 PDT
Comment on
attachment 305626
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=305626&action=review
>> LayoutTests/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt:49 >> + }" did not throw > > Does the test need to be updated or is this correct?
I think the test is wrong here. The spec says: Promise<void> addIceCandidate((RTCIceCandidateInit or RTCIceCandidate) candidate, VoidFunction successCallback, RTCPeerConnectionErrorCallback failureCallback); But the test's IDL returns a void, not a Promise<void>.
youenn fablet
Comment 16
2017-03-28 13:07:34 PDT
(In reply to Eric Carlson from
comment #13
)
> Comment on
attachment 305626
[details]
> Patch for landing > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=305626&action=review
> > > LayoutTests/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt:49 > > +FAIL RTCPeerConnection interface: operation addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) assert_throws: calling operation with this = null didn't throw TypeError function "function () { > > + fn.apply(obj, args); > > + }" did not throw > > Does the test need to be updated or is this correct?
As discussed with Chris, it needs to be updated since it is returning a Promise. In the end, I hope we will be able to remove support for these legacies
youenn fablet
Comment 17
2017-03-28 13:10:13 PDT
> -PASS promise pc.addIceCandidate(null) rejected with TypeError: Argument 1 > ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of > RTCIceCandidate > -PASS promise pc.addIceCandidate(null, emptyFunc, emptyFunc) rejected with > TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate > must be an instance of RTCIceCandidate > +FAIL promise pc.addIceCandidate(null) fulfilled unexpectedly. > +FAIL promise pc.addIceCandidate(null, emptyFunc, emptyFunc) fulfilled > unexpectedly.
We might want to check with other browsers, but I think it is ok to do addIceCandidate(null). It will mean that there is no more ice candidate, just as would be done with addIceCandidate({ })
Carlos Alberto Lopez Perez
Comment 18
2017-03-28 13:21:15 PDT
(In reply to youenn fablet from
comment #17
)
> > -PASS promise pc.addIceCandidate(null) rejected with TypeError: Argument 1 > > ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of > > RTCIceCandidate > > -PASS promise pc.addIceCandidate(null, emptyFunc, emptyFunc) rejected with > > TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate > > must be an instance of RTCIceCandidate > > +FAIL promise pc.addIceCandidate(null) fulfilled unexpectedly. > > +FAIL promise pc.addIceCandidate(null, emptyFunc, emptyFunc) fulfilled > > unexpectedly. > > We might want to check with other browsers, but I think it is ok to do > addIceCandidate(null). It will mean that there is no more ice candidate, > just as would be done with addIceCandidate({ })
That seems right:
https://github.com/w3c/webrtc-pc/issues/569
So I guess this test just needs to be rebaselined. I will do that.
WebKit Commit Bot
Comment 19
2017-03-28 13:40:22 PDT
Comment on
attachment 305626
[details]
Patch for landing Clearing flags on attachment: 305626 Committed
r214490
: <
http://trac.webkit.org/changeset/214490
>
WebKit Commit Bot
Comment 20
2017-03-28 13:40:26 PDT
All reviewed patches have been landed. Closing bug.
Carlos Alberto Lopez Perez
Comment 21
2017-03-28 20:02:15 PDT
(In reply to Carlos Alberto Lopez Perez from
comment #18
)
> (In reply to youenn fablet from
comment #17
) > > > -PASS promise pc.addIceCandidate(null) rejected with TypeError: Argument 1 > > > ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of > > > RTCIceCandidate > > > -PASS promise pc.addIceCandidate(null, emptyFunc, emptyFunc) rejected with > > > TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate > > > must be an instance of RTCIceCandidate > > > +FAIL promise pc.addIceCandidate(null) fulfilled unexpectedly. > > > +FAIL promise pc.addIceCandidate(null, emptyFunc, emptyFunc) fulfilled > > > unexpectedly. > > > > We might want to check with other browsers, but I think it is ok to do > > addIceCandidate(null). It will mean that there is no more ice candidate, > > just as would be done with addIceCandidate({ }) > > That seems right:
https://github.com/w3c/webrtc-pc/issues/569
> > So I guess this test just needs to be rebaselined. I will do that.
Updated on
bug 170223
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug