Bug 168348 – [WebRTC] ICE candidates should be filtered according a policy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

The policy should be controlled by the UIProcess in WebKit2

[youenn fablet]

Created attachment 301567 [details]
WIP

[youenn fablet]

Created attachment 301630 [details]
Patch

[youenn fablet]

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]

Created attachment 301631 [details]
Patch

[Eric Carlson]

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]

Created attachment 301632 [details]
Fixing EFL

[youenn fablet]

Created attachment 301638 [details]
Patch

[youenn fablet]

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]

<rdar://problem/30543773>

[Alex Christensen]

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]

Created attachment 302015 [details]
Patch

[youenn fablet]

Created attachment 302033 [details]
Fixing GTK build

[Alex Christensen]

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]

Created attachment 302035 [details]
Fixing relay comment

[youenn fablet]

(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 on attachment 302035 [details]
Fixing relay comment

Clearing flags on attachment: 302035

Committed r212745: <http://trac.webkit.org/changeset/212745>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.