[Media Router] PresentationFrame entry is not destroyed when the corresponding RenderFrameHost is gone |
||||||
Issue descriptionLooks like a recent refactoring of delegate observers in PresentationServiceDelegateImpl inadvertently removed the statement to erase the PresentationFrame entry on RemoveDelegateObserver. The result is that the PresentationFrame entry stays in the map and is never referenced again, resulting in memory leak. A fix would be to make sure the PresentationFrame entry is erased when Reset() is called with a given RFH ID. Note that Reset() is called when the frame navigates (not including in-page) and when the frame is being destroyed.
,
Jan 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/433b648885b0b665053a43f8cd603dc52ccb44c2 commit 433b648885b0b665053a43f8cd603dc52ccb44c2 Author: zhaobin <zhaobin@chromium.org> Date: Mon Jan 30 19:55:38 2017 [Media Router] Erase PresentationFrame entry when corresponding RenderFrameHost is gone In PresentationServiceDelegateImpl::RemoveObserver(), we should also erase PresentationFrame entry from PresentationFrameManager. BUG= 685879 Review-Url: https://codereview.chromium.org/2657613007 Cr-Commit-Position: refs/heads/master@{#447040} [modify] https://crrev.com/433b648885b0b665053a43f8cd603dc52ccb44c2/chrome/browser/media/router/presentation_service_delegate_impl.cc
,
Jan 30 2017
zhaobin@, can this be closed? Is there an easy way to verify this on tomorrow's canary build?
,
Jan 30 2017
Derek suggests that we merge this into M57. Will verify it in tomorrow's canary and close it after merging. I think we will need to add some logging to verify this.
,
Jan 30 2017
Yes, let's file a merge request.
,
Jan 30 2017
,
Feb 1 2017
I just verified this fix in Chrome canary 58.0.2999.0 on Mac Steps are: 1. open any youtube video and cast to cast device 2. switch to a different tab 3. switch back to youtube tab, and open route I verified there was no crash
,
Feb 2 2017
,
Feb 2 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/136f664d94b8f6dce81dff132071845aad96fda6 commit 136f664d94b8f6dce81dff132071845aad96fda6 Author: Derek Cheng <imcheng@chromium.org> Date: Thu Feb 02 21:30:43 2017 [Media Router] Erase PresentationFrame entry when corresponding RenderFrameHost is gone In PresentationServiceDelegateImpl::RemoveObserver(), we should also erase PresentationFrame entry from PresentationFrameManager. BUG= 685879 Review-Url: https://codereview.chromium.org/2657613007 Cr-Commit-Position: refs/heads/master@{#447040} (cherry picked from commit 433b648885b0b665053a43f8cd603dc52ccb44c2) Review-Url: https://codereview.chromium.org/2670003004 . Cr-Commit-Position: refs/branch-heads/2987@{#282} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/136f664d94b8f6dce81dff132071845aad96fda6/chrome/browser/media/router/presentation_service_delegate_impl.cc |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by imch...@chromium.org
, Jan 27 2017