Adding WebRTC logging filter, handler and handler host.

Adding WebRTC logging filter, handler and handler host.

Updates to and usage of these will be added in coming CLs.


BUG=229829
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194861

[Henrik Grunell, 2013-03-27 14:30:52]

Tommi, can you take a first high-level stab at this?

There's currently only messages for opening a log, notifying opened or failed
back.

[Henrik Grunell, 2013-03-28 10:02:55]

Brett, please look at content/*
Adam, please look at ipc/*

[tommi (sloooow) - chröme, 2013-03-28 11:00:10]

looks pretty good.

https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/webrtc_logging_handler_host.cc (right):

https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/webrtc_logging_handler_host.cc:43: if
(base::SharedMemory::IsHandleValid(shared_memory_.handle()))
{}

Maybe just be a DCHECK for now (until we support multiple OnOpenLog calls if we
need them)?
DCHECK(!base::SharedMemory::IsHandleValid(shared_memory_.handle()));

https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/webrtc_logging_handler_host.h (right):

https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/webrtc_logging_handler_host.h:20: //
BrowserMessageFilter implementation.
make this implementation private (see discussion on chromium-dev for more info)

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
File content/renderer/webrtc_logging_handler_impl.cc (right):

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
content/renderer/webrtc_logging_handler_impl.cc:13: // TODO(grunell): Probably
skip the thread dchecks and make sure here instead
I like the thread dchecks :)

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
File content/renderer/webrtc_logging_handler_impl.h (right):

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
content/renderer/webrtc_logging_handler_impl.h:32: virtual void
OnFilterRemoved();
are these virtual to support mocking? (something else?)

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
File content/renderer/webrtc_logging_message_filter.cc (right):

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
content/renderer/webrtc_logging_message_filter.cc:83: delete message;
log error?

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
File content/renderer/webrtc_logging_message_filter.h (right):

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
content/renderer/webrtc_logging_message_filter.h:28: virtual bool
OnMessageReceived(const IPC::Message& message) OVERRIDE;
make private

[agl, 2013-03-28 14:16:42]

lgtm

[Henrik Grunell, 2013-04-02 10:06:40]

Added Chris as reviewer of:
content/common/webrtc_logging_messages.h
ipc/ipc_message_start.h

Please review these (and any other files).

Also code review fixes.

https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/webrtc_logging_handler_host.cc (right):

https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/webrtc_logging_handler_host.cc:43: if
(base::SharedMemory::IsHandleValid(shared_memory_.handle()))
On 2013/03/28 11:00:10, tommi wrote:
> {}
> 
> Maybe just be a DCHECK for now (until we support multiple OnOpenLog calls if
we
> need them)?
> DCHECK(!base::SharedMemory::IsHandleValid(shared_memory_.handle()));

Yep, sgtm. The handler in the render process can ensure that we only get here
once. Done.

https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/webrtc_logging_handler_host.h (right):

https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/webrtc_logging_handler_host.h:20: //
BrowserMessageFilter implementation.
On 2013/03/28 11:00:10, tommi wrote:
> make this implementation private (see discussion on chromium-dev for more
info)

Done.

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
File content/renderer/webrtc_logging_handler_impl.cc (right):

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
content/renderer/webrtc_logging_handler_impl.cc:13: // TODO(grunell): Probably
skip the thread dchecks and make sure here instead
On 2013/03/28 11:00:10, tommi wrote:
> I like the thread dchecks :)

Me too. But there's also dchecks in the filter. And I think there's were the
border should go. Calls to the filter must be done on the correct thread.

And here, we make sure the filter is called on the right thread. Sounds good?

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
File content/renderer/webrtc_logging_handler_impl.h (right):

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
content/renderer/webrtc_logging_handler_impl.h:32: virtual void
OnFilterRemoved();
On 2013/03/28 11:00:10, tommi wrote:
> are these virtual to support mocking? (something else?)

By mistake :-) Removed.

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
File content/renderer/webrtc_logging_message_filter.cc (right):

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
content/renderer/webrtc_logging_message_filter.cc:83: delete message;
On 2013/03/28 11:00:10, tommi wrote:
> log error?

Sure, sounds good for this filter. Done.

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
File content/renderer/webrtc_logging_message_filter.h (right):

https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
content/renderer/webrtc_logging_message_filter.h:28: virtual bool
OnMessageReceived(const IPC::Message& message) OVERRIDE;
On 2013/03/28 11:00:10, tommi wrote:
> make private

Done.

[Chris Evans, 2013-04-02 21:51:40]

On 2013/04/02 10:06:40, Henrik Grunell wrote:
> Added Chris as reviewer of:
> content/common/webrtc_logging_messages.h
> ipc/ipc_message_start.h
> 
> Please review these (and any other files).
> 
> Also code review fixes.
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
> File content/browser/renderer_host/webrtc_logging_handler_host.cc (right):
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
> content/browser/renderer_host/webrtc_logging_handler_host.cc:43: if
> (base::SharedMemory::IsHandleValid(shared_memory_.handle()))
> On 2013/03/28 11:00:10, tommi wrote:
> > {}
> > 
> > Maybe just be a DCHECK for now (until we support multiple OnOpenLog calls if
> we
> > need them)?
> > DCHECK(!base::SharedMemory::IsHandleValid(shared_memory_.handle()));
> 
> Yep, sgtm. The handler in the render process can ensure that we only get here
> once. Done.
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
> File content/browser/renderer_host/webrtc_logging_handler_host.h (right):
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/browser/renderer_h...
> content/browser/renderer_host/webrtc_logging_handler_host.h:20: //
> BrowserMessageFilter implementation.
> On 2013/03/28 11:00:10, tommi wrote:
> > make this implementation private (see discussion on chromium-dev for more
> info)
> 
> Done.
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
> File content/renderer/webrtc_logging_handler_impl.cc (right):
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
> content/renderer/webrtc_logging_handler_impl.cc:13: // TODO(grunell): Probably
> skip the thread dchecks and make sure here instead
> On 2013/03/28 11:00:10, tommi wrote:
> > I like the thread dchecks :)
> 
> Me too. But there's also dchecks in the filter. And I think there's were the
> border should go. Calls to the filter must be done on the correct thread.
> 
> And here, we make sure the filter is called on the right thread. Sounds good?
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
> File content/renderer/webrtc_logging_handler_impl.h (right):
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
> content/renderer/webrtc_logging_handler_impl.h:32: virtual void
> OnFilterRemoved();
> On 2013/03/28 11:00:10, tommi wrote:
> > are these virtual to support mocking? (something else?)
> 
> By mistake :-) Removed.
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
> File content/renderer/webrtc_logging_message_filter.cc (right):
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
> content/renderer/webrtc_logging_message_filter.cc:83: delete message;
> On 2013/03/28 11:00:10, tommi wrote:
> > log error?
> 
> Sure, sounds good for this filter. Done.
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
> File content/renderer/webrtc_logging_message_filter.h (right):
> 
>
https://codereview.chromium.org/13119009/diff/3001/content/renderer/webrtc_lo...
> content/renderer/webrtc_logging_message_filter.h:28: virtual bool
> OnMessageReceived(const IPC::Message& message) OVERRIDE;
> On 2013/03/28 11:00:10, tommi wrote:
> > make private
> 
> Done.

What I see LGTM. It's hard to evaluate the security design in full, though,
without seeing the format of the data exchanged in the shared memory, and I
assume there might be some synchronization piece missing?

[Henrik Grunell, 2013-04-03 09:07:12]

On 2013/04/02 21:51:40, Chris Evans wrote:
> On 2013/04/02 10:06:40, Henrik Grunell wrote:
> > Added Chris as reviewer of:
> > content/common/webrtc_logging_messages.h
> > ipc/ipc_message_start.h
> > 
> > Please review these (and any other files).
> > 
> 
> What I see LGTM. It's hard to evaluate the security design in full, though,
> without seeing the format of the data exchanged in the shared memory, and I
> assume there might be some synchronization piece missing?

Sync will be so that the handler writes to it; no reading during that time. When
it's done, it tells the handler host and it will read from it; no reading during
that time.

[Henrik Grunell, 2013-04-05 11:43:01]

Pinging Brett.

[tommi (sloooow) - chröme, 2013-04-05 12:25:58]

lgtm

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
File content/renderer/webrtc_logging_handler_impl.h (right):

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
content/renderer/webrtc_logging_handler_impl.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
nit: I think the (c) should not be there but lint should point that out.

[brettw, 2013-04-05 19:59:05]

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
File content/renderer/webrtc_logging_handler_impl.cc (right):

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
content/renderer/webrtc_logging_handler_impl.cc:28:
if(!io_message_loop_->BelongsToCurrentThread()) {
I'm usually kind of worried when I see functions where you don't know what
thread you're on. Is there a reason you can't just assert that you're on the I/O
thread?

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
content/renderer/webrtc_logging_handler_impl.cc:49: LOG(ERROR) << "Could not
open log.";
Does this need to be non-debug logging? Since this is in the renderer you're
probably not going to see it anyway in non-debug environments due to the
sandbox.

[Henrik Grunell, 2013-04-08 09:15:39]

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
File content/renderer/webrtc_logging_handler_impl.cc (right):

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
content/renderer/webrtc_logging_handler_impl.cc:28:
if(!io_message_loop_->BelongsToCurrentThread()) {
On 2013/04/05 19:59:05, brettw wrote:
> I'm usually kind of worried when I see functions where you don't know what
> thread you're on. Is there a reason you can't just assert that you're on the
I/O
> thread?

This will be called from libjingle. Should libjingle be enforced to call this on
the I/O thread? (That's OK with me if it's preferred.)

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
content/renderer/webrtc_logging_handler_impl.cc:49: LOG(ERROR) << "Could not
open log.";
On 2013/04/05 19:59:05, brettw wrote:
> Does this need to be non-debug logging? Since this is in the renderer you're
> probably not going to see it anyway in non-debug environments due to the
> sandbox.

DLOG is fine with me. Changed.

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
File content/renderer/webrtc_logging_handler_impl.h (right):

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
content/renderer/webrtc_logging_handler_impl.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/04/05 12:25:58, tommi wrote:
> nit: I think the (c) should not be there but lint should point that out.

The discussion seems not finished yet on chromium-dev. (Last mail: 'From that I
conclude: "Who cares, just keep using (c) and that is fine".') Neither lint nor
presubmit script complains. But I'll sure remove if you want, let me know.

[tommi (sloooow) - chröme, 2013-04-08 16:35:55]

lgtm

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
File content/renderer/webrtc_logging_handler_impl.h (right):

https://codereview.chromium.org/13119009/diff/17001/content/renderer/webrtc_l...
content/renderer/webrtc_logging_handler_impl.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/04/08 09:15:39, Henrik Grunell wrote:
> On 2013/04/05 12:25:58, tommi wrote:
> > nit: I think the (c) should not be there but lint should point that out.
> 
> The discussion seems not finished yet on chromium-dev. (Last mail: 'From that
I
> conclude: "Who cares, just keep using (c) and that is fine".') Neither lint
nor
> presubmit script complains. But I'll sure remove if you want, let me know.

Ah, let's not waste more time on it then.

[Henrik Grunell, 2013-04-10 10:06:15]

Brett, can you please look at this now? (The others have LGTMd.)

[Henrik Grunell, 2013-04-10 10:07:29]

On 2013/04/10 10:06:15, Henrik Grunell wrote:
> Brett, can you please look at this now? (The others have LGTMd.)

Oh sorry, was thinking about the other CL. You have already started on this. Can
you please take another look? :)

[Henrik Grunell, 2013-04-11 19:23:31]

On 2013/04/10 10:07:29, Henrik Grunell wrote:
> On 2013/04/10 10:06:15, Henrik Grunell wrote:
> > Brett, can you please look at this now? (The others have LGTMd.)
> 
> Oh sorry, was thinking about the other CL. You have already started on this.
Can
> you please take another look? :)

Brett, if you have a chance to look at this today that would be great. I have a
couple of CLs in the pipeline in a dependency chain so I like to get it moving.

[brettw, 2013-04-11 20:00:00]

I'm definitely in favor of defining which thread libjingle will call us back on.
I don't particularly care which thread this is, though, I'm just concerned that
the code doesn't seem to know which thread and is designed to handle "whatever."
:)

https://chromiumcodereview.appspot.com/13119009/diff/63001/content/browser/re...
File content/browser/renderer_host/webrtc_logging_handler_host.cc (right):

https://chromiumcodereview.appspot.com/13119009/diff/63001/content/browser/re...
content/browser/renderer_host/webrtc_logging_handler_host.cc:13: const size_t
kWebRtcLogSize = 6 * 1024 * 1024;  // 6 MB
Under what circumstances will this class be used? It seems like once we get an
"OpenLog" message we'll immediately eat 6MB of address space. It's difficult for
me to see whether this is OK or not just in the context of this one patch.

https://chromiumcodereview.appspot.com/13119009/diff/63001/content/browser/re...
content/browser/renderer_host/webrtc_logging_handler_host.cc:46: LOG(ERROR) <<
"Failed to create shared memory.";
DLOG is probably better.

[Henrik Grunell, 2013-04-12 12:23:41]

Only rebase changes for *.gypi files.

https://chromiumcodereview.appspot.com/13119009/diff/17001/content/renderer/w...
File content/renderer/webrtc_logging_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/13119009/diff/17001/content/renderer/w...
content/renderer/webrtc_logging_handler_impl.cc:28:
if(!io_message_loop_->BelongsToCurrentThread()) {
On 2013/04/08 09:15:39, Henrik Grunell wrote:
> On 2013/04/05 19:59:05, brettw wrote:
> > I'm usually kind of worried when I see functions where you don't know what
> > thread you're on. Is there a reason you can't just assert that you're on the
> I/O
> > thread?
> 
> This will be called from libjingle. Should libjingle be enforced to call this
on
> the I/O thread? (That's OK with me if it's preferred.)

Adding reply by brettw:
"I'm definitely in favor of defining which thread libjingle will call us back
on.
I don't particularly care which thread this is, though, I'm just concerned that
the code doesn't seem to know which thread and is designed to handle "whatever."
:)"

I was mistaken, it will be called from Chromium. Changed to dcheck.

https://chromiumcodereview.appspot.com/13119009/diff/63001/content/browser/re...
File content/browser/renderer_host/webrtc_logging_handler_host.cc (right):

https://chromiumcodereview.appspot.com/13119009/diff/63001/content/browser/re...
content/browser/renderer_host/webrtc_logging_handler_host.cc:46: LOG(ERROR) <<
"Failed to create shared memory.";
On 2013/04/11 20:00:00, brettw wrote:
> DLOG is probably better.

Done.

[brettw, 2013-04-17 18:10:31]

lgtm

https://chromiumcodereview.appspot.com/13119009/diff/68001/content/renderer/w...
File content/renderer/webrtc_logging_message_filter.cc (right):

https://chromiumcodereview.appspot.com/13119009/diff/68001/content/renderer/w...
content/renderer/webrtc_logging_message_filter.cc:83: LOG(ERROR) << "IPC channel
not available.";
Can this be a DLOG?

[Henrik Grunell, 2013-04-18 06:57:02]

One code review fix. 3 gypi file changes are just rebase.

https://chromiumcodereview.appspot.com/13119009/diff/68001/content/renderer/w...
File content/renderer/webrtc_logging_message_filter.cc (right):

https://chromiumcodereview.appspot.com/13119009/diff/68001/content/renderer/w...
content/renderer/webrtc_logging_message_filter.cc:83: LOG(ERROR) << "IPC channel
not available.";
On 2013/04/17 18:10:31, brettw wrote:
> Can this be a DLOG?

I think so. Changed.

[commit-bot: I haz the power, 2013-04-18 06:59:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/13119009/77001

[commit-bot: I haz the power, 2013-04-18 07:14:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/13119009/82003

[commit-bot: I haz the power, 2013-04-18 09:17:04]

Change committed as 194861