We should do ICE candidate filtering at the Document level

• Component: Media
• Status: RESOLVED FIXED

[youenn fablet]

Currently, we are doing it at the page level, meaning that if the main frame has access to the camera, third party frames can get access to ICE candidates.

[Radar WebKit Bug Importer]

<rdar://problem/33122058>

[youenn fablet]

Created attachment 314980 [details]
Patch

[youenn fablet]

This patch moves

[Build Bot]

Comment on attachment 314980 [details]
Patch

Attachment 314980 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4091910

New failing tests:
http/tests/webrtc/filtering-ice-candidate-same-origin-frame.html
http/tests/webrtc/filtering-ice-candidate-cross-origin-frame.html

[Build Bot]

Created attachment 314991 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314980 [details]
Patch

Attachment 314980 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4091841

New failing tests:
http/tests/webrtc/filtering-ice-candidate-same-origin-frame.html
http/tests/webrtc/filtering-ice-candidate-cross-origin-frame.html

[Build Bot]

Created attachment 314992 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[youenn fablet]

Created attachment 315006 [details]
Patch

[youenn fablet]

Created attachment 315007 [details]
Patch

[youenn fablet]

Created attachment 315012 [details]
Fixing non WebRTC build

[youenn fablet]

Created attachment 315018 [details]
Fixing non WebRTC build

[youenn fablet]

Created attachment 315082 [details]
Keeping controller at page

[Build Bot]

Comment on attachment 315082 [details]
Keeping controller at page

Attachment 315082 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4098759

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html

[Build Bot]

Created attachment 315085 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Eric Carlson]

Comment on attachment 315082 [details]
Keeping controller at page

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

> Source/WebCore/ChangeLog:25
> +        Making UserMediaRequest disable the ICE candidate filtering for the page RTCController.
> +        All RTCPeerConnection of the page that are created on a document that are same-origin as the top document are now registered to the RTCController.
> +        This allows disabling filtering to only these RTCPeerConnection.
> +
> +        The page keeps the default ICE candidate filtering policy.
> +        This policy allows disabling ICE candidate filtering for all RTCPeerConnection.
> +
> +        When the top document is changing, the RTCController filtering policy is reset and its list of RTCPeerConnection is emptied.
> +
> +        Internals no longer disables ICE candidate filtering by default.
> +        This allows finer grained testing.
> +        ICE candidate filtering is disabled for tests including testharnessreport.js to enable web-platform-tests to run without modifications.

Nit: please wrap these lines to make them easier to read.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:72
> +        // ICE candidate filtering can only be disabled for connections from documents that have the same origin as the top document, or if the page was set to disable it.

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:585
>  #if ENABLE(WEB_RTC)
>  void WebPage::disableICECandidateFiltering()
>  {
> -    m_page->rtcController().disableICECandidateFiltering();
> +    m_page->disableICECandidateFiltering();
>  }
>  
>  void WebPage::enableICECandidateFiltering()
>  {
> -    m_page->rtcController().disableICECandidateFiltering();
> +    m_page->enableICECandidateFiltering();
>  }

Are these needed now that UserMediaProcessManager is no longer involved in managing filtering?

[youenn fablet]

Thanks for the review.

(In reply to Eric Carlson from comment #15)
> Comment on attachment 315082 [details]
> Keeping controller at page
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315082&action=review
> 
> > Source/WebCore/ChangeLog:25
> > +        Making UserMediaRequest disable the ICE candidate filtering for the page RTCController.
> > +        All RTCPeerConnection of the page that are created on a document that are same-origin as the top document are now registered to the RTCController.
> > +        This allows disabling filtering to only these RTCPeerConnection.
> > +
> > +        The page keeps the default ICE candidate filtering policy.
> > +        This policy allows disabling ICE candidate filtering for all RTCPeerConnection.
> > +
> > +        When the top document is changing, the RTCController filtering policy is reset and its list of RTCPeerConnection is emptied.
> > +
> > +        Internals no longer disables ICE candidate filtering by default.
> > +        This allows finer grained testing.
> > +        ICE candidate filtering is disabled for tests including testharnessreport.js to enable web-platform-tests to run without modifications.
> 
> Nit: please wrap these lines to make them easier to read.

OK

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:72
> > +        // ICE candidate filtering can only be disabled for connections from documents that have the same origin as the top document, or if the page was set to disable it.
> 
> Ditto.

OK

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:585
> >  #if ENABLE(WEB_RTC)
> >  void WebPage::disableICECandidateFiltering()
> >  {
> > -    m_page->rtcController().disableICECandidateFiltering();
> > +    m_page->disableICECandidateFiltering();
> >  }
> >  
> >  void WebPage::enableICECandidateFiltering()
> >  {
> > -    m_page->rtcController().disableICECandidateFiltering();
> > +    m_page->enableICECandidateFiltering();
> >  }
> 
> Are these needed now that UserMediaProcessManager is no longer involved in
> managing filtering?

Yes, the UIProcess should use these to modify existing web process behaviors when the UIProcess ICE candidate filtering policy option is modified.
Currently, this only takes effect for new tabs.

[youenn fablet]

Created attachment 315111 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315111 [details]
Patch

Clearing flags on attachment: 315111

Committed r219331: <http://trac.webkit.org/changeset/219331>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Looks like it broke WPE (not sure about GTK as that bot has not caught up yet):

/home/buildbot/wpe/wpe-linux-64-release/build/Source/WebKit2/UIProcess/WebPageProxy.cpp: In member function ‘void WebKit::WebPageProxy::activateMediaStreamCaptureInPage()’:
/home/buildbot/wpe/wpe-linux-64-release/build/Source/WebKit2/UIProcess/WebPageProxy.cpp:1672:5: error: ‘UserMediaProcessManager’ has not been declared
     UserMediaProcessManager::singleton().muteCaptureMediaStreamsExceptIn(*this);
     ^~~~~~~~~~~~~~~~~~~~~~~
At global scope:

I'm confused how it happened. I don't see what portion of the change caused this failure. WebPageProxy.cpp was not even modified, and it still includes UserMediaProcessManager.h.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #20)
> (not sure about GTK as that bot has not caught up yet)

Our secondary bots are caught up, and they are hitting the same failure.

[Michael Catanzaro]

Ah, mystery solved. The build breakage was from r219330, not this change. Whoops. :)

[youenn fablet]

(In reply to Michael Catanzaro from comment #22)
> Ah, mystery solved. The build breakage was from r219330, not this change.
> Whoops. :)

I will fix r219330 issue asap.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 315153 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #25)
> Created attachment 315153 [details]
> Patch

Fixing the case of bots without WebRTC support.

[WebKit Commit Bot]

Comment on attachment 315153 [details]
Patch

Clearing flags on attachment: 315153

Committed r219362: <http://trac.webkit.org/changeset/219362>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

*** Bug 174112 has been marked as a duplicate of this bug. ***

[Matt Lewis]

Marked tests as skip:
https://trac.webkit.org/changeset/219478

[Matt Lewis]

These tests started to become flaky on macOS WK2 as well.
https://trac.webkit.org/changeset/219748/webkit