[DevTools] [PowerProfiler] Fix for browser crash with active timeline recording for capturing power.

[DevTools] [PowerProfiler] Fix for browser crash with active timeline recording for capturing power.

The PowerProfilerService is a singleton class. It stores the PowerProfilerObserver to notify
about the power events. With the active timeline recording, the DevToolsPowerHandler registers
itself as the observer. When the tab close event occurs, the DevToolsPowerHandler is killed.
But the PowerProfilerService still contains a dangling reference to the observer thereby
causing the crash.

With this CL, DevToolsPowerHandler will remove itself from the observer to avoid the dangling
reference with the PowerProfilerService.

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

[vivekg, 2014-06-12 05:14:59]

PTAL, thank you!

[vivekg, 2014-06-12 05:16:44]

To verify this crash on linux, I have created a dummy power provider here:
https://codereview.chromium.org/324283002/

[qsr, 2014-06-13 15:45:59]

https://codereview.chromium.org/332623003/diff/1/content/browser/devtools/dev...
File content/browser/devtools/devtools_power_handler.cc (right):

https://codereview.chromium.org/332623003/diff/1/content/browser/devtools/dev...
content/browser/devtools/devtools_power_handler.cc:27:
PowerProfilerService::GetInstance()->RemoveObserver(this);
LGTM, but you might want to use a base/scoped_observer.h instead, if you do not
want to have to handle this kind of issues.

[yurys, 2014-06-15 05:37:29]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-06-15 05:38:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/332623003/1

[commit-bot: I haz the power, 2014-06-15 07:35:06]

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

[commit-bot: I haz the power, 2014-06-15 07:35:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[vivekg, 2014-06-16 06:26:05]

On 2014/06/15 05:37:29, yurys wrote:
> The CQ bit was checked by mailto:yurys@chromium.org

@yurys, chromium presubmit is failing due to missing LGTM from OWNER.

[yurys, 2014-06-16 07:20:20]

lgtm

[commit-bot: I haz the power, 2014-06-16 09:27:23]

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

[commit-bot: I haz the power, 2014-06-16 09:27:24]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[vivekg, 2014-06-16 09:29:57]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-06-16 09:30:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/332623003/1

[commit-bot: I haz the power, 2014-06-16 11:17:25]

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

[commit-bot: I haz the power, 2014-06-16 11:17:26]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[vivekg, 2014-06-17 08:32:35]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-06-17 08:34:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/332623003/1

[commit-bot: I haz the power, 2014-06-17 12:54:20]

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

[commit-bot: I haz the power, 2014-06-17 12:54:21]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[vivekg, 2014-06-17 16:47:20]

On 2014/06/17 12:54:21, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

This was failing due to DCHECK in PowerProfilerService::Stop(). With the
DevToolsPowerHandler object being created, without the PowerDataProvider, the
state is UNINITIALIZED. With the object destruction, we invoke RemoveObserver
which in trun tries to call Stop() thereby causing the crash during the run of
the telemetry tests. Adding the HasObserver method to check for existance of the
observer and then call the removal.

[qsr, 2014-06-17 16:52:49]

https://codereview.chromium.org/332623003/diff/20001/content/browser/devtools...
File content/browser/devtools/devtools_power_handler.cc (right):

https://codereview.chromium.org/332623003/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_power_handler.cc:27: if
(PowerProfilerService::GetInstance()->HasObserver(this)) {
That feels wrong to me. If I have added myself to something, I should not have
to test that I am still an observer. If the service didn't do anything when I
register myself as an observer, it should find out by itself that and do the
right thing.

[vivekg, 2014-06-17 17:25:53]

On 2014/06/17 16:52:49, qsr wrote:
>
https://codereview.chromium.org/332623003/diff/20001/content/browser/devtools...
> File content/browser/devtools/devtools_power_handler.cc (right):
> 
>
https://codereview.chromium.org/332623003/diff/20001/content/browser/devtools...
> content/browser/devtools/devtools_power_handler.cc:27: if
> (PowerProfilerService::GetInstance()->HasObserver(this)) {
> That feels wrong to me. If I have added myself to something, I should not have
> to test that I am still an observer. If the service didn't do anything when I
> register myself as an observer, it should find out by itself that and do the
> right thing.

I agree to your point about the self check for being a valid observer. 

The reason I added this check is due to the fact that the AddObserver is called
upon starting of the profiling. With no DataProvider, there wont be any start
possible. With this, either we need to have a status variable (e.g. a boolean)
to reflect whether the object is acting as an observer. If yes, then remove it.
Instead of each object keeping the track of it, I thought we could have a
utility method, HasObserver, as is also the case with [1] to check and remove
the observer. I may be wrong though with my assumption. :)

[1]
https://code.google.com/p/chromium/codesearch#search/&q=%5C:%5C:HasObserver%5...

[qsr, 2014-06-17 17:58:30]

On 2014/06/17 17:25:53, vivekg_ wrote:
> On 2014/06/17 16:52:49, qsr wrote:
> >
>
https://codereview.chromium.org/332623003/diff/20001/content/browser/devtools...
> > File content/browser/devtools/devtools_power_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/332623003/diff/20001/content/browser/devtools...
> > content/browser/devtools/devtools_power_handler.cc:27: if
> > (PowerProfilerService::GetInstance()->HasObserver(this)) {
> > That feels wrong to me. If I have added myself to something, I should not
have
> > to test that I am still an observer. If the service didn't do anything when
I
> > register myself as an observer, it should find out by itself that and do the
> > right thing.
> 
> I agree to your point about the self check for being a valid observer. 
> 
> The reason I added this check is due to the fact that the AddObserver is
called
> upon starting of the profiling. With no DataProvider, there wont be any start
> possible. With this, either we need to have a status variable (e.g. a boolean)
> to reflect whether the object is acting as an observer. If yes, then remove
it.
> Instead of each object keeping the track of it, I thought we could have a
> utility method, HasObserver, as is also the case with [1] to check and remove
> the observer. I may be wrong though with my assumption. :)
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#search/&q=%5C:%5C:HasObserver%5...

 The usage of this is not -> AddObserver(this); if(HasObserver(this)
RemoveObserver(this). If I know I added myself, I should not have to test for
it.

 Another way of solving your issue is doing an early return in RemoveObserver if
the list is empty.

[vivekg, 2014-06-18 04:10:23]

On 2014/06/17 17:58:30, qsr wrote:
>  Another way of solving your issue is doing an early return in RemoveObserver
if
> the list is empty.

Done, PTAL.

[vivekg, 2014-06-18 16:34:55]

The patch https://codereview.chromium.org/341853002/ is dependent on this.

[qsr, 2014-06-18 16:36:47]

lgtm

[vivekg, 2014-06-18 16:46:50]

On 2014/06/18 16:36:47, qsr wrote:
> lgtm

Thank you @qsr again. :) I think we also need an OWNER's lgtm for this to be
landed. 

+sky

[vivekg, 2014-06-18 23:38:56]

On 2014/06/18 16:46:50, vivekg_ wrote:
> On 2014/06/18 16:36:47, qsr wrote:
> > lgtm
> 
> Thank you @qsr again. :) I think we also need an OWNER's lgtm for this to be
> landed. 
> 
> +sky

+jam

[jam, 2014-06-19 16:42:34]

https://codereview.chromium.org/332623003/diff/40001/content/browser/power_pr...
File content/browser/power_profiler/power_profiler_service.cc (right):

https://codereview.chromium.org/332623003/diff/40001/content/browser/power_pr...
content/browser/power_profiler/power_profiler_service.cc:63: if
(!observers_.might_have_observers())
why is this needed?

[vivekg, 2014-06-19 17:46:02]

https://codereview.chromium.org/332623003/diff/40001/content/browser/power_pr...
File content/browser/power_profiler/power_profiler_service.cc (right):

https://codereview.chromium.org/332623003/diff/40001/content/browser/power_pr...
content/browser/power_profiler/power_profiler_service.cc:63: if
(!observers_.might_have_observers())
On 2014/06/19 16:42:34, jam wrote:
> why is this needed?

This was added after the discussion as per the
https://codereview.chromium.org/332623003/#msg24

[jam, 2014-06-20 05:59:46]

https://codereview.chromium.org/332623003/diff/40001/content/browser/power_pr...
File content/browser/power_profiler/power_profiler_service.cc (right):

https://codereview.chromium.org/332623003/diff/40001/content/browser/power_pr...
content/browser/power_profiler/power_profiler_service.cc:63: if
(!observers_.might_have_observers())
On 2014/06/19 17:46:02, vivekg_ wrote:
> On 2014/06/19 16:42:34, jam wrote:
> > why is this needed?
> 
> This was added after the discussion as per the
> https://codereview.chromium.org/332623003/#msg24

I don't follow that conversation, can you explained in a self-contained comment?

I'm looking at ObserverList::RemoveObserver code in
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...
and it doesn't assert that the given pointer is in the list.

[vivekg, 2014-06-20 06:20:59]

On 2014/06/20 05:59:46, jam wrote:
> 
> I don't follow that conversation, can you explained in a self-contained
comment?
> 
> I'm looking at ObserverList::RemoveObserver code in
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...
> and it doesn't assert that the given pointer is in the list.

The assertion is coming in PowerProfilerService::Stop() for the DCHECK(status_
== PROFILING) here [1].

I think the right fix would be in PowerProfilerService::RemoveObserver at [2] to
have the condition to check the status_ variable for PROFILING.

  if (status_ == PROFILING && !observers_.might_have_observers())
    Stop();

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/po...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/po...

[vivekg, 2014-06-20 06:31:19]

Uploaded a new patch, PTAL.

[jam, 2014-06-20 14:56:44]

lgtm

[qsr, 2014-06-20 15:14:53]

Still LGTM

[vivekg, 2014-06-20 15:44:16]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-06-20 15:46:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/332623003/60001

[commit-bot: I haz the power, 2014-06-20 17:14:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-20 19:33:46]

Change committed as 278798