Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(222)

Issue 332623003: [DevTools] [PowerProfiler] Fix for browser crash with active timeline recording for capturing power. (Closed)

Created:
6 years, 6 months ago by vivekg_samsung
Modified:
6 years, 6 months ago
CC:
chromium-reviews, vsevik, jam, yurys, paulirish+reviews_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org, pfeldman
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

[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

Patch Set 1 #

Total comments: 1

Patch Set 2 : Patch for landing! #

Total comments: 1

Patch Set 3 : #

Total comments: 3

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -1 line) Patch
M content/browser/devtools/devtools_power_handler.cc View 2 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/power_profiler/power_profiler_service.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 40 (0 generated)
vivekg
PTAL, thank you!
6 years, 6 months ago (2014-06-12 05:14:59 UTC) #1
vivekg
To verify this crash on linux, I have created a dummy power provider here: https://codereview.chromium.org/324283002/
6 years, 6 months ago (2014-06-12 05:16:44 UTC) #2
vivekg
6 years, 6 months ago (2014-06-13 06:08:06 UTC) #3
qsr
https://codereview.chromium.org/332623003/diff/1/content/browser/devtools/devtools_power_handler.cc File content/browser/devtools/devtools_power_handler.cc (right): https://codereview.chromium.org/332623003/diff/1/content/browser/devtools/devtools_power_handler.cc#newcode27 content/browser/devtools/devtools_power_handler.cc:27: PowerProfilerService::GetInstance()->RemoveObserver(this); LGTM, but you might want to use a ...
6 years, 6 months ago (2014-06-13 15:45:59 UTC) #4
yurys
The CQ bit was checked by yurys@chromium.org
6 years, 6 months ago (2014-06-15 05:37:29 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/332623003/1
6 years, 6 months ago (2014-06-15 05:38:11 UTC) #6
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 6 months ago (2014-06-15 07:35:06 UTC) #7
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/builds/73791)
6 years, 6 months ago (2014-06-15 07:35:07 UTC) #8
vivekg
On 2014/06/15 05:37:29, yurys wrote: > The CQ bit was checked by mailto:yurys@chromium.org @yurys, chromium ...
6 years, 6 months ago (2014-06-16 06:26:05 UTC) #9
yurys
lgtm
6 years, 6 months ago (2014-06-16 07:20:20 UTC) #10
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 6 months ago (2014-06-16 09:27:23 UTC) #11
commit-bot: I haz the power
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_triggered_tests/builds/24222)
6 years, 6 months ago (2014-06-16 09:27:24 UTC) #12
vivekg
The CQ bit was checked by vivekg@chromium.org
6 years, 6 months ago (2014-06-16 09:29:57 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/332623003/1
6 years, 6 months ago (2014-06-16 09:30:30 UTC) #14
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 6 months ago (2014-06-16 11:17:25 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/builds/42026)
6 years, 6 months ago (2014-06-16 11:17:26 UTC) #16
vivekg
The CQ bit was checked by vivekg@chromium.org
6 years, 6 months ago (2014-06-17 08:32:35 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/332623003/1
6 years, 6 months ago (2014-06-17 08:34:03 UTC) #18
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 6 months ago (2014-06-17 12:54:20 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/builds/42480)
6 years, 6 months ago (2014-06-17 12:54:21 UTC) #20
vivekg
On 2014/06/17 12:54:21, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years, 6 months ago (2014-06-17 16:47:20 UTC) #21
qsr
https://codereview.chromium.org/332623003/diff/20001/content/browser/devtools/devtools_power_handler.cc File content/browser/devtools/devtools_power_handler.cc (right): https://codereview.chromium.org/332623003/diff/20001/content/browser/devtools/devtools_power_handler.cc#newcode27 content/browser/devtools/devtools_power_handler.cc:27: if (PowerProfilerService::GetInstance()->HasObserver(this)) { That feels wrong to me. If ...
6 years, 6 months ago (2014-06-17 16:52:49 UTC) #22
vivekg
On 2014/06/17 16:52:49, qsr wrote: > https://codereview.chromium.org/332623003/diff/20001/content/browser/devtools/devtools_power_handler.cc > File content/browser/devtools/devtools_power_handler.cc (right): > > https://codereview.chromium.org/332623003/diff/20001/content/browser/devtools/devtools_power_handler.cc#newcode27 > ...
6 years, 6 months ago (2014-06-17 17:25:53 UTC) #23
qsr
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/devtools_power_handler.cc ...
6 years, 6 months ago (2014-06-17 17:58:30 UTC) #24
vivekg
On 2014/06/17 17:58:30, qsr wrote: > Another way of solving your issue is doing an ...
6 years, 6 months ago (2014-06-18 04:10:23 UTC) #25
vivekg
The patch https://codereview.chromium.org/341853002/ is dependent on this.
6 years, 6 months ago (2014-06-18 16:34:55 UTC) #26
qsr
lgtm
6 years, 6 months ago (2014-06-18 16:36:47 UTC) #27
vivekg
On 2014/06/18 16:36:47, qsr wrote: > lgtm Thank you @qsr again. :) I think we ...
6 years, 6 months ago (2014-06-18 16:46:50 UTC) #28
vivekg
On 2014/06/18 16:46:50, vivekg_ wrote: > On 2014/06/18 16:36:47, qsr wrote: > > lgtm > ...
6 years, 6 months ago (2014-06-18 23:38:56 UTC) #29
jam
https://codereview.chromium.org/332623003/diff/40001/content/browser/power_profiler/power_profiler_service.cc File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/332623003/diff/40001/content/browser/power_profiler/power_profiler_service.cc#newcode63 content/browser/power_profiler/power_profiler_service.cc:63: if (!observers_.might_have_observers()) why is this needed?
6 years, 6 months ago (2014-06-19 16:42:34 UTC) #30
vivekg
https://codereview.chromium.org/332623003/diff/40001/content/browser/power_profiler/power_profiler_service.cc File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/332623003/diff/40001/content/browser/power_profiler/power_profiler_service.cc#newcode63 content/browser/power_profiler/power_profiler_service.cc:63: if (!observers_.might_have_observers()) On 2014/06/19 16:42:34, jam wrote: > why ...
6 years, 6 months ago (2014-06-19 17:46:02 UTC) #31
jam
https://codereview.chromium.org/332623003/diff/40001/content/browser/power_profiler/power_profiler_service.cc File content/browser/power_profiler/power_profiler_service.cc (right): https://codereview.chromium.org/332623003/diff/40001/content/browser/power_profiler/power_profiler_service.cc#newcode63 content/browser/power_profiler/power_profiler_service.cc:63: if (!observers_.might_have_observers()) On 2014/06/19 17:46:02, vivekg_ wrote: > On ...
6 years, 6 months ago (2014-06-20 05:59:46 UTC) #32
vivekg
On 2014/06/20 05:59:46, jam wrote: > > I don't follow that conversation, can you explained ...
6 years, 6 months ago (2014-06-20 06:20:59 UTC) #33
vivekg
Uploaded a new patch, PTAL.
6 years, 6 months ago (2014-06-20 06:31:19 UTC) #34
jam
lgtm
6 years, 6 months ago (2014-06-20 14:56:44 UTC) #35
qsr
Still LGTM
6 years, 6 months ago (2014-06-20 15:14:53 UTC) #36
vivekg
The CQ bit was checked by vivekg@chromium.org
6 years, 6 months ago (2014-06-20 15:44:16 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/332623003/60001
6 years, 6 months ago (2014-06-20 15:46:07 UTC) #38
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium ...
6 years, 6 months ago (2014-06-20 17:14:26 UTC) #39
commit-bot: I haz the power
6 years, 6 months ago (2014-06-20 19:33:46 UTC) #40
Message was sent while issue was closed.
Change committed as 278798

Powered by Google App Engine
This is Rietveld 408576698