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

Issue 2715723002: Fix a crash in MediaSessionServiceImpl due to life time issue (Closed)

Created:
3 years, 10 months ago by Zhiqiang Zhang (Slow)
Modified:
3 years, 10 months ago
CC:
whywhat, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix a crash in MediaSessionServiceImpl due to life time issue Previously we assume MediaSessionServiceImpl is owned by RenderFrameHost and will be destroyed as RenderFrameHost is destroyed. However this does not stand and MediaSessionServiceImpl may outlive RenderFrameHost. Therefore it is unsafe to keep a raw RenderFrameHost pointer in MediaSessionServiceImpl. This CL fixes a crash due to this issue. BUG=695398 Review-Url: https://codereview.chromium.org/2715723002 Cr-Commit-Position: refs/heads/master@{#452649} Committed: https://chromium.googlesource.com/chromium/src/+/2d0ddb393c09c26c3d225bed0a005ee8ec76cc57

Patch Set 1 #

Total comments: 4

Patch Set 2 : addressed avayvod@'s comments #

Total comments: 4

Patch Set 3 : addressed nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -8 lines) Patch
M content/browser/media/session/media_session_impl.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/media/session/media_session_impl.cc View 2 chunks +10 lines, -1 line 0 comments Download
M content/browser/media/session/media_session_service_impl.h View 2 chunks +3 lines, -2 lines 0 comments Download
M content/browser/media/session/media_session_service_impl.cc View 1 2 4 chunks +20 lines, -5 lines 0 comments Download

Messages

Total messages: 30 (16 generated)
Zhiqiang Zhang (Slow)
PTAL
3 years, 10 months ago (2017-02-23 15:37:35 UTC) #3
Zhiqiang Zhang (Slow)
On 2017/02/23 at 15:37:35, Zhiqiang Zhang wrote: > PTAL FYI: A fix for a similar ...
3 years, 10 months ago (2017-02-23 17:36:41 UTC) #7
whywhat
lgtm https://codereview.chromium.org/2715723002/diff/1/content/browser/media/session/media_session_service_impl.cc File content/browser/media/session/media_session_service_impl.cc (right): https://codereview.chromium.org/2715723002/diff/1/content/browser/media/session/media_session_service_impl.cc#newcode65 content/browser/media/session/media_session_service_impl.cc:65: if (GetRenderFrameHost()) { nit: maybe store GetRenderFrameHost() not ...
3 years, 10 months ago (2017-02-23 17:57:42 UTC) #8
mlamouri (slow - plz ping)
I think we should do something like the permission service context but maybe that's better ...
3 years, 10 months ago (2017-02-23 17:57:51 UTC) #10
mlamouri (slow - plz ping)
Also, I think we need a test :) or at least be able to manually ...
3 years, 10 months ago (2017-02-23 17:58:25 UTC) #12
Zhiqiang Zhang (Slow)
https://codereview.chromium.org/2715723002/diff/1/content/browser/media/session/media_session_service_impl.cc File content/browser/media/session/media_session_service_impl.cc (right): https://codereview.chromium.org/2715723002/diff/1/content/browser/media/session/media_session_service_impl.cc#newcode65 content/browser/media/session/media_session_service_impl.cc:65: if (GetRenderFrameHost()) { On 2017/02/23 at 17:57:42, whywhat wrote: ...
3 years, 10 months ago (2017-02-23 20:15:55 UTC) #13
Zhiqiang Zhang (Slow)
On 2017/02/23 at 17:58:25, mlamouri wrote: > Also, I think we need a test :) ...
3 years, 10 months ago (2017-02-23 20:24:35 UTC) #14
Zhiqiang Zhang (Slow)
On 2017/02/23 at 17:57:51, mlamouri wrote: > I think we should do something like the ...
3 years, 10 months ago (2017-02-23 20:33:04 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715723002/20001
3 years, 10 months ago (2017-02-23 20:34:09 UTC) #18
Charlie Reis
LGTM with nits. https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/session/media_session_impl.h File content/browser/media/session/media_session_impl.h (right): https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/session/media_session_impl.h#newcode170 content/browser/media/session/media_session_impl.h:170: void RenderFrameDeleted(RenderFrameHost* rfh) override; nit: The ...
3 years, 10 months ago (2017-02-23 20:36:41 UTC) #19
Charlie Reis
On 2017/02/23 20:33:04, Zhiqiang Zhang wrote: > On 2017/02/23 at 17:57:51, mlamouri wrote: > > ...
3 years, 10 months ago (2017-02-23 20:38:04 UTC) #22
Zhiqiang Zhang (Slow)
https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/session/media_session_impl.h File content/browser/media/session/media_session_impl.h (right): https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/session/media_session_impl.h#newcode170 content/browser/media/session/media_session_impl.h:170: void RenderFrameDeleted(RenderFrameHost* rfh) override; On 2017/02/23 at 20:36:41, Charlie ...
3 years, 10 months ago (2017-02-23 21:04:06 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715723002/40001
3 years, 10 months ago (2017-02-23 21:04:26 UTC) #27
commit-bot: I haz the power
3 years, 10 months ago (2017-02-23 22:09:13 UTC) #30
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2d0ddb393c09c26c3d225bed0a00...

Powered by Google App Engine
This is Rietveld 408576698