WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(31.44 KB, patch)
2017-02-15 10:25 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(31.43 KB, patch)
2017-02-15 10:30 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Fixing EFL
(31.47 KB, patch)
2017-02-15 10:47 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(31.49 KB, patch)
2017-02-15 11:36 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(33.50 KB, patch)
2017-02-17 17:05 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Fixing GTK build
(34.13 KB, patch)
2017-02-17 18:39 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Fixing relay comment
(32.43 KB, patch)
2017-02-17 18:56 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2017-02-14 18:04:35 PST
Created
attachment 301567
[details]
WIP
youenn fablet
Comment 2
2017-02-15 10:25:07 PST
Created
attachment 301630
[details]
Patch
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
Created
attachment 301631
[details]
Patch
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
Created
attachment 301638
[details]
Patch
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
<
rdar://problem/30543773
>
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
Created
attachment 302015
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug