Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2782053002:
MediaSession: Stop observers on MediaSession destruction (Closed)
|
Created:
3 years, 8 months ago by seantopping Modified:
3 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaSession: Stop observers on MediaSession destruction
MediaSessionObservers should have their MediaSession pointers reset
immediately after MediaSessionDestroyed. This prevents a use-after-free
bug that can occurs when MediaSessionObservers outlive the MediaSession.
BUG=None
Review-Url: https://codereview.chromium.org/2782053002
Cr-Commit-Position: refs/heads/master@{#460740}
Committed: https://chromium.googlesource.com/chromium/src/+/757f5dc9a5cd6c3679b809701d09ba6aa6ff812b
Patch Set 1 #
Total comments: 2
Patch Set 2 : One loop is better than two #Patch Set 3 : Rebase #Messages
Total messages: 27 (17 generated)
seantopping@chromium.org changed reviewers: + derekjchow@chromium.org, zqzhang@chromium.org
It looks like MediaSessionObserver is emulating the WebContentsObserver pattern, so we want to make sure we also perform the same observer reset that WebContents does here: https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... This fixes a bug as described in the commit message. Thanks!
derekjchow@google.com changed reviewers: + derekjchow@google.com
https://codereview.chromium.org/2782053002/diff/1/content/browser/media/sessi... File content/browser/media/session/media_session_impl.cc (right): https://codereview.chromium.org/2782053002/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_impl.cc:99: observer.StopObserving(); Probably makes sense to fuse the two for loops.
https://codereview.chromium.org/2782053002/diff/1/content/browser/media/sessi... File content/browser/media/session/media_session_impl.cc (right): https://codereview.chromium.org/2782053002/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_impl.cc:99: observer.StopObserving(); On 2017/03/29 00:50:17, derekjchow wrote: > Probably makes sense to fuse the two for loops. Done.
seantopping@chromium.org changed reviewers: + mlamouri@chromium.org
+mlamouri for OWNERS
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm IIRC there were some reviewers having opinions on not putting this line in MediaSessionImpl, as they said the clients should stop observing themselves. Now it seems that both clients (CastShell and Chrome) agree that this should be done automatically, feel free to go ahead.
The CQ bit was checked by seantopping@chromium.org to run a CQ dry run
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.
The CQ bit was checked by seantopping@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zqzhang@chromium.org Link to the patchset: https://codereview.chromium.org/2782053002/#ps40001 (title: "Rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rs-lgtm
The CQ bit was checked by zqzhang@chromium.org
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": 1490880162776600,
"parent_rev": "ea4eca92a1bc78ebef953fe1875fe0caa08a5965", "commit_rev":
"757f5dc9a5cd6c3679b809701d09ba6aa6ff812b"}
Message was sent while issue was closed.
Description was changed from ========== MediaSession: Stop observers on MediaSession destruction MediaSessionObservers should have their MediaSession pointers reset immediately after MediaSessionDestroyed. This prevents a use-after-free bug that can occurs when MediaSessionObservers outlive the MediaSession. BUG=None ========== to ========== MediaSession: Stop observers on MediaSession destruction MediaSessionObservers should have their MediaSession pointers reset immediately after MediaSessionDestroyed. This prevents a use-after-free bug that can occurs when MediaSessionObservers outlive the MediaSession. BUG=None Review-Url: https://codereview.chromium.org/2782053002 Cr-Commit-Position: refs/heads/master@{#460740} Committed: https://chromium.googlesource.com/chromium/src/+/757f5dc9a5cd6c3679b809701d09... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/757f5dc9a5cd6c3679b809701d09... |
