chrome/browser/media/router/presentation_service_delegate_impl.cc - Issue 2627463003: Convert MediaRouter mojom apis to intake url::Origin objects instead of strings

Unified Diff: chrome/browser/media/router/presentation_service_delegate_impl.cc

Issue 2627463003: Convert MediaRouter mojom apis to intake url::Origin objects instead of strings (Closed)

Patch Set: comment and ternary op Created 3 years, 11 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« chrome/browser/media/router/presentation_request.cc ('K') | « chrome/browser/media/router/presentation_request_unittest.cc ('k') | chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc » ('j') | chrome/browser/ui/webui/media_router/media_router_ui.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/media/router/presentation_service_delegate_impl.cc

diff --git a/chrome/browser/media/router/presentation_service_delegate_impl.cc b/chrome/browser/media/router/presentation_service_delegate_impl.cc

index ba385dcebbcfb22ffd203b33dc3086aeb4549f2c..f7541b316e5ed530053e769c5d4369d6aed82bae 100644

--- a/chrome/browser/media/router/presentation_service_delegate_impl.cc

+++ b/chrome/browser/media/router/presentation_service_delegate_impl.cc

@@ -57,14 +57,12 @@ RenderFrameHostId GetRenderFrameHostId(RenderFrameHost* render_frame_host) {

// Gets the last committed URL for the render frame specified by

// |render_frame_host_id|.

-GURL GetLastCommittedURLForFrame(RenderFrameHostId render_frame_host_id) {

+url::Origin GetLastCommittedURLForFrame(

+ RenderFrameHostId render_frame_host_id) {

RenderFrameHost* render_frame_host = RenderFrameHost::FromID(

render_frame_host_id.first, render_frame_host_id.second);

- if (!render_frame_host)

- return GURL();

- // TODO(crbug.com/632623): Use url::Origin in place of GURL for origins

- return render_frame_host->GetLastCommittedOrigin().GetURL();

+ return render_frame_host ? render_frame_host->GetLastCommittedOrigin()

dcheng 2017/01/26 04:16:32 I was trying to figure out why |render_frame_host|

steimel 2017/01/27 00:28:59 Just had some offline discussion with mfoltz@ and

imcheng 2017/01/27 00:55:06 I think what dcheng@ is saying is the correspondin

imcheng 2017/01/27 01:04:36 Filed crbug.com/685879

dcheng 2017/01/27 09:11:11 So it sounds like for now, we still need the NULL

So it sounds like for now, we still need the NULL check. But we can annotate this with TODO(crbug.com/685879) to clean it up once we're correctly removing entries from the map.

steimel 2017/01/27 18:55:19 Hmm so what's getting cleaned up? Do you mean remo

On 2017/01/27 09:11:11, dcheng wrote: > On 2017/01/27 01:04:36, imcheng wrote: > > On 2017/01/27 00:55:06, imcheng wrote: > > > On 2017/01/27 00:28:59, steimel wrote: > > > > On 2017/01/26 04:16:32, dcheng wrote: > > > > > I was trying to figure out why |render_frame_host| can be null, but I'm > a > > > bit > > > > > confused. I guess this is needed for PresentationFrame, but I don't > > > understand > > > > > why we'd ever need to handle a case where the RenderFrameHost is gone. > > > > > > > > > > Looking at this in more detail, it seem like |presentation_frames_| is > > never > > > > > cleaned up: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/media/router/presentation... > > > > > > > > > > So maybe there's a bug here -- we need to be removing entries from the > map > > > > when > > > > > RenderFrameHosts are destroyed... > > > > > > > > Just had some offline discussion with mfoltz@ and imcheng@ regarding this. > > > There > > > > used to be a DCHECK here, but it was removed a while back for some tests > > that > > > > were using dummy ids. Those tests are no longer around, so I've re-added > the > > > > DCHECK and now assume that render_frame_host exists. Also, regarding your > > > > comment on removing entries, the entries are removed in > > RenderFrameHostImpl's > > > > dtor: > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... > > > > > > I think what dcheng@ is saying is the corresponding PresentationFrame object > > is > > > never removed from the PresentationFrameManager. > > > > > > The answer is that we used to erase the PresentationFrame entry during > > > RemoveDelegateObserver (which gets called when the RFH is about to be > > > destroyed). A recent refactoring (you will need to rebase) inadvertently > > removed > > > that statement. So this is a subtle memory leak. > > > > > > We can fix it by removing the entry in PresentationFrameManager::Reset() > > (which > > > gets called if the frame is being destroyed or is navgiating). I will file a > > bug > > > for this. > > > > Filed crbug.com/685879 > > So it sounds like for now, we still need the NULL check. But we can annotate > this with TODO(crbug.com/685879) to clean it up once we're correctly removing > entries from the map.

Hmm so what's getting cleaned up? Do you mean removing the DCHECK? I'm not sure that it'd be a good idea to remove it, even after removing entries from the map

imcheng 2017/01/27 19:02:50 I do not think the nullptr check is necessary. To

On 2017/01/27 18:55:19, steimel wrote: > On 2017/01/27 09:11:11, dcheng wrote: > > On 2017/01/27 01:04:36, imcheng wrote: > > > On 2017/01/27 00:55:06, imcheng wrote: > > > > On 2017/01/27 00:28:59, steimel wrote: > > > > > On 2017/01/26 04:16:32, dcheng wrote: > > > > > > I was trying to figure out why |render_frame_host| can be null, but > I'm > > a > > > > bit > > > > > > confused. I guess this is needed for PresentationFrame, but I don't > > > > understand > > > > > > why we'd ever need to handle a case where the RenderFrameHost is gone. > > > > > > > > > > > > Looking at this in more detail, it seem like |presentation_frames_| is > > > never > > > > > > cleaned up: > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/media/router/presentation... > > > > > > > > > > > > So maybe there's a bug here -- we need to be removing entries from the > > map > > > > > when > > > > > > RenderFrameHosts are destroyed... > > > > > > > > > > Just had some offline discussion with mfoltz@ and imcheng@ regarding > this. > > > > There > > > > > used to be a DCHECK here, but it was removed a while back for some tests > > > that > > > > > were using dummy ids. Those tests are no longer around, so I've re-added > > the > > > > > DCHECK and now assume that render_frame_host exists. Also, regarding > your > > > > > comment on removing entries, the entries are removed in > > > RenderFrameHostImpl's > > > > > dtor: > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... > > > > > > > > I think what dcheng@ is saying is the corresponding PresentationFrame > object > > > is > > > > never removed from the PresentationFrameManager. > > > > > > > > The answer is that we used to erase the PresentationFrame entry during > > > > RemoveDelegateObserver (which gets called when the RFH is about to be > > > > destroyed). A recent refactoring (you will need to rebase) inadvertently > > > removed > > > > that statement. So this is a subtle memory leak. > > > > > > > > We can fix it by removing the entry in PresentationFrameManager::Reset() > > > (which > > > > gets called if the frame is being destroyed or is navgiating). I will file > a > > > bug > > > > for this. > > > > > > Filed crbug.com/685879 > > > > So it sounds like for now, we still need the NULL check. But we can annotate > > this with TODO(crbug.com/685879) to clean it up once we're correctly removing > > entries from the map. > > Hmm so what's getting cleaned up? Do you mean removing the DCHECK? I'm not sure > that it'd be a good idea to remove it, even after removing entries from the map

I do not think the nullptr check is necessary. To clarify, the bug is that we are not cleaning up PresentationFrame objects for dead RFHs. But they are never referenced again after the RFH is dead. Also, we never invoke PresentationServiceDelegateImpl / PresentationFrameManager / PresentationFrame methods with a dead RFH ID. So this method should never get invoked with a dead RFH ID.

+ : url::Origin();

}

// Observes messages originating from the MediaSink connected to a MediaRoute

@@ -236,7 +234,7 @@ bool PresentationFrame::SetScreenAvailabilityListener(

sinks_observer.reset(new PresentationMediaSinksObserver(

router_, listener, source,

- GetLastCommittedURLForFrame(render_frame_host_id_).GetOrigin()));

+ GetLastCommittedURLForFrame(render_frame_host_id_)));

if (!sinks_observer->Init()) {

url_to_sinks_observer_.erase(source.id());

@@ -560,9 +558,10 @@ void PresentationFrameManager::SetDefaultPresentationUrls(

ClearDefaultPresentationRequest();

} else {

DCHECK(!callback.is_null());

- GURL frame_url(GetLastCommittedURLForFrame(render_frame_host_id));

+ const auto& frame_origin =

+ GetLastCommittedURLForFrame(render_frame_host_id);

PresentationRequest request(render_frame_host_id, default_presentation_urls,

- frame_url);

+ frame_origin);

default_presentation_started_callback_ = callback;

SetDefaultPresentationRequest(request);

}

@@ -831,8 +830,8 @@ void PresentationServiceDelegateImpl::JoinSession(

return;

}

- const url::Origin& origin = url::Origin(GetLastCommittedURLForFrame(

- RenderFrameHostId(render_process_id, render_frame_id)));

+ const url::Origin& origin = GetLastCommittedURLForFrame(

+ RenderFrameHostId(render_process_id, render_frame_id));

#if !defined(OS_ANDROID)

if (IsAutoJoinPresentationId(presentation_id) &&

@@ -853,7 +852,7 @@ void PresentationServiceDelegateImpl::JoinSession(

weak_factory_.GetWeakPtr(), render_process_id, render_frame_id,

presentation_url, presentation_id, success_cb, error_cb));

router_->JoinRoute(MediaSourceForPresentationUrl(presentation_url).id(),

- presentation_id, origin.GetURL(), web_contents_,

+ presentation_id, origin, web_contents_,

route_response_callbacks, base::TimeDelta(), incognito);

}