[WebRTC] Activate ICE candidate privacy policy

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

We should activate ICE candidate privacy policy by default and find a way to disable it when needed.

[youenn fablet]

Created attachment 302956 [details]
Patch

[youenn fablet]

This patch is a second step towards implementing privacy policy.
We should add tests and probably reactivate filtering in some cases.
For instance when access is being revoked from the address bar.

[Eric Carlson]

Comment on attachment 302956 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302956&action=review

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:209
> +#if ENABLE(WEB_RTC) && USE(LIBWEBRTC)
> +    m_page.process().send(Messages::WebPage::DisableICECandidateFiltering(), m_page.pageID());
> +#endif

I don't think this should be done here, because it will disable filtering if the user accepts one stream and denies another. Instead, I think it should be in UserMediaProcessManager::willCreateMediaStream and UserMediaProcessManager::endedCaptureSession.

[youenn fablet]

Created attachment 303074 [details]
Patch

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 302956 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302956&action=review
> 
> > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:209
> > +#if ENABLE(WEB_RTC) && USE(LIBWEBRTC)
> > +    m_page.process().send(Messages::WebPage::DisableICECandidateFiltering(), m_page.pageID());
> > +#endif
> 
> I don't think this should be done here, because it will disable filtering if
> the user accepts one stream and denies another. Instead, I think it should
> be in UserMediaProcessManager::willCreateMediaStream and
> UserMediaProcessManager::endedCaptureSession.

Thanks for the feedback.
I updated patch accordingly.
I also added a layout test for it.

[Eric Carlson]

Comment on attachment 303074 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303074&action=review

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:205
> +#if ENABLE(WEB_RTC) && USE(LIBWEBRTC)
> +    m_page.process().send(Messages::WebPage::DisableICECandidateFiltering(), m_page.pageID());
> +#endif

Don't you want to remove this?

> Source/WebKit2/UIProcess/UserMediaProcessManager.cpp:159
> +#if ENABLE(WEB_RTC) && USE(LIBWEBRTC)
> +    if (currentExtensions == ProcessState::SandboxExtensionsGranted::None)
> +        proxy.page().process().send(Messages::WebPage::DisableICECandidateFiltering(), proxy.page().pageID());
> +#endif

Shouldn't this be in UserMediaProcessManager::endedCaptureSession?

[youenn fablet]

Created attachment 303143 [details]
Patch

[youenn fablet]

(In reply to comment #6)
> Comment on attachment 303074 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303074&action=review
> 
> > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:205
> > +#if ENABLE(WEB_RTC) && USE(LIBWEBRTC)
> > +    m_page.process().send(Messages::WebPage::DisableICECandidateFiltering(), m_page.pageID());
> > +#endif
> 
> Don't you want to remove this?

Right

> > Source/WebKit2/UIProcess/UserMediaProcessManager.cpp:159
> > +#if ENABLE(WEB_RTC) && USE(LIBWEBRTC)
> > +    if (currentExtensions == ProcessState::SandboxExtensionsGranted::None)
> > +        proxy.page().process().send(Messages::WebPage::DisableICECandidateFiltering(), proxy.page().pageID());
> > +#endif
> 
> Shouldn't this be in UserMediaProcessManager::endedCaptureSession?

We want to disable filtering when capture is authorised, so I think this is ok like this. I reworked a bit the way this is handled to ensure we got good coverage here.

[youenn fablet]

Eric, could you check that the granting/revocation goes well with the disable/enable filtering?
Alex, one thing to consider is that we probably do not want these preferences to stay persistent WebKit launch after WebKit launch. Maybe more a Safari issue though but this may be the two-only preferences that act like this currently.

[WebKit Commit Bot]

Comment on attachment 303143 [details]
Patch

Clearing flags on attachment: 303143

Committed r213283: <http://trac.webkit.org/changeset/213283>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.