The policy should be controlled by the UIProcess in WebKit2
Created attachment 301567 [details] WIP
Created attachment 301630 [details] Patch
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.
Created attachment 301631 [details] Patch
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"
Created attachment 301632 [details] Fixing EFL
Created attachment 301638 [details] Patch
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
<rdar://problem/30543773>
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
Created attachment 302015 [details] Patch
Created attachment 302033 [details] Fixing GTK build
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?
Created attachment 302035 [details] Fixing relay comment
(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
Comment on attachment 302035 [details] Fixing relay comment Clearing flags on attachment: 302035 Committed r212745: <http://trac.webkit.org/changeset/212745>
All reviewed patches have been landed. Closing bug.