Report if video capturing meets output protection requirement.

Report if there is any ongoing insecured capturing. The capturing is
deemed secure if all connected video sinks are reported secure and
the extension is trusted.

BUG=601859
Committed: https://crrev.com/f6f63a8db446f2114984ca8e48c28097155bb0e3
Cr-Commit-Position: refs/heads/master@{#393005}

[xjz, 2016-04-11 19:26:26]

Patchset #1 (id:1) has been deleted

[xjz, 2016-04-11 19:30:05]

xjz@chromium.org changed reviewers:
+ miu@chromium.org, xhwang@chromium.org

[xjz, 2016-04-11 19:30:06]

PTAL.
Here is a brief description of this CL:
https://docs.google.com/a/google.com/document/d/1_nLvP4hL2QaRoUiBH3rSLy4jppjU...

Here is the flow chart of parsing:
https://docs.google.com/a/google.com/presentation/d/10q_vT95LHyaEcH7bsUGdKE4k...

[xhwang, 2016-04-12 22:18:50]

The design doc lg but I am not really familiar with this area. I only reviewed
the API and have a few comments/nits.

https://chromiumcodereview.appspot.com/1873293002/diff/20001/chrome/browser/m...
File chrome/browser/media/media_capture_devices_dispatcher.h (right):

https://chromiumcodereview.appspot.com/1873293002/diff/20001/chrome/browser/m...
chrome/browser/media/media_capture_devices_dispatcher.h:155: //
|is_link_secure|.
We are more interested in whether all links are secure, rather than whether "it
has secure link".

https://chromiumcodereview.appspot.com/1873293002/diff/20001/chrome/browser/m...
chrome/browser/media/media_capture_devices_dispatcher.h:158: bool*
is_link_secure);
Does this cover all capturing cases, e.g. desktop/screen/tab?

[xjz, 2016-04-12 22:29:08]

https://chromiumcodereview.appspot.com/1873293002/diff/20001/chrome/browser/m...
File chrome/browser/media/media_capture_devices_dispatcher.h (right):

https://chromiumcodereview.appspot.com/1873293002/diff/20001/chrome/browser/m...
chrome/browser/media/media_capture_devices_dispatcher.h:155: //
|is_link_secure|.
On 2016/04/12 22:18:50, xhwang wrote:
> We are more interested in whether all links are secure, rather than whether
"it
> has secure link".

Yes. |is_link_secure| is true only when all links connected with this content
are secure. The comment is not accurate. I Will modify it in the next patch.

https://chromiumcodereview.appspot.com/1873293002/diff/20001/chrome/browser/m...
chrome/browser/media/media_capture_devices_dispatcher.h:158: bool*
is_link_secure);
On 2016/04/12 22:18:49, xhwang wrote:
> Does this cover all capturing cases, e.g. desktop/screen/tab?

Yes.

[xjz, 2016-04-13 23:04:05]

Patchset #2 (id:40001) has been deleted

[xjz, 2016-04-13 23:04:55]

Addressed xhwang's comments and some fixes.

[miu, 2016-04-16 00:05:30]

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:448:
extension_whitelisted_ = MediaCaptureDevicesDispatcher::IsOriginForCasting(
This value will be different for each session.  In other words, you need one of
these for each combination of
{render_process_id,render_frame_id,page_request_id}.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:500: it->render_frame_id
== render_frame_id) {
You're not checking page_request_id here.  So, should you only set
|*is_link_secure| to true when ALL sessions (i.e., those in the same render
frame, but with different page_request_id's) are secure?

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:512:
content::MediaStreamType stream_type,
IMO, passing and checking the stream_type here is unnecessary. 
MediaCaptureDevicesDispatcher can only be calling this method in this class
after it has explicitly checked the stream_type.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.h (right):

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:38: // TODO(xjz): To be
removed.
crbug link?

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:41: bool
IsCaptureInProgress(int render_process_id,
Seems like these two new methods should be in the MediaAccessHandler interface.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:70: // If the extension is
authorized for casting.
nit: Let's expand a bit on this comment here.  How about: "Extensions control
the routing of the captured MediaStream content.  Therefore, links can be deemed
secure only when built-in extensions (and certain whitelisted ones) are in
control of the routing."

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:71: bool
extension_whitelisted_;
naming nit (for accuracy): How about: trusted_extension_

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.h (right):

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.h:156: // secure and the
extension is white listed for casting.
s/extension is white listed for casting/extension is trusted/.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.h:159: bool*
is_link_secure);
Can you eliminate the 3rd argument, and just return it as true/false here?  (And
the same in DesktopCaptureAccessHandler and TabCaptureAccessHandler?)

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/ta...
File chrome/browser/media/tab_capture_access_handler.h (right):

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/ta...
chrome/browser/media/tab_capture_access_handler.h:40: int render_process_id_;
1. Shouldn't there be a list of render frames and |is_link_secure| state for
each, just like in DesktopCaptureAccessHandler?

2. Instead of duplicating all the "link secure" and "session struct" code here,
consider creating a base class that both DesktopCaptureAccessHandler and
TabCaptureAccessHandler extend from.  For example:

  class CaptureAccessHandlerBase : public MediaAccessHander {
   public:
    bool IsCaptureInProgress(...);
    void UpdateLinkSecured(...);

   private:
    struct Session {
      int render_process_id;
      int render_frame_id;
      int page_request_id;
      bool is_extension_trusted;
      bool is_capturing_link_secure;
    };

    std::list<Session> sessions_;
  };

  class DesktopCaptureAccessHander : public CaptureAccessHandlerBase { ... };
  class TabCaptureAccessHander : public CaptureAccessHandlerBase { ... };

https://codereview.chromium.org/1873293002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/1873293002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:2139:
request->SetCapturingLinkSecured(is_secure);
This sets all of them, not the |request| specific to a particular capture
session.  This method needs to take a label argument, and only modify the one
DeviceRequest having that label.

https://codereview.chromium.org/1873293002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/1873293002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_track.cc:294:
video_capturing_link_secured_ &= is_secure;
You need to track these booleans individually.  For example, this logic fails
for this sequence:

1. Add secure sink.  Track is secure.
2. Add insecure sink.  Track is no longer secure.
3. Remove insecure sink.  Track should be secure again (because only a secure
sink remains connected).

This also needs to be fixed in the MediaStreamVideoSources.

https://codereview.chromium.org/1873293002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/1873293002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_track.h:56: void
SetVideoSinkSecured(bool is_secure);
Instead of this separate method, could this bool be passed as an arg to
AddSink()?

[xjz, 2016-04-21 16:57:22]

Patchset #3 (id:80001) has been deleted

[xjz, 2016-04-21 17:27:32]

Patchset #3 (id:100001) has been deleted

[xjz, 2016-04-21 18:38:23]

Patchset #3 (id:120001) has been deleted

[xjz, 2016-04-21 21:45:34]

Patchset #3 (id:140001) has been deleted

[xjz, 2016-04-21 23:49:26]

Patchset #3 (id:160001) has been deleted

[xjz, 2016-04-21 23:50:45]

Thanks Yuri! Addressed your comments. PTAL.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:448:
extension_whitelisted_ = MediaCaptureDevicesDispatcher::IsOriginForCasting(
On 2016/04/16 00:05:29, miu wrote:
> This value will be different for each session.  In other words, you need one
of
> these for each combination of
> {render_process_id,render_frame_id,page_request_id}.

Done.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:500: it->render_frame_id
== render_frame_id) {
On 2016/04/16 00:05:29, miu wrote:
> You're not checking page_request_id here.  So, should you only set
> |*is_link_secure| to true when ALL sessions (i.e., those in the same render
> frame, but with different page_request_id's) are secure?

Done. Check all sessions with same render frame.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:512:
content::MediaStreamType stream_type,
On 2016/04/16 00:05:29, miu wrote:
> IMO, passing and checking the stream_type here is unnecessary. 
> MediaCaptureDevicesDispatcher can only be calling this method in this class
> after it has explicitly checked the stream_type.

Done.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.h (right):

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:38: // TODO(xjz): To be
removed.
On 2016/04/16 00:05:29, miu wrote:
> crbug link?

Do we need a crbug for this one? It will be removed after content output
protection switches to use the new API.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:41: bool
IsCaptureInProgress(int render_process_id,
On 2016/04/16 00:05:29, miu wrote:
> Seems like these two new methods should be in the MediaAccessHandler
interface.

Done. Moved them to the new CaptureAccessHandlerBase class.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:70: // If the extension is
authorized for casting.
On 2016/04/16 00:05:29, miu wrote:
> nit: Let's expand a bit on this comment here.  How about: "Extensions control
> the routing of the captured MediaStream content.  Therefore, links can be
deemed
> secure only when built-in extensions (and certain whitelisted ones) are in
> control of the routing."

Moved it into the |session| structure, as it should relate to each capturing
session.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:71: bool
extension_whitelisted_;
On 2016/04/16 00:05:29, miu wrote:
> naming nit (for accuracy): How about: trusted_extension_

Moved it into the |Session| structure.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.h (right):

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.h:156: // secure and the
extension is white listed for casting.
On 2016/04/16 00:05:29, miu wrote:
> s/extension is white listed for casting/extension is trusted/.

Done.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.h:159: bool*
is_link_secure);
On 2016/04/16 00:05:29, miu wrote:
> Can you eliminate the 3rd argument, and just return it as true/false here? 
(And
> the same in DesktopCaptureAccessHandler and TabCaptureAccessHandler?)

Done. Initially I thought we need to report if capturing is in process. After
disscussed with Xiaohan offline, it seems that the output protection only cares
if there is any insecure video capturing in process. Renamed the API as
|IsInsecuredCapturingInProcess| and removed the 3rd argument.

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/ta...
File chrome/browser/media/tab_capture_access_handler.h (right):

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/ta...
chrome/browser/media/tab_capture_access_handler.h:40: int render_process_id_;
On 2016/04/16 00:05:29, miu wrote:
> 1. Shouldn't there be a list of render frames and |is_link_secure| state for
> each, just like in DesktopCaptureAccessHandler?
> 
> 2. Instead of duplicating all the "link secure" and "session struct" code
here,
> consider creating a base class that both DesktopCaptureAccessHandler and
> TabCaptureAccessHandler extend from.  For example:
> 
>   class CaptureAccessHandlerBase : public MediaAccessHander {
>    public:
>     bool IsCaptureInProgress(...);
>     void UpdateLinkSecured(...);
> 
>    private:
>     struct Session {
>       int render_process_id;
>       int render_frame_id;
>       int page_request_id;
>       bool is_extension_trusted;
>       bool is_capturing_link_secure;
>     };
> 
>     std::list<Session> sessions_;
>   };
> 
>   class DesktopCaptureAccessHander : public CaptureAccessHandlerBase { ... };
>   class TabCaptureAccessHander : public CaptureAccessHandlerBase { ... };

Done.

https://codereview.chromium.org/1873293002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/1873293002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:2139:
request->SetCapturingLinkSecured(is_secure);
On 2016/04/16 00:05:29, miu wrote:
> This sets all of them, not the |request| specific to a particular capture
> session.  This method needs to take a label argument, and only modify the one
> DeviceRequest having that label.

Done. Sets the requests with same session_id and MediaStreamType (as session_id
is unique for all capture sessions of the same type).

https://codereview.chromium.org/1873293002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/1873293002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_track.cc:294:
video_capturing_link_secured_ &= is_secure;
On 2016/04/16 00:05:29, miu wrote:
> You need to track these booleans individually.  For example, this logic fails
> for this sequence:
> 
> 1. Add secure sink.  Track is secure.
> 2. Add insecure sink.  Track is no longer secure.
> 3. Remove insecure sink.  Track should be secure again (because only a secure
> sink remains connected).
> 
> This also needs to be fixed in the MediaStreamVideoSources.

Done.

https://codereview.chromium.org/1873293002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/1873293002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_track.h:56: void
SetVideoSinkSecured(bool is_secure);
On 2016/04/16 00:05:29, miu wrote:
> Instead of this separate method, could this bool be passed as an arg to
> AddSink()?

Done.

[miu, 2016-04-26 01:25:14]

Looking good!  Comments on PS3:

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.h (right):

https://codereview.chromium.org/1873293002/diff/60001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.h:38: // TODO(xjz): To be
removed.
On 2016/04/21 23:50:44, xjz wrote:
> On 2016/04/16 00:05:29, miu wrote:
> > crbug link?
> 
> Do we need a crbug for this one? It will be removed after content output
> protection switches to use the new API.

That's fine, if you know this will happen soon.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:21: bool is_link_secure) {
I'm thinking, when a capture session is first started, there are no sinks yet. 
So, to avoid problems, perhaps you don't want to have the |is_link_secure|
argument here (just always default it to true)?

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:113: if (sessions_.size() <=
0)
nit: size_t can never be less than zero.  How about just:

  if (sessions_.empty())

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:117: for (auto it =
sessions_.begin(); it != sessions_.end(); ++it) {
This seems a bit more complex than it needs to be.  Suggestion:

  for (const Session& session : sessions) {
    if (session.rp_id != rp_id || session.rf_id != rf_id)
      continue;
    if (!session.is_extension_trusted || !session.is_capturing_link_secure)
      return true;
  }
  return false;

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:135: if (it !=
sessions_.end()) {
Will/should this ever be false?  I suppose we don't care about updates after a
session has ended (this would indicate stray IPCs, IIUC).  Could it possibly
ever happen before AddCaptureSession gets called?

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.h (right):

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:4: #ifndef
CHROME_BROWSER_MEDIA_CAPTURE_ACCESS_HANDLER_BASE_H_
nit: need one blank line above this line

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:35: // extension is trusted.
s/and the extensions is trusted/and the connections to the sinks are being
managed by a trusted extension/

(Because capture can also be initiated from web pages using
navigator.webkitGetUserMedia().)

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:50: void
AddCaptureSession(int render_process_id,
It seems that subclasses don't call Add/RemoveCaptureSession() directly.  So,
perhaps these should be made private methods?

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:67: // Extensions control the
routing of the captured MediaStream content.
Sometimes normal web pages can capture/route content.  This comment, like the
one above, should mention that only trusted extensions (and never normal web
pages) can be trusted to set-up secure links.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/d...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/d...
chrome/browser/media/desktop_capture_access_handler.cc:448: bool
is_extension_trusted =
Consider rolling this into CaptureAccessHandlerBase::UpdateExtensionTrusted(),
since all of these should be trusted for tab capture as well (i.e., if they ever
someday decide to use tab capture).

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/m...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/m...
chrome/browser/media/media_capture_devices_dispatcher.cc:69: inline
DesktopCaptureAccessHandler* ToDesktopCaptureAccessHandler(
I think you can delete this function now.

https://codereview.chromium.org/1873293002/diff/180001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:2137: for
(LabeledDeviceRequest labeled_request : requests_) {
s/LabeledDeviceRequest/const LabeledDeviceRequest&/  (to avoid making a copy)

https://codereview.chromium.org/1873293002/diff/180001/content/common/media/v...
File content/common/media/video_capture_messages.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/common/media/v...
content/common/media/video_capture_messages.h:122:
IPC_MESSAGE_CONTROL3(VideoCaptureHostMsg_SetCapturingLinkSecured,
Hmm...It seems like this should be a MediaStreamHostMsg instead (in
content/common/media/media_stream_messages.h).  Having VideoCaptureHost worry
about MediaStreamTypes feels like an abstraction violation.

https://codereview.chromium.org/1873293002/diff/180001/content/public/browser...
File content/public/browser/media_observer.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/public/browser...
content/public/browser/media_observer.h:37: // Called when setting if the video
capturing is secure.
s/if/whether/

I'd suggest expanding this comment a bit more to provide more context. 
Something like: Called when the secure display link status of one or more
consumers of this stream has changed.

https://codereview.chromium.org/1873293002/diff/180001/content/public/rendere...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/public/rendere...
content/public/renderer/media_stream_video_sink.h:46: // meets output protection
requirement).
nit: Consider adding a cautionary comment like: "Generally, this should be false
unless you know what you are doing."

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
File content/renderer/media/capturing_link_secure_tracker.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/capturing_link_secure_tracker.h:5: #ifndef
CONTENT_RENDERER_MEDIA_CAPTURING_LINK_SECURT_TRACKER_H_
s/SECURT/SECURE/

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/capturing_link_secure_tracker.h:13: class
CapturingLinkSecureTracker {
Suggestion: Since this is tracking whether downstream display of the video is
secure, consider naming this something like SecureDisplayLinkTracker.

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/capturing_link_secure_tracker.h:18: void AddLinkSecure(T*
link, bool is_link_secure);
naming nit: How about just Add(), Remove(), and Update()?

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/capturing_link_secure_tracker.h:27:
std::vector<std::pair<T*, bool>> links_secure_status_;
Idea: What if this class just maintained a vector of *only* the insecure links? 
Then, you don't need the code to re-compute the overall secure status in the
Add/Remove/Update() methods.  Also, is_capturing_secure() then becomes:

  bool is_capturing_secure() const { return insecure_links_.empty(); }

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:57: true);
Consider adding a comment like: Local display video rendering is considered a
secure sink.

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/media_stream_video_source.h:197:
std::unique_ptr<CapturingLinkSecureTracker<MediaStreamVideoTrack>>
Please remove the indirection here.

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/media_stream_video_track.h:95:
std::unique_ptr<CapturingLinkSecureTracker<MediaStreamVideoSink>>
ditto: Extra indirection unnecessary here.

https://codereview.chromium.org/1873293002/diff/180001/media/base/video_captu...
File media/base/video_capturer_source.h (right):

https://codereview.chromium.org/1873293002/diff/180001/media/base/video_captu...
media/base/video_capturer_source.h:91: virtual void SetCapturingLinkSecured(bool
is_secure) {}
Do we need this in the interface definition?  It seems like this is side-detail
that only applies to the MediaStream framework in libcontent (and src/chrome
code).

[xjz, 2016-04-29 00:05:03]

Patchset #4 (id:200001) has been deleted

[xjz, 2016-04-29 00:11:43]

Yuri: Thanks! Addressed your comments. PTAL.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:21: bool is_link_secure) {
On 2016/04/26 01:25:13, miu wrote:
> I'm thinking, when a capture session is first started, there are no sinks yet.

> So, to avoid problems, perhaps you don't want to have the |is_link_secure|
> argument here (just always default it to true)?

Yes, you are correct. Actually I did always set it to false. Now removed this
input argument.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:113: if (sessions_.size() <=
0)
On 2016/04/26 01:25:13, miu wrote:
> nit: size_t can never be less than zero.  How about just:
> 
>   if (sessions_.empty())

Done.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:117: for (auto it =
sessions_.begin(); it != sessions_.end(); ++it) {
On 2016/04/26 01:25:13, miu wrote:
> This seems a bit more complex than it needs to be.  Suggestion:
> 
>   for (const Session& session : sessions) {
>     if (session.rp_id != rp_id || session.rf_id != rf_id)
>       continue;
>     if (!session.is_extension_trusted || !session.is_capturing_link_secure)
>       return true;
>   }
>   return false;

Done. This is much simpler and more readable. Thanks!

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:135: if (it !=
sessions_.end()) {
On 2016/04/26 01:25:13, miu wrote:
> Will/should this ever be false?  I suppose we don't care about updates after a
> session has ended (this would indicate stray IPCs, IIUC).  Could it possibly
> ever happen before AddCaptureSession gets called?

I think this will be false only when a session has ended. I don't think it will
happen before AddCaptureSession gets called, as  HandelRequest() will be called
first, which will call AddCaptureSession.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.h (right):

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:4: #ifndef
CHROME_BROWSER_MEDIA_CAPTURE_ACCESS_HANDLER_BASE_H_
On 2016/04/26 01:25:13, miu wrote:
> nit: need one blank line above this line

Done.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:35: // extension is trusted.
On 2016/04/26 01:25:13, miu wrote:
> s/and the extensions is trusted/and the connections to the sinks are being
> managed by a trusted extension/
> 
> (Because capture can also be initiated from web pages using
> navigator.webkitGetUserMedia().)

Done.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:50: void
AddCaptureSession(int render_process_id,
On 2016/04/26 01:25:13, miu wrote:
> It seems that subclasses don't call Add/RemoveCaptureSession() directly.  So,
> perhaps these should be made private methods?

Done.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:67: // Extensions control the
routing of the captured MediaStream content.
On 2016/04/26 01:25:13, miu wrote:
> Sometimes normal web pages can capture/route content.  This comment, like the
> one above, should mention that only trusted extensions (and never normal web
> pages) can be trusted to set-up secure links.

Done.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/d...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/d...
chrome/browser/media/desktop_capture_access_handler.cc:448: bool
is_extension_trusted =
On 2016/04/26 01:25:13, miu wrote:
> Consider rolling this into CaptureAccessHandlerBase::UpdateExtensionTrusted(),
> since all of these should be trusted for tab capture as well (i.e., if they
ever
> someday decide to use tab capture).

Done.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/m...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/m...
chrome/browser/media/media_capture_devices_dispatcher.cc:69: inline
DesktopCaptureAccessHandler* ToDesktopCaptureAccessHandler(
On 2016/04/26 01:25:13, miu wrote:
> I think you can delete this function now.

This is used by |IsDesktopCaptureInProgress()| for now, and will be removed
after output protection switches to use the new API for checking.

https://codereview.chromium.org/1873293002/diff/180001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:2137: for
(LabeledDeviceRequest labeled_request : requests_) {
On 2016/04/26 01:25:13, miu wrote:
> s/LabeledDeviceRequest/const LabeledDeviceRequest&/  (to avoid making a copy)

Done.

https://codereview.chromium.org/1873293002/diff/180001/content/common/media/v...
File content/common/media/video_capture_messages.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/common/media/v...
content/common/media/video_capture_messages.h:122:
IPC_MESSAGE_CONTROL3(VideoCaptureHostMsg_SetCapturingLinkSecured,
On 2016/04/26 01:25:13, miu wrote:
> Hmm...It seems like this should be a MediaStreamHostMsg instead (in
> content/common/media/media_stream_messages.h).  Having VideoCaptureHost worry
> about MediaStreamTypes feels like an abstraction violation.

The reason to use MediaStreamType here is that the |session_id| is unique only
for the same MediaStreamType. Now changed to use device id, which should also
work.

https://codereview.chromium.org/1873293002/diff/180001/content/public/browser...
File content/public/browser/media_observer.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/public/browser...
content/public/browser/media_observer.h:37: // Called when setting if the video
capturing is secure.
On 2016/04/26 01:25:13, miu wrote:
> s/if/whether/
> 
> I'd suggest expanding this comment a bit more to provide more context. 
> Something like: Called when the secure display link status of one or more
> consumers of this stream has changed.

Done.

https://codereview.chromium.org/1873293002/diff/180001/content/public/rendere...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/public/rendere...
content/public/renderer/media_stream_video_sink.h:46: // meets output protection
requirement).
On 2016/04/26 01:25:13, miu wrote:
> nit: Consider adding a cautionary comment like: "Generally, this should be
false
> unless you know what you are doing."

Done.

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
File content/renderer/media/capturing_link_secure_tracker.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/capturing_link_secure_tracker.h:5: #ifndef
CONTENT_RENDERER_MEDIA_CAPTURING_LINK_SECURT_TRACKER_H_
On 2016/04/26 01:25:13, miu wrote:
> s/SECURT/SECURE/

Done.

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/capturing_link_secure_tracker.h:13: class
CapturingLinkSecureTracker {
On 2016/04/26 01:25:14, miu wrote:
> Suggestion: Since this is tracking whether downstream display of the video is
> secure, consider naming this something like SecureDisplayLinkTracker.

Done.

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/capturing_link_secure_tracker.h:18: void AddLinkSecure(T*
link, bool is_link_secure);
On 2016/04/26 01:25:14, miu wrote:
> naming nit: How about just Add(), Remove(), and Update()?

Done.

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/capturing_link_secure_tracker.h:27:
std::vector<std::pair<T*, bool>> links_secure_status_;
On 2016/04/26 01:25:14, miu wrote:
> Idea: What if this class just maintained a vector of *only* the insecure
links? 
> Then, you don't need the code to re-compute the overall secure status in the
> Add/Remove/Update() methods.  Also, is_capturing_secure() then becomes:
> 
>   bool is_capturing_secure() const { return insecure_links_.empty(); }

Done. Smart idea! Saves many lines of codes and computing time. :-)

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:57: true);
On 2016/04/26 01:25:14, miu wrote:
> Consider adding a comment like: Local display video rendering is considered a
> secure sink.

Done.

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/media_stream_video_source.h:197:
std::unique_ptr<CapturingLinkSecureTracker<MediaStreamVideoTrack>>
On 2016/04/26 01:25:14, miu wrote:
> Please remove the indirection here.

Done.

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/renderer/media...
content/renderer/media/media_stream_video_track.h:95:
std::unique_ptr<CapturingLinkSecureTracker<MediaStreamVideoSink>>
On 2016/04/26 01:25:14, miu wrote:
> ditto: Extra indirection unnecessary here.

Done.

https://codereview.chromium.org/1873293002/diff/180001/media/base/video_captu...
File media/base/video_capturer_source.h (right):

https://codereview.chromium.org/1873293002/diff/180001/media/base/video_captu...
media/base/video_capturer_source.h:91: virtual void SetCapturingLinkSecured(bool
is_secure) {}
On 2016/04/26 01:25:14, miu wrote:
> Do we need this in the interface definition?  It seems like this is
side-detail
> that only applies to the MediaStream framework in libcontent (and src/chrome
> code).

Done. Removed the definition.

[miu, 2016-05-02 21:02:45]

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/m...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/m...
chrome/browser/media/media_capture_devices_dispatcher.cc:69: inline
DesktopCaptureAccessHandler* ToDesktopCaptureAccessHandler(
On 2016/04/29 00:11:42, xjz wrote:
> On 2016/04/26 01:25:13, miu wrote:
> > I think you can delete this function now.
> 
> This is used by |IsDesktopCaptureInProgress()| for now, and will be removed
> after output protection switches to use the new API for checking.

Yes, but can IsDesktopCaptureInProgress() just call ToCaptureAccessHandlerBase()
instead of this function?

https://codereview.chromium.org/1873293002/diff/180001/content/common/media/v...
File content/common/media/video_capture_messages.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/common/media/v...
content/common/media/video_capture_messages.h:122:
IPC_MESSAGE_CONTROL3(VideoCaptureHostMsg_SetCapturingLinkSecured,
On 2016/04/29 00:11:42, xjz wrote:
> On 2016/04/26 01:25:13, miu wrote:
> > Hmm...It seems like this should be a MediaStreamHostMsg instead (in
> > content/common/media/media_stream_messages.h).  Having VideoCaptureHost
worry
> > about MediaStreamTypes feels like an abstraction violation.
> 
> The reason to use MediaStreamType here is that the |session_id| is unique only
> for the same MediaStreamType. Now changed to use device id, which should also
> work. 

I should have explained more clearly before: What I'm concerned about is the
encapsulation of responsibilities/functionality here. MediaStreamManager is
responsible for managing control and permissions for the streams (and
bubbling-up metadata/state to the embedder), whereas VideoCaptureHost is only
supposed to be responsible for delivering video frames across browser-->renderer
process. More specifically:

1. The code you added in VideoCaptureHost has to call into MediaStreamManager
directly anyway. In other words, VideoCaptureHost doesn't seem to need to be
involved in propagating this signal, but MediaStreamManager does.

2. The MediaObserver, which you use to propagate the secure link signal to the
embedder (i.e., the code in src/chrome/...), is something only
MediaStreamManager knows about.

3. The secure link state is "control/permissions metadata management," and not
related to video frame delivery. MediaStreamManager seems to be a more
appropriate handler for this information.

It should be relatively simple to migrate the IPC messaging in this change:

1. Just rename VideoCaptureHostMsg_SetCapturingLinkSecured to
MediaStreamHostMsg_SetCapturingLinkSecured, and put it in
media_stream_messages.h; and the use of MediaStreamType+session_id would be
reasonable here.

2. Have MediaStreamDispatcherHost handle the IPC message (browser-side) rather
than VideoCaptureHost.

3. LocalVideoCapturerSource (in the render process) can send the IPC to the
browser process via MediaStreamDispatcher: First, you'll need to plumb-in access
to the RenderFrameImpl instance from the LocalVideoCapturerSource by passing
render_frame() in the constructor call from here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
LocalVideoCapturerSource should extend RenderFrameObserver to always be sure the
pointer to the RenderFrameImpl is valid before trying to call: if
(render_frame()) render_frame()->GetMediaStreamDispatcher()->Send(new
MediaStreamHostMsg_...) If this is a bit confusing, feel free to stop by my
desk, and I can explain how RenderFrameImpl works in more detail.

https://codereview.chromium.org/1873293002/diff/220001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.cc (right):

https://codereview.chromium.org/1873293002/diff/220001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:23: is_extension_trusted,
false};
I'm concerned about defaulting this to "false" (which means not secure). It
seems there could be a race condition where IsInsecureCapturingInProgress()
would return true during the brief period of time before any consumers (sinks)
notify the browser process (via VideoCaptureHost IPCs) that the link is secure.
From the user's perpective, that means the video could black-out for several
seconds for no good reason when tab capture is first started.

Logically, if there are no consumers/sinks of the captured video, then a link
should be considered secure, right?

[xjz, 2016-05-04 01:28:17]

Patchset #5 (id:240001) has been deleted

[xjz, 2016-05-04 17:46:49]

Patchset #5 (id:260001) has been deleted

[xjz, 2016-05-04 18:23:49]

Addressed comments. PTAL.

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/m...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1873293002/diff/180001/chrome/browser/media/m...
chrome/browser/media/media_capture_devices_dispatcher.cc:69: inline
DesktopCaptureAccessHandler* ToDesktopCaptureAccessHandler(
On 2016/05/02 21:02:45, miu wrote:
> On 2016/04/29 00:11:42, xjz wrote:
> > On 2016/04/26 01:25:13, miu wrote:
> > > I think you can delete this function now.
> > 
> > This is used by |IsDesktopCaptureInProgress()| for now, and will be removed
> > after output protection switches to use the new API for checking.
> 
> Yes, but can IsDesktopCaptureInProgress() just call
ToCaptureAccessHandlerBase()
> instead of this function?

Done. Actually now changed the OutputProtectionDelegate to call the new API and
removed all unused codes.:)

https://codereview.chromium.org/1873293002/diff/180001/content/common/media/v...
File content/common/media/video_capture_messages.h (right):

https://codereview.chromium.org/1873293002/diff/180001/content/common/media/v...
content/common/media/video_capture_messages.h:122:
IPC_MESSAGE_CONTROL3(VideoCaptureHostMsg_SetCapturingLinkSecured,
On 2016/05/02 21:02:45, miu wrote:
> On 2016/04/29 00:11:42, xjz wrote:
> > On 2016/04/26 01:25:13, miu wrote:
> > > Hmm...It seems like this should be a MediaStreamHostMsg instead (in
> > > content/common/media/media_stream_messages.h).  Having VideoCaptureHost
> worry
> > > about MediaStreamTypes feels like an abstraction violation.
> > 
> > The reason to use MediaStreamType here is that the |session_id| is unique
only
> > for the same MediaStreamType. Now changed to use device id, which should
also
> > work. 
> 
> I should have explained more clearly before: What I'm concerned about is the
> encapsulation of responsibilities/functionality here. MediaStreamManager is
> responsible for managing control and permissions for the streams (and
> bubbling-up metadata/state to the embedder), whereas VideoCaptureHost is only
> supposed to be responsible for delivering video frames across
browser-->renderer
> process. More specifically:
> 
> 1. The code you added in VideoCaptureHost has to call into MediaStreamManager
> directly anyway. In other words, VideoCaptureHost doesn't seem to need to be
> involved in propagating this signal, but MediaStreamManager does.
> 
> 2. The MediaObserver, which you use to propagate the secure link signal to the
> embedder (i.e., the code in src/chrome/...), is something only
> MediaStreamManager knows about.
> 
> 3. The secure link state is "control/permissions metadata management," and not
> related to video frame delivery. MediaStreamManager seems to be a more
> appropriate handler for this information.
> 
> It should be relatively simple to migrate the IPC messaging in this change:
> 
> 1. Just rename VideoCaptureHostMsg_SetCapturingLinkSecured to
> MediaStreamHostMsg_SetCapturingLinkSecured, and put it in
> media_stream_messages.h; and the use of MediaStreamType+session_id would be
> reasonable here.
> 
> 2. Have MediaStreamDispatcherHost handle the IPC message (browser-side) rather
> than VideoCaptureHost.
> 
> 3. LocalVideoCapturerSource (in the render process) can send the IPC to the
> browser process via MediaStreamDispatcher: First, you'll need to plumb-in
access
> to the RenderFrameImpl instance from the LocalVideoCapturerSource by passing
> render_frame() in the constructor call from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> LocalVideoCapturerSource should extend RenderFrameObserver to always be sure
the
> pointer to the RenderFrameImpl is valid before trying to call: if
> (render_frame()) render_frame()->GetMediaStreamDispatcher()->Send(new
> MediaStreamHostMsg_...) If this is a bit confusing, feel free to stop by my
> desk, and I can explain how RenderFrameImpl works in more detail.

Thanks for the clear explanation! Changed the IPC message to MediaStreamHostMsg.

https://codereview.chromium.org/1873293002/diff/220001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.cc (right):

https://codereview.chromium.org/1873293002/diff/220001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:23: is_extension_trusted,
false};
On 2016/05/02 21:02:45, miu wrote:
> I'm concerned about defaulting this to "false" (which means not secure). It
> seems there could be a race condition where IsInsecureCapturingInProgress()
> would return true during the brief period of time before any consumers (sinks)
> notify the browser process (via VideoCaptureHost IPCs) that the link is
secure.
> From the user's perpective, that means the video could black-out for several
> seconds for no good reason when tab capture is first started.
> 
> Logically, if there are no consumers/sinks of the captured video, then a link
> should be considered secure, right?

Done. Changed the default to true.

[miu, 2016-05-06 18:56:22]

Excellent work! This is a very involved change! :)

Almost there, just a few small things:

https://codereview.chromium.org/1873293002/diff/280001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.cc (right):

https://codereview.chromium.org/1873293002/diff/280001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:72: VLOG(2) << "Add new
session while UpdateMediaRequestState"
Consider making all these DVLOGs instead of VLOGs. (Otherwise, they will be
bloating the release binaries.) However, if you think you'll need them to
sanity-check things "in-the-wild" over the next few months, it would be okay to
keep them as VLOG (with the intention of changing them to DVLOG later).

https://codereview.chromium.org/1873293002/diff/280001/content/renderer/media...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/1873293002/diff/280001/content/renderer/media...
content/renderer/media/media_stream_video_capturer_source.cc:418: if
(render_frame_) {
If this class extends RenderFrameObserver, all the code in this method becomes:

  Send(new MediaStreamHostMsg_...);

(Since RenderFrameObserver provides a Send() method that null-checks the
RenderFrame for you.)

https://codereview.chromium.org/1873293002/diff/280001/content/renderer/media...
File content/renderer/media/media_stream_video_capturer_source.h (right):

https://codereview.chromium.org/1873293002/diff/280001/content/renderer/media...
content/renderer/media/media_stream_video_capturer_source.h:67: RenderFrame*
const render_frame_;  // Not owned.
For safety, instead of having this render_frame_ member, you should have this
class extend content::RenderFrameObserver. That way,
MediaStreamVideoCapturerSource will be notified whenever the RenderFrame is
destroyed.

[xjz, 2016-05-06 22:01:05]

Description was changed from

==========
Report if video capturing meets output protection requirement.
Report the video capturing is secure if all connected video sinks are
reported secure and the extension is white listed for casting.

BUG=601859
==========

to

==========
Report if there is any ongoing insecured capturing. The capturing is
deemed secure if all connected video sinks are reported secure and 
the extension is trusted.

BUG=601859
==========

[xjz, 2016-05-06 22:08:46]

Thanks Yuri! Addressed your comments. PTAL.

https://codereview.chromium.org/1873293002/diff/280001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.cc (right):

https://codereview.chromium.org/1873293002/diff/280001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:72: VLOG(2) << "Add new
session while UpdateMediaRequestState"
On 2016/05/06 18:56:22, miu wrote:
> Consider making all these DVLOGs instead of VLOGs. (Otherwise, they will be
> bloating the release binaries.) However, if you think you'll need them to
> sanity-check things "in-the-wild" over the next few months, it would be okay
to
> keep them as VLOG (with the intention of changing them to DVLOG later).

Done. Changed to DVLOGs. :)

https://codereview.chromium.org/1873293002/diff/280001/content/renderer/media...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/1873293002/diff/280001/content/renderer/media...
content/renderer/media/media_stream_video_capturer_source.cc:418: if
(render_frame_) {
On 2016/05/06 18:56:22, miu wrote:
> If this class extends RenderFrameObserver, all the code in this method
becomes:
> 
>   Send(new MediaStreamHostMsg_...);
> 
> (Since RenderFrameObserver provides a Send() method that null-checks the
> RenderFrame for you.)

Done.

https://codereview.chromium.org/1873293002/diff/280001/content/renderer/media...
File content/renderer/media/media_stream_video_capturer_source.h (right):

https://codereview.chromium.org/1873293002/diff/280001/content/renderer/media...
content/renderer/media/media_stream_video_capturer_source.h:67: RenderFrame*
const render_frame_;  // Not owned.
On 2016/05/06 18:56:22, miu wrote:
> For safety, instead of having this render_frame_ member, you should have this
> class extend content::RenderFrameObserver. That way,
> MediaStreamVideoCapturerSource will be notified whenever the RenderFrame is
> destroyed.

Done.

[miu, 2016-05-06 23:15:37]

lgtm

[xjz, 2016-05-06 23:28:46]

xjz@chromium.org changed reviewers:
+ nasko@chromium.org, oshima@chromium.org

[xjz, 2016-05-06 23:28:46]

Thanks Yuri!
oshima@: PTAL changes on
chrome/browser/chromeos/display/output_protection_delegate.cc

nasko@: PTAL changes on content/common/media/media_stream_messages.h

[xjz, 2016-05-06 23:47:23]

nasko@: Also needs your approval on content/browser/* and 
content/public/*.

[oshima, 2016-05-07 14:41:50]

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/display/output_protection_delegate.cc (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/display/output_protection_delegate.cc:126:
content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);
do you still need this?

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.cc (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:36:
std::list<CaptureAccessHandlerBase::Session>::iterator it =
optional nit: you can use

auto it =

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:46: for (auto it =
sessions_.begin(); it != sessions_.end(); ++it) {
find_if + lamba?

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.h (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:10: #include
"base/callback.h"
you do need this?

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:64: };
move the definition to .cc file.

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:79: std::list<Session>
sessions_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/t...
File chrome/browser/media/tab_capture_access_handler.h (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/t...
chrome/browser/media/tab_capture_access_handler.h:9: #include
"chrome/browser/media/media_access_handler.h"
is this still necessary?

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/t...
chrome/browser/media/tab_capture_access_handler.h:11: class
MediaStreamCaptureIndicator;
and this?

https://codereview.chromium.org/1873293002/diff/300001/content/renderer/media...
File content/renderer/media/secure_display_link_tracker.h (right):

https://codereview.chromium.org/1873293002/diff/300001/content/renderer/media...
content/renderer/media/secure_display_link_tracker.h:21: bool
is_capturing_secure() { return insecure_links_.empty(); }
const

https://codereview.chromium.org/1873293002/diff/300001/content/renderer/media...
content/renderer/media/secure_display_link_tracker.h:25: std::vector<T*>
insecure_links_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1873293002/diff/300001/content/renderer/media...
content/renderer/media/secure_display_link_tracker.h:36: for (auto it =
insecure_links_.begin(); it != insecure_links_.end(); ++it) {
std::find ?

[xhwang, 2016-05-09 18:29:48]

chrome/browser/chromeos/display/output_protection_delegate.cc and the new API
LGTM. You need other owners to review detailed changes. Thanks!!!

https://chromiumcodereview.appspot.com/1873293002/diff/300001/chrome/browser/...
File chrome/browser/chromeos/display/output_protection_delegate.cc (right):

https://chromiumcodereview.appspot.com/1873293002/diff/300001/chrome/browser/...
chrome/browser/chromeos/display/output_protection_delegate.cc:126:
content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);
On 2016/05/07 14:41:49, oshima wrote:
> do you still need this?

I don't really know when this could happen. But we have similar checks above in
QueryStatus() and below in OnWindowHierarchyChanged(). So I feel we should keep
this as is and investigate whether we need these in a separate CL.

[miu, 2016-05-09 20:30:51]

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.h (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:64: };
On 2016/05/07 14:41:49, oshima wrote:
> move the definition to .cc file.

This can't be done: struct Session is needed for the std::list<Session> type.

[nasko, 2016-05-09 21:33:30]

https://codereview.chromium.org/1873293002/diff/300001/content/common/media/m...
File content/common/media/media_stream_messages.h (right):

https://codereview.chromium.org/1873293002/diff/300001/content/common/media/m...
content/common/media/media_stream_messages.h:161: bool /* is_secure */)
This seems incorrect from threat modelling perspective. We should not be
trusting the renderer process to tell us secure vs non-secure state. Is there no
way for the browser process to know this information?

[oshima, 2016-05-09 22:24:48]

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.h (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:64: };
On 2016/05/09 20:30:50, miu wrote:
> On 2016/05/07 14:41:49, oshima wrote:
> > move the definition to .cc file.
> 
> This can't be done: struct Session is needed for the std::list<Session> type.

But you need it only in .cc right?

you should be able to use std::list with forward decl struct Session.

see 
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts

[miu, 2016-05-09 23:09:31]

https://codereview.chromium.org/1873293002/diff/300001/content/common/media/m...
File content/common/media/media_stream_messages.h (right):

https://codereview.chromium.org/1873293002/diff/300001/content/common/media/m...
content/common/media/media_stream_messages.h:161: bool /* is_secure */)
On 2016/05/09 21:33:29, nasko (slow) wrote:
> This seems incorrect from threat modelling perspective. We should not be
> trusting the renderer process to tell us secure vs non-secure state. Is there
no
> way for the browser process to know this information?

In the general case, you'd be absolutely correct. However, this is being used
for a more specialized purpose: At a high level, we are trying to determine
whether protected video content is being screen-captured (e.g., via the
chrome.tabCapture API) and then the resulting video stream routed by an
extension to insecure links/destinations.

The browser process only trusts the "is_secure" value in this IPC message if
it's coming from a trusted, whitelisted extension. Extensions run in separate
render processes. So it shouldn't be possible, for example, for a user's visit
to a malicious web page to compromise a render process running a trusted
extension to make it report falsehood in this IPC message.

[miu, 2016-05-09 23:10:52]

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.h (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:64: };
On 2016/05/09 22:24:48, oshima wrote:
> On 2016/05/09 20:30:50, miu wrote:
> > On 2016/05/07 14:41:49, oshima wrote:
> > > move the definition to .cc file.
> > 
> > This can't be done: struct Session is needed for the std::list<Session>
type.
> 
> But you need it only in .cc right?
> 
> you should be able to use std::list with forward decl struct Session.
> 
> see 
> https://www.chromium.org/developers/coding-style/cpp-dos-and-donts

Ah, yes! It would work because the template instantiation for code referencing
"struct Session" would all be in the .cc file.

[xjz, 2016-05-10 00:28:25]

PTAL. Addressed oshima's comments in PS# 7.

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/display/output_protection_delegate.cc (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/display/output_protection_delegate.cc:126:
content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);
On 2016/05/07 14:41:49, oshima wrote:
> do you still need this?

It is not needed for this purpose. Added a TODO comment based on xhwang's
comment.

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.cc (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:36:
std::list<CaptureAccessHandlerBase::Session>::iterator it =
On 2016/05/07 14:41:49, oshima wrote:
> optional nit: you can use
> 
> auto it =

Done.

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.cc:46: for (auto it =
sessions_.begin(); it != sessions_.end(); ++it) {
On 2016/05/07 14:41:49, oshima wrote:
> find_if + lamba?

Done.

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
File chrome/browser/media/capture_access_handler_base.h (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:10: #include
"base/callback.h"
On 2016/05/07 14:41:49, oshima wrote:
> you do need this?

Removed.

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:64: };
On 2016/05/09 22:24:48, oshima wrote:
> On 2016/05/09 20:30:50, miu wrote:
> > On 2016/05/07 14:41:49, oshima wrote:
> > > move the definition to .cc file.
> > 
> > This can't be done: struct Session is needed for the std::list<Session>
type.
> 
> But you need it only in .cc right?
> 
> you should be able to use std::list with forward decl struct Session.
> 
> see 
> https://www.chromium.org/developers/coding-style/cpp-dos-and-donts

Done.

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/c...
chrome/browser/media/capture_access_handler_base.h:79: std::list<Session>
sessions_;
On 2016/05/07 14:41:49, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/t...
File chrome/browser/media/tab_capture_access_handler.h (right):

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/t...
chrome/browser/media/tab_capture_access_handler.h:9: #include
"chrome/browser/media/media_access_handler.h"
On 2016/05/07 14:41:49, oshima wrote:
> is this still necessary?

Removed.

https://codereview.chromium.org/1873293002/diff/300001/chrome/browser/media/t...
chrome/browser/media/tab_capture_access_handler.h:11: class
MediaStreamCaptureIndicator;
On 2016/05/07 14:41:50, oshima wrote:
> and this?

Removed.

https://codereview.chromium.org/1873293002/diff/300001/content/common/media/m...
File content/common/media/media_stream_messages.h (right):

https://codereview.chromium.org/1873293002/diff/300001/content/common/media/m...
content/common/media/media_stream_messages.h:161: bool /* is_secure */)
On 2016/05/09 23:09:31, miu wrote:
> On 2016/05/09 21:33:29, nasko (slow) wrote:
> > This seems incorrect from threat modelling perspective. We should not be
> > trusting the renderer process to tell us secure vs non-secure state. Is
there
> no
> > way for the browser process to know this information?
> 
> In the general case, you'd be absolutely correct. However, this is being used
> for a more specialized purpose: At a high level, we are trying to determine
> whether protected video content is being screen-captured (e.g., via the
> chrome.tabCapture API) and then the resulting video stream routed by an
> extension to insecure links/destinations.
> 
> The browser process only trusts the "is_secure" value in this IPC message if
> it's coming from a trusted, whitelisted extension. Extensions run in separate
> render processes. So it shouldn't be possible, for example, for a user's visit
> to a malicious web page to compromise a render process running a trusted
> extension to make it report falsehood in this IPC message.

Yuri: Thanks for clarifying this!

https://codereview.chromium.org/1873293002/diff/300001/content/renderer/media...
File content/renderer/media/secure_display_link_tracker.h (right):

https://codereview.chromium.org/1873293002/diff/300001/content/renderer/media...
content/renderer/media/secure_display_link_tracker.h:21: bool
is_capturing_secure() { return insecure_links_.empty(); }
On 2016/05/07 14:41:50, oshima wrote:
> const

Done.

https://codereview.chromium.org/1873293002/diff/300001/content/renderer/media...
content/renderer/media/secure_display_link_tracker.h:25: std::vector<T*>
insecure_links_;
On 2016/05/07 14:41:50, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1873293002/diff/300001/content/renderer/media...
content/renderer/media/secure_display_link_tracker.h:36: for (auto it =
insecure_links_.begin(); it != insecure_links_.end(); ++it) {
On 2016/05/07 14:41:50, oshima wrote:
> std::find ?

Done.

[oshima, 2016-05-10 02:13:45]

lgtm with optional nits:

https://codereview.chromium.org/1873293002/diff/320001/content/renderer/media...
File content/renderer/media/secure_display_link_tracker.h (right):

https://codereview.chromium.org/1873293002/diff/320001/content/renderer/media...
content/renderer/media/secure_display_link_tracker.h:31: void
SecureDisplayLinkTracker<T>::Add(T* link, bool is_link_secure) {
optional nit: maybe

DCHECK(std::find(, link) == insecure_links_.end())

?

https://codereview.chromium.org/1873293002/diff/320001/content/renderer/media...
content/renderer/media/secure_display_link_tracker.h:52:
insecure_links_.push_back(link);
optional nit: Add(link, is_link_secure)

?

[nasko, 2016-05-11 16:36:33]

On 2016/05/09 23:09:31, miu wrote:
>
https://codereview.chromium.org/1873293002/diff/300001/content/common/media/m...
> File content/common/media/media_stream_messages.h (right):
> 
>
https://codereview.chromium.org/1873293002/diff/300001/content/common/media/m...
> content/common/media/media_stream_messages.h:161: bool /* is_secure */)
> On 2016/05/09 21:33:29, nasko (slow) wrote:
> > This seems incorrect from threat modelling perspective. We should not be
> > trusting the renderer process to tell us secure vs non-secure state. Is
there
> no
> > way for the browser process to know this information?
> 
> In the general case, you'd be absolutely correct. However, this is being used
> for a more specialized purpose: At a high level, we are trying to determine
> whether protected video content is being screen-captured (e.g., via the
> chrome.tabCapture API) and then the resulting video stream routed by an
> extension to insecure links/destinations.
> 
> The browser process only trusts the "is_secure" value in this IPC message if
> it's coming from a trusted, whitelisted extension. Extensions run in separate
> render processes. So it shouldn't be possible, for example, for a user's visit
> to a malicious web page to compromise a render process running a trusted
> extension to make it report falsehood in this IPC message.

It will be useful to have this explanation along with the IPC message, so it
is easier for future readers of the code to know why this is "safe".
I am still worried that people might exploit renderer processes, even through
extensions, if they want to fool the browser process in letting the stream
go through and be captured.

[nasko, 2016-05-11 16:42:56]

IPC code LGTM, with the remark that I still think it is better to manage the
"secure" bit in the browser process.

https://codereview.chromium.org/1873293002/diff/320001/content/browser/render...
File content/browser/renderer_host/media/media_stream_dispatcher_host.h (right):

https://codereview.chromium.org/1873293002/diff/320001/content/browser/render...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:110: // IPC
message: Set if the video capturing is secure.
nit: IPC message handler

https://codereview.chromium.org/1873293002/diff/320001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/1873293002/diff/320001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:258: void
SetCapturingLinkSecured(int render_process_id,
nit: Add a comment to be consistent with the rest of the file.

[xjz, 2016-05-11 18:07:24]

Thanks everyone for reviewing this! Addressed oshima and nasko's comments in PS#
8.

https://codereview.chromium.org/1873293002/diff/320001/content/browser/render...
File content/browser/renderer_host/media/media_stream_dispatcher_host.h (right):

https://codereview.chromium.org/1873293002/diff/320001/content/browser/render...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:110: // IPC
message: Set if the video capturing is secure.
On 2016/05/11 16:42:56, nasko (Out until May 23rd) wrote:
> nit: IPC message handler

Done.

https://codereview.chromium.org/1873293002/diff/320001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/1873293002/diff/320001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.h:258: void
SetCapturingLinkSecured(int render_process_id,
On 2016/05/11 16:42:56, nasko (Out until May 23rd) wrote:
> nit: Add a comment to be consistent with the rest of the file.

Done.

https://codereview.chromium.org/1873293002/diff/320001/content/renderer/media...
File content/renderer/media/secure_display_link_tracker.h (right):

https://codereview.chromium.org/1873293002/diff/320001/content/renderer/media...
content/renderer/media/secure_display_link_tracker.h:31: void
SecureDisplayLinkTracker<T>::Add(T* link, bool is_link_secure) {
On 2016/05/10 02:13:44, oshima wrote:
> optional nit: maybe
> 
> DCHECK(std::find(, link) == insecure_links_.end())
> 
> ?

Done.

https://codereview.chromium.org/1873293002/diff/320001/content/renderer/media...
content/renderer/media/secure_display_link_tracker.h:52:
insecure_links_.push_back(link);
On 2016/05/10 02:13:44, oshima wrote:
> optional nit: Add(link, is_link_secure)
> 
> ?

Done.

[xjz, 2016-05-11 18:07:34]

The CQ bit was checked by xjz@chromium.org

[xjz, 2016-05-11 18:07:34]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
miu@chromium.org, oshima@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1873293002/#ps340001
(title: "Addressed oshima and nasko's comments, and rebased.")

[commit-bot: I haz the power, 2016-05-11 18:08:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1873293002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1873293002/340001

[commit-bot: I haz the power, 2016-05-11 18:35:40]

Description was changed from

==========
Report if there is any ongoing insecured capturing. The capturing is
deemed secure if all connected video sinks are reported secure and 
the extension is trusted.

BUG=601859
==========

to

==========
Report if there is any ongoing insecured capturing. The capturing is
deemed secure if all connected video sinks are reported secure and 
the extension is trusted.

BUG=601859
==========

[commit-bot: I haz the power, 2016-05-11 18:35:44]

Committed patchset #8 (id:340001)

[commit-bot: I haz the power, 2016-05-11 18:38:46]

Description was changed from

==========
Report if there is any ongoing insecured capturing. The capturing is
deemed secure if all connected video sinks are reported secure and 
the extension is trusted.

BUG=601859
==========

to

==========
Report if there is any ongoing insecured capturing. The capturing is
deemed secure if all connected video sinks are reported secure and
the extension is trusted.

BUG=601859

Committed: https://crrev.com/f6f63a8db446f2114984ca8e48c28097155bb0e3
Cr-Commit-Position: refs/heads/master@{#393005}
==========

[commit-bot: I haz the power, 2016-05-11 18:38:48]

Patchset 8 (id:??) landed as
https://crrev.com/f6f63a8db446f2114984ca8e48c28097155bb0e3
Cr-Commit-Position: refs/heads/master@{#393005}