Media Session: passing metadata from renderer/ to browser/.

Media Session: passing metadata from renderer/ to browser/.

This is implementing the IPC logic but also adds the base classes that
are required to use MediaMetadata in content/.

This CL is written on top of:
https://codereview.chromium.org/1441883003

BUG=497735
Committed: https://crrev.com/08842a7edc1618ba5c86faff3977b5ca03637bdf
Cr-Commit-Position: refs/heads/master@{#371510}

[mlamouri (slow - plz ping), 2015-12-11 09:36:36]

Description was changed from

==========
[WIP] Media Session: passing metadata from renderer/ to browser/.

This is implementing the IPC logic but also adds the base classes that
are required to use MediaMetadata in content/.

This CL is written on top of:
https://codereview.chromium.org/1441883003

BUG=497735
==========

to

==========
Media Session: passing metadata from renderer/ to browser/.

This is implementing the IPC logic but also adds the base classes that
are required to use MediaMetadata in content/.

This CL is written on top of:
https://codereview.chromium.org/1441883003

BUG=497735
==========

[mlamouri (slow - plz ping), 2015-12-11 09:38:48]

mlamouri@chromium.org changed reviewers:
+ avayvod@chromium.org, davve@opera.com, jochen@chromium.org,
mkwst@chromium.org, philipj@opera.com

[mlamouri (slow - plz ping), 2015-12-11 09:38:48]

Can you PTAL at:
- jochen@:
  content/ OWNERS stamp
- mkwst@:
  content/common/media/*messages*.h
- davve@, philipj@ and avayvod@:
 Media Session stuff

Thanks :)

[DaleCurtis, 2015-12-11 17:16:01]

It also seems like the session managers should move up out of the android
directory, but that's not necessary at this time.

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:301:
IPC_MESSAGE_FORWARD(MediaSessionHostMsg_Activate,
nit: Can you move these three messages into a new
OnMediaSessionMessageReceived() function? As time goes on we want to remove all
the MediaPlayer messages, likely by transitioning them to Mojo.

This will also make it easier to share this code on desktop.

[Mike West, 2015-12-13 07:38:07]

IPC LGTM, thanks for adding in the sanity checks. :)

[jochen (gone - plz use gerrit), 2015-12-14 12:38:30]

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/a...
File content/browser/media/android/browser_media_session_manager.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_session_manager.cc:42: 
kill the renderer if insecure_metadata != metadata?

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
File content/public/common/media_metadata.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.cc:11: MediaMetadata::~MediaMetadata() =
default;
that's more code than {}...

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
File content/public/common/media_metadata.h (right):

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.h:5: #ifndef
CONTENT_PUBLIC_COMMON_MEDIA_METADATA_H_
we is this in public, if you only use it from content?

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.h:16: MediaMetadata();
can you please add ctor args for all struct fields?

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.h:36: static const size_t
kMaxIPCStringLength = 4 * 1024;
can you move the definition to the cc file please?

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.cc:68:
ipc_metadata.title = base::NullableString16(
i'm surprised this has to be done manually, and you don't have to just use the
ipc macros to declare the struct fields :-/

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.h (right):

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.h:37: void
SetMetadata(int session_id, const content::MediaMetadata& metadata);
you're already in namespace content here

[philipj_slow, 2015-12-14 14:09:25]

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/a...
File content/browser/media/android/browser_media_session_manager.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_session_manager.cc:32: MediaMetadata
metadata = insecure_metadata;
Can you initialize it to an empty struct, so that when things are added to
MediaMetadata, it's not possible to forget handling them below?

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
File content/public/common/media_metadata.h (right):

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.h:24: base::NullableString16 title;
Rather than making every nullable, could we not use the empty string? The
web-exposed MediaMetadata does not have nullable members, so if we have just the
empty string to represent "missing" internally, that seems nice and simple.

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.cc:66: // Apply
some security rules on the MediaMetadata before sending over IPC.
Does it help to apply this on the renderer side as well? If the renderer is
compromised, then presumably these checks won't stop it from being naughty?
(Pardon my ignorance if this is how it's always done, for some reason.)

[mlamouri (slow - plz ping), 2016-01-05 16:01:17]

PTAL.

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/a...
File content/browser/media/android/browser_media_session_manager.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_session_manager.cc:32: MediaMetadata
metadata = insecure_metadata;
On 2015/12/14 at 14:09:24, philipj_OOO_til_Jan_11 wrote:
> Can you initialize it to an empty struct, so that when things are added to
MediaMetadata, it's not possible to forget handling them below?

Done.

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/a...
content/browser/media/android/browser_media_session_manager.cc:42: 
On 2015/12/14 at 12:38:30, jochen (OOO) wrote:
> kill the renderer if insecure_metadata != metadata?

Done.

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:301:
IPC_MESSAGE_FORWARD(MediaSessionHostMsg_Activate,
On 2015/12/11 at 17:16:00, DaleCurtis_OOO_Until_Jan_5 wrote:
> nit: Can you move these three messages into a new
OnMediaSessionMessageReceived() function? As time goes on we want to remove all
the MediaPlayer messages, likely by transitioning them to Mojo.
> 
> This will also make it easier to share this code on desktop.

Done.

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
File content/public/common/media_metadata.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.cc:11: MediaMetadata::~MediaMetadata() =
default;
On 2015/12/14 at 12:38:30, jochen (OOO) wrote:
> that's more code than {}...

C++ standard body probably takes feedback :)

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
File content/public/common/media_metadata.h (right):

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.h:5: #ifndef
CONTENT_PUBLIC_COMMON_MEDIA_METADATA_H_
On 2015/12/14 at 12:38:30, jochen (OOO) wrote:
> we is this in public, if you only use it from content?

This is going to be used by content embedders for UI purposes.

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.h:16: MediaMetadata();
On 2015/12/14 at 12:38:30, jochen (OOO) wrote:
> can you please add ctor args for all struct fields?

I think the issue of such a ctor is that it would be easy to swap strings by
mistake. What's the intent behind the request?

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.h:24: base::NullableString16 title;
On 2015/12/14 at 14:09:24, philipj_OOO_til_Jan_11 wrote:
> Rather than making every nullable, could we not use the empty string? The
web-exposed MediaMetadata does not have nullable members, so if we have just the
empty string to represent "missing" internally, that seems nice and simple.

Null will be used here to know whether a string was set or not. If we use empty
string, we wouldn't be able to know if a website set the author to "" or didn't
specify one. Content embedders might want to behave differently depending on
that.

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.h:36: static const size_t
kMaxIPCStringLength = 4 * 1024;
On 2015/12/14 at 12:38:30, jochen (OOO) wrote:
> can you move the definition to the cc file please?

Sure. I thought that given that we are now allowed to initialize non-static
members in headers it should be fine for static ones.

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.cc:66: // Apply
some security rules on the MediaMetadata before sending over IPC.
On 2015/12/14 at 14:09:24, philipj_OOO_til_Jan_11 wrote:
> Does it help to apply this on the renderer side as well? If the renderer is
compromised, then presumably these checks won't stop it from being naughty?
(Pardon my ignorance if this is how it's always done, for some reason.)

This is mostly sanity check: a string more than 4000 characters long is whether
a bug or an attempt to mess with the browser process. A process sending a string
with 10k characters will not be killed, the string will only be shortened.

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.cc:68:
ipc_metadata.title = base::NullableString16(
On 2015/12/14 at 12:38:30, jochen (OOO) wrote:
> i'm surprised this has to be done manually, and you don't have to just use the
ipc macros to declare the struct fields :-/

AFAICT, it isn't done very often.

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.h (right):

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.h:37: void
SetMetadata(int session_id, const content::MediaMetadata& metadata);
On 2015/12/14 at 12:38:30, jochen (OOO) wrote:
> you're already in namespace content here

Done.

[mlamouri (slow - plz ping), 2016-01-18 12:22:53]

review ping

[mlamouri (slow - plz ping), 2016-01-18 12:22:53]

review ping

[philipj_slow, 2016-01-18 12:31:43]

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
File content/public/common/media_metadata.h (right):

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.h:24: base::NullableString16 title;
On 2016/01/05 16:01:17, Mounir Lamouri wrote:
> On 2015/12/14 at 14:09:24, philipj_OOO_til_Jan_11 wrote:
> > Rather than making every nullable, could we not use the empty string? The
> web-exposed MediaMetadata does not have nullable members, so if we have just
the
> empty string to represent "missing" internally, that seems nice and simple.
> 
> Null will be used here to know whether a string was set or not. If we use
empty
> string, we wouldn't be able to know if a website set the author to "" or
didn't
> specify one. Content embedders might want to behave differently depending on
> that.

But the MediaMetadata interface assumes that "" is not a value that's meaningful
to set explicitly, as it's the default value for all of the dictionary members.
We'd have to change the IDL or use custom bindings to even know in
MediaMetadata.cpp whether the "" was explicitly passed or the default. If
content embedders did behave differently for these cases, the difference
couldn't be explained in terms of the spec.

Can we either stick with empty strings, and change the spec and implementation
if we find a reason to distinguish between explicit and default ""?

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.cc:66: // Apply
some security rules on the MediaMetadata before sending over IPC.
On 2016/01/05 16:01:17, Mounir Lamouri wrote:
> On 2015/12/14 at 14:09:24, philipj_OOO_til_Jan_11 wrote:
> > Does it help to apply this on the renderer side as well? If the renderer is
> compromised, then presumably these checks won't stop it from being naughty?
> (Pardon my ignorance if this is how it's always done, for some reason.)
> 
> This is mostly sanity check: a string more than 4000 characters long is
whether
> a bug or an attempt to mess with the browser process. A process sending a
string
> with 10k characters will not be killed, the string will only be shortened.

If it's only a sanity check, maybe s/security rules/sanity check/ in the comment
so that it doesn't look like this code matters for security? Still not sure if
there's any harm in just removing the checks, would like to learn :)

[mlamouri (slow - plz ping), 2016-01-19 14:19:22]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-19 14:19:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1515623002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1515623002/80001

[mlamouri (slow - plz ping), 2016-01-19 14:20:09]

Comments applied. PTAL.

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
File content/public/common/media_metadata.h (right):

https://codereview.chromium.org/1515623002/diff/40001/content/public/common/m...
content/public/common/media_metadata.h:24: base::NullableString16 title;
On 2016/01/18 at 12:31:43, philipj wrote:
> On 2016/01/05 16:01:17, Mounir Lamouri wrote:
> > On 2015/12/14 at 14:09:24, philipj_OOO_til_Jan_11 wrote:
> > > Rather than making every nullable, could we not use the empty string? The
> > web-exposed MediaMetadata does not have nullable members, so if we have just
the
> > empty string to represent "missing" internally, that seems nice and simple.
> > 
> > Null will be used here to know whether a string was set or not. If we use
empty
> > string, we wouldn't be able to know if a website set the author to "" or
didn't
> > specify one. Content embedders might want to behave differently depending on
> > that.
> 
> But the MediaMetadata interface assumes that "" is not a value that's
meaningful to set explicitly, as it's the default value for all of the
dictionary members. We'd have to change the IDL or use custom bindings to even
know in MediaMetadata.cpp whether the "" was explicitly passed or the default.
If content embedders did behave differently for these cases, the difference
couldn't be explained in terms of the spec.
> 
> Can we either stick with empty strings, and change the spec and implementation
if we find a reason to distinguish between explicit and default ""?

Ok. Done.

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/1515623002/diff/40001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.cc:66: // Apply
some security rules on the MediaMetadata before sending over IPC.
On 2016/01/18 at 12:31:43, philipj wrote:
> On 2016/01/05 16:01:17, Mounir Lamouri wrote:
> > On 2015/12/14 at 14:09:24, philipj_OOO_til_Jan_11 wrote:
> > > Does it help to apply this on the renderer side as well? If the renderer
is
> > compromised, then presumably these checks won't stop it from being naughty?
> > (Pardon my ignorance if this is how it's always done, for some reason.)
> > 
> > This is mostly sanity check: a string more than 4000 characters long is
whether
> > a bug or an attempt to mess with the browser process. A process sending a
string
> > with 10k characters will not be killed, the string will only be shortened.
> 
> If it's only a sanity check, maybe s/security rules/sanity check/ in the
comment so that it doesn't look like this code matters for security? Still not
sure if there's any harm in just removing the checks, would like to learn :)

Updated the comment. I was asked to do that a while ago by the security team for
websites provided string sent over IPC. My understanding is that malicious
websites might try to overflow the browser process. Also, long string might help
using exploit. Though, my understanding might be wrong. I'm not part of the
security team for a reason ;)

[commit-bot: I haz the power, 2016-01-19 15:23:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-19 15:23:53]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-01-25 21:26:23]

review ping.

[philipj_slow, 2016-01-26 02:02:30]

lgtm

https://codereview.chromium.org/1515623002/diff/80001/content/browser/media/a...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1515623002/diff/80001/content/browser/media/a...
content/browser/media/android/media_web_contents_observer_android.cc:184:
IPC_MESSAGE_UNHANDLED(handled = false);
Was this added semicolon a typo? The macro already ends with a semicolon.

[mlamouri (slow - plz ping), 2016-01-26 10:48:21]

jochen@, PTAL :)

https://codereview.chromium.org/1515623002/diff/80001/content/browser/media/a...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1515623002/diff/80001/content/browser/media/a...
content/browser/media/android/media_web_contents_observer_android.cc:184:
IPC_MESSAGE_UNHANDLED(handled = false);
On 2016/01/26 at 02:02:30, philipj_UTC7 wrote:
> Was this added semicolon a typo? The macro already ends with a semicolon.

Done.

[jochen (gone - plz use gerrit), 2016-01-26 11:57:20]

lgtm

[mlamouri (slow - plz ping), 2016-01-26 13:38:59]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-01-26 13:38:59]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1515623002/#ps100001
(title: "semicolon")

[commit-bot: I haz the power, 2016-01-26 13:39:30]

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

[commit-bot: I haz the power, 2016-01-26 14:49:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-26 14:50:49]

Description was changed from

==========
Media Session: passing metadata from renderer/ to browser/.

This is implementing the IPC logic but also adds the base classes that
are required to use MediaMetadata in content/.

This CL is written on top of:
https://codereview.chromium.org/1441883003

BUG=497735
==========

to

==========
Media Session: passing metadata from renderer/ to browser/.

This is implementing the IPC logic but also adds the base classes that
are required to use MediaMetadata in content/.

This CL is written on top of:
https://codereview.chromium.org/1441883003

BUG=497735

Committed: https://crrev.com/08842a7edc1618ba5c86faff3977b5ca03637bdf
Cr-Commit-Position: refs/heads/master@{#371510}
==========

[commit-bot: I haz the power, 2016-01-26 14:50:50]

Patchset 6 (id:??) landed as
https://crrev.com/08842a7edc1618ba5c86faff3977b5ca03637bdf
Cr-Commit-Position: refs/heads/master@{#371510}