RESOLVED FIXED 168348
[WebRTC] ICE candidates should be filtered according a policy
https://bugs.webkit.org/show_bug.cgi?id=168348
Summary [WebRTC] ICE candidates should be filtered according a policy
youenn fablet
Reported 2017-02-14 18:02:00 PST
The policy should be controlled by the UIProcess in WebKit2
Attachments
WIP (32.03 KB, patch)
2017-02-14 18:04 PST, youenn fablet
no flags
Patch (31.44 KB, patch)
2017-02-15 10:25 PST, youenn fablet
no flags
Patch (31.43 KB, patch)
2017-02-15 10:30 PST, youenn fablet
no flags
Fixing EFL (31.47 KB, patch)
2017-02-15 10:47 PST, youenn fablet
no flags
Patch (31.49 KB, patch)
2017-02-15 11:36 PST, youenn fablet
no flags
Patch (33.50 KB, patch)
2017-02-17 17:05 PST, youenn fablet
no flags
Fixing GTK build (34.13 KB, patch)
2017-02-17 18:39 PST, youenn fablet
no flags
Fixing relay comment (32.43 KB, patch)
2017-02-17 18:56 PST, youenn fablet
no flags
youenn fablet
Comment 1 2017-02-14 18:04:35 PST
youenn fablet
Comment 2 2017-02-15 10:25:07 PST
youenn fablet
Comment 3 2017-02-15 10:26:34 PST
Patch adds the infrastructure to filter out ICE candidate according a given policy defined by the UI process. At this early stage, current policy is to not disable any filtering.
youenn fablet
Comment 4 2017-02-15 10:30:07 PST
Eric Carlson
Comment 5 2017-02-15 10:44:14 PST
Comment on attachment 301631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301631&action=review > Source/WebCore/ChangeLog:14 > + Stored candidates may be asvertised to the JS layer if RTCController is notified of a change of filtering policy. Nit: "asvertised" -> "advertised" > Source/WebCore/ChangeLog:15 > + To implement that, PeerConnectionBackend is storing all filtered out candidates and RTCPeerConnection register Nit: "is storing" -> "stores" > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:293 > + Vector<String> items; Why not use a StringBuilder, is a Vector<String> more efficient? > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:307 > + bool skip = false; > + bool isFirst = true; > + StringBuilder filteredSDP; > + for (auto& item : items) { > + if (skip) { > + skip = false; > + continue; > + } > + if (item == "raddr" || item == "rport") { > + skip = true; > + continue; > + } Nit: I had to read this several times to understand the logic, renaming the variable to something like "skipNextItem" would make it easier to understand at a glance. > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:67 > + // RTCPeerConnection may send events at about any time during its lifetime. Nit: "about" is unnecessary. > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:68 > + // Let's make it uncollectable until the pc is closed by JS or the page is stopping it. Nit: "the page is stopping it" -> "the page stops it"
youenn fablet
Comment 6 2017-02-15 10:47:21 PST
Created attachment 301632 [details] Fixing EFL
youenn fablet
Comment 7 2017-02-15 11:36:31 PST
youenn fablet
Comment 8 2017-02-15 11:37:49 PST
Thanks for the feedback. (In reply to comment #5) > Comment on attachment 301631 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301631&action=review > > > Source/WebCore/ChangeLog:14 > > + Stored candidates may be asvertised to the JS layer if RTCController is notified of a change of filtering policy. > > Nit: "asvertised" -> "advertised" OK > > Source/WebCore/ChangeLog:15 > > + To implement that, PeerConnectionBackend is storing all filtered out candidates and RTCPeerConnection register > > Nit: "is storing" -> "stores" OK > > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:293 > > + Vector<String> items; > > Why not use a StringBuilder, is a Vector<String> more efficient? We use a StringBuilder to write the new string. To split the filtered string, we use the Vector. > > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:307 > > + bool skip = false; > > + bool isFirst = true; > > + StringBuilder filteredSDP; > > + for (auto& item : items) { > > + if (skip) { > > + skip = false; > > + continue; > > + } > > + if (item == "raddr" || item == "rport") { > > + skip = true; > > + continue; > > + } > > Nit: I had to read this several times to understand the logic, renaming the > variable to something like "skipNextItem" would make it easier to understand > at a glance. OK
Radar WebKit Bug Importer
Comment 9 2017-02-15 14:45:29 PST
Alex Christensen
Comment 10 2017-02-16 15:59:26 PST
Comment on attachment 301638 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301638&action=review We really ought to move the filtering to the NetworkProcess. > Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:148 > + String sdp; > + String mid; These need better names or explanations of what they are. > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:97 > + if (configuration.iceTransportPolicy == PeerConnectionStates::IceTransportPolicy::Relay) > + disableICECandidateFiltering(); I think this isn't worth the risk of leaking privacy information for any benefits from skipping filtering. > Source/WebKit2/UIProcess/WebPageProxy.cpp:832 > +#if ENABLE(WEB_RTC) > + // FIXME: Tie ICE filtering with getUserMedia permission. > + m_process->send(Messages::WebPage::DisableICECandidateFiltering(), m_pageID); > +#if USE(LIBWEBRTC) > + // FIXME: Turn down network interface enumeration by default. > + m_process->send(Messages::WebPage::EnableEnumeratingAllNetworkInterfaces(), m_pageID); > +#endif > +#endif This should be put in the creationParameters instead of a message sent just after WebProcess::CreateWebPage
youenn fablet
Comment 11 2017-02-17 17:05:05 PST
youenn fablet
Comment 12 2017-02-17 18:39:10 PST
Created attachment 302033 [details] Fixing GTK build
Alex Christensen
Comment 13 2017-02-17 18:48:56 PST
Comment on attachment 302033 [details] Fixing GTK build View in context: https://bugs.webkit.org/attachment.cgi?id=302033&action=review > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:94 > + if (configuration.iceTransportPolicy == PeerConnectionStates::IceTransportPolicy::Relay) > + disableICECandidateFiltering(); Is there a reason this wasn't removed?
youenn fablet
Comment 14 2017-02-17 18:56:21 PST
Created attachment 302035 [details] Fixing relay comment
youenn fablet
Comment 15 2017-02-17 18:56:52 PST
(In reply to comment #13) > Comment on attachment 302033 [details] > Fixing GTK build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302033&action=review > > > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:94 > > + if (configuration.iceTransportPolicy == PeerConnectionStates::IceTransportPolicy::Relay) > > + disableICECandidateFiltering(); > > Is there a reason this wasn't removed? The change got lost... fixed in latest patch
WebKit Commit Bot
Comment 16 2017-02-21 13:30:48 PST
Comment on attachment 302035 [details] Fixing relay comment Clearing flags on attachment: 302035 Committed r212745: <http://trac.webkit.org/changeset/212745>
WebKit Commit Bot
Comment 17 2017-02-21 13:30:53 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.