Bug 168348

Summary: [WebRTC] ICE candidates should be filtered according a policy
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, eric.carlson, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=168392
Attachments:
Description Flags
WIP
none
Patch
none
Patch
none
Fixing EFL
none
Patch
none
Patch
none
Fixing GTK build
none
Fixing relay comment none

Description youenn fablet 2017-02-14 18:02:00 PST
The policy should be controlled by the UIProcess in WebKit2
Comment 1 youenn fablet 2017-02-14 18:04:35 PST
Created attachment 301567 [details]
WIP
Comment 2 youenn fablet 2017-02-15 10:25:07 PST
Created attachment 301630 [details]
Patch
Comment 3 youenn fablet 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.
Comment 4 youenn fablet 2017-02-15 10:30:07 PST
Created attachment 301631 [details]
Patch
Comment 5 Eric Carlson 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"
Comment 6 youenn fablet 2017-02-15 10:47:21 PST
Created attachment 301632 [details]
Fixing EFL
Comment 7 youenn fablet 2017-02-15 11:36:31 PST
Created attachment 301638 [details]
Patch
Comment 8 youenn fablet 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
Comment 9 Radar WebKit Bug Importer 2017-02-15 14:45:29 PST
<rdar://problem/30543773>
Comment 10 Alex Christensen 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
Comment 11 youenn fablet 2017-02-17 17:05:05 PST
Created attachment 302015 [details]
Patch
Comment 12 youenn fablet 2017-02-17 18:39:10 PST
Created attachment 302033 [details]
Fixing GTK build
Comment 13 Alex Christensen 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?
Comment 14 youenn fablet 2017-02-17 18:56:21 PST
Created attachment 302035 [details]
Fixing relay comment
Comment 15 youenn fablet 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
Comment 16 WebKit Commit Bot 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>
Comment 17 WebKit Commit Bot 2017-02-21 13:30:53 PST
All reviewed patches have been landed.  Closing bug.