Change WebRTC log callback registration in browser process.

Change WebRTC log callback registration in browser process.

This ensures we forward all log messages, also when shutting down.

* Register the callback when logging is started, and unregister when stopping.
* Remove the MediaStream request filtering when running callbacks.

BUG=601389
Committed: https://crrev.com/34f2e5e23e6a3a34742089fc49590075d4352685
Cr-Commit-Position: refs/heads/master@{#386058}

[Henrik Grunell, 2016-04-07 12:27:22]

Description was changed from

==========
Change WebRTC log callback registration in browser process.

* Register the callback when logging is started, and unregister when stopping.
* Remove the MediaStream request filtering when running callbacks.

BUG=601389
==========

to

==========
Change WebRTC log callback registration in browser process.

This ensures we forward all log messages, also when shutting down.

* Register the callback when logging is started, and unregister when stopping.
* Remove the MediaStream request filtering when running callbacks.

BUG=601389
==========

[Henrik Grunell, 2016-04-07 12:27:23]

grunell@chromium.org changed reviewers:
+ olka@chromium.org

[Henrik Grunell, 2016-04-07 12:28:47]

grunell@chromium.org changed reviewers:
+ tommi@chromium.org

[Henrik Grunell, 2016-04-07 12:33:06]

grunell@chromium.org changed reviewers:
+ avi@chromium.org

[Henrik Grunell, 2016-04-07 12:35:43]

Tommi: chrome/browser/*/media/*

Avi: content/browser/renderer_host/* and content/public/browser/*

Olga: Everything

[Henrik Grunell, 2016-04-07 12:50:32]

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

[commit-bot: I haz the power, 2016-04-07 12:50:50]

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

[tommi (sloooow) - chröme, 2016-04-07 12:51:44]

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:671:
host->UnsetWebRtcLogMessageCallback();
could this possibly 'unset' a callback that was not set via
EnableBrowserProcessLoggingOnUIThread?

E.g. a call sequence like this:

1. A call is made to EnableBrowserProcessLoggingOnUIThread()
  -> host is null
2. A second call is made to EnableBrowserProcessLoggingOnUIThread()
  -> is not null, so the callback is set.
3. The call to Disable that corresponds to step #1 happens
  -> This causes the #2 Enable scope, to be prematurely closed.
4. Second call to Disable is effectively a no-op since a callback isn't
registered.

So, should Enable return a bool?

Should we have a Scoped type class that makes sure we automatically always call
Disable if Enable has returned true?

Should we DCHECK that a callback is in fact registered when we call
UnsetWebRtcLogMessageCallback?

https://codereview.chromium.org/1871533002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1871533002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:153: void
UnsetWebRtcLogMessageCallback() override;
nit: s/Unset/Clear

[o1ka, 2016-04-07 13:00:11]

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:644:
BrowserThread::PostTask(
Probably move it to the top to have less chances to miss something in the
beginning?

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:661:
host->SetWebRtcLogMessageCallback(
What guarantees that |host| is still alive here (i.e. the pointer is valid)? A
comment would be nice.

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:671:
host->UnsetWebRtcLogMessageCallback();
What guarantees that |host| is still alive here (i.e. the pointer is valid)

[commit-bot: I haz the power, 2016-04-07 13:00:58]

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

[commit-bot: I haz the power, 2016-04-07 13:00:59]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Henrik Grunell, 2016-04-07 13:34:53]

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:671:
host->UnsetWebRtcLogMessageCallback();
On 2016/04/07 12:51:44, tommi-sloooow wrote:
> could this possibly 'unset' a callback that was not set via
> EnableBrowserProcessLoggingOnUIThread?
> 
> E.g. a call sequence like this:
> 
> 1. A call is made to EnableBrowserProcessLoggingOnUIThread()
>   -> host is null
> 2. A second call is made to EnableBrowserProcessLoggingOnUIThread()
>   -> is not null, so the callback is set.
> 3. The call to Disable that corresponds to step #1 happens
>   -> This causes the #2 Enable scope, to be prematurely closed.
> 4. Second call to Disable is effectively a no-op since a callback isn't
> registered.
> 
> So, should Enable return a bool?

This can't happen. These two functions are only called when logging is started
and stopped respectively, and we have a state so it's not possible to start
twice.

> 
> Should we have a Scoped type class that makes sure we automatically always
call
> Disable if Enable has returned true?

Hmm, there is still a disable when the render process is going away, but that's
in RenderProcessHostImpl:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

It would be nice to move that to here, but I don't think we can rely on the host
still being alive which is needed to be able to remove the callback. If we
continue to rely on the RPHI to remove it, some commenting around this is good.

Where you thinking of some weak ptr style type?

> Should we DCHECK that a callback is in fact registered when we call
> UnsetWebRtcLogMessageCallback?

If we keep the current approach, we can't.

https://codereview.chromium.org/1871533002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1871533002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:153: void
UnsetWebRtcLogMessageCallback() override;
On 2016/04/07 12:51:44, tommi-sloooow wrote:
> nit: s/Unset/Clear

Done.

[tommi (sloooow) - chröme, 2016-04-07 13:52:19]

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:644:
BrowserThread::PostTask(
On 2016/04/07 13:00:11, o1ka wrote:
> Probably move it to the top to have less chances to miss something in the
> beginning?

Won't that introduce a potential race with writing the log header?

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:661:
host->SetWebRtcLogMessageCallback(
On 2016/04/07 13:00:11, o1ka wrote:
> What guarantees that |host| is still alive here (i.e. the pointer is valid)? A
> comment would be nice.

FromID() must be called on the UI thread, it returns a pointer that can only be
cleared on the UI thread, so we're safe in this scope as long as we don't do any
message processing (which we don't)

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:671:
host->UnsetWebRtcLogMessageCallback();
On 2016/04/07 13:34:53, Henrik Grunell wrote:
> On 2016/04/07 12:51:44, tommi-sloooow wrote:
> > could this possibly 'unset' a callback that was not set via
> > EnableBrowserProcessLoggingOnUIThread?
> > 
> > E.g. a call sequence like this:
> > 
> > 1. A call is made to EnableBrowserProcessLoggingOnUIThread()
> >   -> host is null
> > 2. A second call is made to EnableBrowserProcessLoggingOnUIThread()
> >   -> is not null, so the callback is set.
> > 3. The call to Disable that corresponds to step #1 happens
> >   -> This causes the #2 Enable scope, to be prematurely closed.
> > 4. Second call to Disable is effectively a no-op since a callback isn't
> > registered.
> > 
> > So, should Enable return a bool?
> 
> This can't happen. These two functions are only called when logging is started
> and stopped respectively, and we have a state so it's not possible to start
> twice.

From the code, it looks like it's at least theoratically possible, right?  If
it's not supposed to be possible, can we have a check to that effect?

> 
> > 
> > Should we have a Scoped type class that makes sure we automatically always
> call
> > Disable if Enable has returned true?
> 
> Hmm, there is still a disable when the render process is going away, but
that's
> in RenderProcessHostImpl:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> It would be nice to move that to here, but I don't think we can rely on the
host
> still being alive which is needed to be able to remove the callback. If we
> continue to rely on the RPHI to remove it, some commenting around this is
good.
> 
> Where you thinking of some weak ptr style type?
> 
> > Should we DCHECK that a callback is in fact registered when we call
> > UnsetWebRtcLogMessageCallback?
> 
> If we keep the current approach, we can't.

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:671:
host->UnsetWebRtcLogMessageCallback();
On 2016/04/07 13:34:53, Henrik Grunell wrote:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> It would be nice to move that to here, but I don't think we can rely on the
host
> still being alive which is needed to be able to remove the callback. If we
> continue to rely on the RPHI to remove it, some commenting around this is
good.
> 
> Where you thinking of some weak ptr style type?
> 

Depends on the use, but I was thinking of something like:

class Foo {
 public:
  ...
  class ScopedEnableLogging {
   public:
    ScopedEnableLogging(Foo* foo) : foo_(foo), enabled_(foo->EnableLogging()) {}
    ~ScopedEnableLogging() {
      if (enabled_)
        foo_->DisableLogging();
    }

   private:  
    const bool enabled_;
  };

 protected:
  friend class ScopedEnableLogging;
  void EnableLogging();
  void DisableLogging();
  ...
};

Maybe that's not applicable in this situation.

[Henrik Grunell, 2016-04-07 14:32:59]

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:644:
BrowserThread::PostTask(
On 2016/04/07 13:52:19, tommi-sloooow wrote:
> On 2016/04/07 13:00:11, o1ka wrote:
> > Probably move it to the top to have less chances to miss something in the
> > beginning?
> 
> Won't that introduce a potential race with writing the log header?

It won't race, since this function and LogMessage() are both on the IO thread.

It's not a big thing, but doesn't hurt to do it early. Moved.

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:661:
host->SetWebRtcLogMessageCallback(
On 2016/04/07 13:52:19, tommi-sloooow wrote:
> On 2016/04/07 13:00:11, o1ka wrote:
> > What guarantees that |host| is still alive here (i.e. the pointer is valid)?
A
> > comment would be nice.
> 
> FromID() must be called on the UI thread, it returns a pointer that can only
be
> cleared on the UI thread, so we're safe in this scope as long as we don't do
any
> message processing (which we don't)

Yes. Actually, I don't think we should spam with a comment for this. In that
case everyone who uses FromID() should have that.

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:671:
host->UnsetWebRtcLogMessageCallback();
On 2016/04/07 13:52:19, tommi-sloooow wrote:
> On 2016/04/07 13:34:53, Henrik Grunell wrote:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> > 
> > It would be nice to move that to here, but I don't think we can rely on the
> host
> > still being alive which is needed to be able to remove the callback. If we
> > continue to rely on the RPHI to remove it, some commenting around this is
> good.
> > 
> > Where you thinking of some weak ptr style type?
> > 
> 
> Depends on the use, but I was thinking of something like:
> 
> class Foo {
>  public:
>   ...
>   class ScopedEnableLogging {
>    public:
>     ScopedEnableLogging(Foo* foo) : foo_(foo), enabled_(foo->EnableLogging())
{}
>     ~ScopedEnableLogging() {
>       if (enabled_)
>         foo_->DisableLogging();
>     }
> 
>    private:  
>     const bool enabled_;
>   };
> 
>  protected:
>   friend class ScopedEnableLogging;
>   void EnableLogging();
>   void DisableLogging();
>   ...
> };
> 
> Maybe that's not applicable in this situation.

No it's no applicable. In fact, the callback will hold a ref to the logging
handler, so we won't even ever destroy it as long as the callback is registered.
We have to clear it in RPHI as done now.

https://codereview.chromium.org/1871533002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_logging_handler_host.cc:671:
host->UnsetWebRtcLogMessageCallback();
On 2016/04/07 13:52:19, tommi-sloooow wrote:
> On 2016/04/07 13:34:53, Henrik Grunell wrote:
> > On 2016/04/07 12:51:44, tommi-sloooow wrote:
> > > could this possibly 'unset' a callback that was not set via
> > > EnableBrowserProcessLoggingOnUIThread?
> > > 
> > > E.g. a call sequence like this:
> > > 
> > > 1. A call is made to EnableBrowserProcessLoggingOnUIThread()
> > >   -> host is null
> > > 2. A second call is made to EnableBrowserProcessLoggingOnUIThread()
> > >   -> is not null, so the callback is set.
> > > 3. The call to Disable that corresponds to step #1 happens
> > >   -> This causes the #2 Enable scope, to be prematurely closed.
> > > 4. Second call to Disable is effectively a no-op since a callback isn't
> > > registered.
> > > 
> > > So, should Enable return a bool?
> > 
> > This can't happen. These two functions are only called when logging is
started
> > and stopped respectively, and we have a state so it's not possible to start
> > twice.
> 
> From the code, it looks like it's at least theoratically possible, right?  If
> it's not supposed to be possible, can we have a check to that effect?

Hmm, in MSM it's OK to call several times, as a comment says. Tests do this.
Here, it should not be. I'd need to add book keeping (a flag) here to check it.
I'd prefer not to add more. You have the last say. :)

> 
> > 
> > > 
> > > Should we have a Scoped type class that makes sure we automatically always
> > call
> > > Disable if Enable has returned true?
> > 
> > Hmm, there is still a disable when the render process is going away, but
> that's
> > in RenderProcessHostImpl:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> > 
> > It would be nice to move that to here, but I don't think we can rely on the
> host
> > still being alive which is needed to be able to remove the callback. If we
> > continue to rely on the RPHI to remove it, some commenting around this is
> good.
> > 
> > Where you thinking of some weak ptr style type?
> > 
> > > Should we DCHECK that a callback is in fact registered when we call
> > > UnsetWebRtcLogMessageCallback?
> > 
> > If we keep the current approach, we can't.
>

[Henrik Grunell, 2016-04-07 14:33:06]

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

[commit-bot: I haz the power, 2016-04-07 14:33:28]

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

[Avi (use Gerrit), 2016-04-07 14:53:31]

lgtm

[commit-bot: I haz the power, 2016-04-07 15:45:34]

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

[commit-bot: I haz the power, 2016-04-07 15:45:34]

Dry run: This issue passed the CQ dry run.

[Henrik Grunell, 2016-04-08 12:13:30]

Ping Tommi.

[tommi (sloooow) - chröme, 2016-04-08 12:42:37]

lgtm

[Henrik Grunell, 2016-04-08 12:48:59]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2016-04-08 12:49:07]

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

[commit-bot: I haz the power, 2016-04-08 12:55:12]

Description was changed from

==========
Change WebRTC log callback registration in browser process.

This ensures we forward all log messages, also when shutting down.

* Register the callback when logging is started, and unregister when stopping.
* Remove the MediaStream request filtering when running callbacks.

BUG=601389
==========

to

==========
Change WebRTC log callback registration in browser process.

This ensures we forward all log messages, also when shutting down.

* Register the callback when logging is started, and unregister when stopping.
* Remove the MediaStream request filtering when running callbacks.

BUG=601389
==========

[commit-bot: I haz the power, 2016-04-08 12:55:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-08 12:56:43]

Description was changed from

==========
Change WebRTC log callback registration in browser process.

This ensures we forward all log messages, also when shutting down.

* Register the callback when logging is started, and unregister when stopping.
* Remove the MediaStream request filtering when running callbacks.

BUG=601389
==========

to

==========
Change WebRTC log callback registration in browser process.

This ensures we forward all log messages, also when shutting down.

* Register the callback when logging is started, and unregister when stopping.
* Remove the MediaStream request filtering when running callbacks.

BUG=601389

Committed: https://crrev.com/34f2e5e23e6a3a34742089fc49590075d4352685
Cr-Commit-Position: refs/heads/master@{#386058}
==========

[commit-bot: I haz the power, 2016-04-08 12:56:45]

Patchset 3 (id:??) landed as
https://crrev.com/34f2e5e23e6a3a34742089fc49590075d4352685
Cr-Commit-Position: refs/heads/master@{#386058}