|
|
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. |
DescriptionFix 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 #
Messages
Total messages: 30 (16 generated)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/23 at 15:37:35, Zhiqiang Zhang wrote: > PTAL FYI: A fix for a similar crash is https://bugs.chromium.org/p/chromium/issues/detail?id=655686, might help you to review.
lgtm https://codereview.chromium.org/2715723002/diff/1/content/browser/media/sessi... File content/browser/media/session/media_session_service_impl.cc (right): https://codereview.chromium.org/2715723002/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_service_impl.cc:65: if (GetRenderFrameHost()) { nit: maybe store GetRenderFrameHost() not to call it twice? https://codereview.chromium.org/2715723002/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_service_impl.cc:97: static_cast<WebContentsImpl*>(WebContentsImpl::FromRenderFrameHost(rfh)); nit: does it handle nullptr properly?
mlamouri@chromium.org changed reviewers: + creis@chromium.org, mlamouri@chromium.org - avayvod@chromium.org
I think we should do something like the permission service context but maybe that's better for a beta cherry-pick. Charlie, how does that look to you?
mlamouri@chromium.org changed reviewers: + avayvod@chromium.org
Also, I think we need a test :) or at least be able to manually reproduce the bug :)
https://codereview.chromium.org/2715723002/diff/1/content/browser/media/sessi... File content/browser/media/session/media_session_service_impl.cc (right): https://codereview.chromium.org/2715723002/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_service_impl.cc:65: if (GetRenderFrameHost()) { On 2017/02/23 at 17:57:42, whywhat wrote: > nit: maybe store GetRenderFrameHost() not to call it twice? Done. https://codereview.chromium.org/2715723002/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_service_impl.cc:97: static_cast<WebContentsImpl*>(WebContentsImpl::FromRenderFrameHost(rfh)); On 2017/02/23 at 17:57:42, whywhat wrote: > nit: does it handle nullptr properly? Yes. But let's be safe. Added a null-check.
On 2017/02/23 at 17:58:25, mlamouri wrote: > Also, I think we need a test :) or at least be able to manually reproduce the bug :) I can reproduce locally in a modified build -- by posting a delayed task to call MediaSessionServiceImpl::SetPlaybackState() when RenderFrameDeleted(). The crash is gone after applying this CL. I'm going to land this.
On 2017/02/23 at 17:57:51, mlamouri wrote: > I think we should do something like the permission service context but maybe that's better for a beta cherry-pick. Charlie, how does that look to you? Plus one question: why the mojo services bounded to a RenderFrameHost are not destroyed after the RFH is destroyed? I see there's method RenderFrameHostImpl::InvalidateMojoConnection(), but the service destructors are not called after that. IIRC I did some logging before and the mojo services used to be destroyed after RFH destruction. Should this be a wider issue wrt RFH and mojo services?
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2715723002/#ps20001 (title: "addressed avayvod@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM with nits. https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/s... File content/browser/media/session/media_session_impl.h (right): https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_impl.h:170: void RenderFrameDeleted(RenderFrameHost* rfh) override; nit: The convention is to have no blank line before. (Have all the overrides in a block.) https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/s... File content/browser/media/session/media_session_service_impl.cc (right): https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_service_impl.cc:42: return content::RenderFrameHost::FromID(render_frame_process_id_, nit: Drop "content::", since we're already in the content namespace.
Description was changed from ========== 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 ========== to ========== 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 ==========
creis@chromium.org changed reviewers: + dcheng@chromium.org
On 2017/02/23 20:33:04, Zhiqiang Zhang wrote: > On 2017/02/23 at 17:57:51, mlamouri wrote: > > I think we should do something like the permission service context but maybe > that's better for a beta cherry-pick. Charlie, how does that look to you? > > Plus one question: why the mojo services bounded to a RenderFrameHost are not > destroyed after the RFH is destroyed? I see there's method > RenderFrameHostImpl::InvalidateMojoConnection(), but the service destructors are > not called after that. IIRC I did some logging before and the mojo services used > to be destroyed after RFH destruction. Should this be a wider issue wrt RFH and > mojo services? [+dcheng for Mojo RFH lifetime question]
The CQ bit was unchecked by zqzhang@chromium.org
The CQ bit was checked by zqzhang@chromium.org
https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/s... File content/browser/media/session/media_session_impl.h (right): https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_impl.h:170: void RenderFrameDeleted(RenderFrameHost* rfh) override; On 2017/02/23 at 20:36:41, Charlie Reis wrote: > nit: The convention is to have no blank line before. (Have all the overrides in a block.) Done. https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/s... File content/browser/media/session/media_session_service_impl.cc (right): https://codereview.chromium.org/2715723002/diff/20001/content/browser/media/s... content/browser/media/session/media_session_service_impl.cc:42: return content::RenderFrameHost::FromID(render_frame_process_id_, On 2017/02/23 at 20:36:41, Charlie Reis wrote: > nit: Drop "content::", since we're already in the content namespace. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2715723002/#ps40001 (title: "addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487883845235450, "parent_rev": "8e0074effadf2beceb5df755b7e61ef095cde7a1", "commit_rev": "2d0ddb393c09c26c3d225bed0a005ee8ec76cc57"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/2d0ddb393c09c26c3d225bed0a00... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2d0ddb393c09c26c3d225bed0a00... |