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

Issue 2932453002: PlzNavigate: Release StreamHandle. (Closed)

Created:
3 years, 6 months ago by arthursonzogni
Modified:
3 years, 6 months ago
Reviewers:
clamy, nasko
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, mlamouri+watch-content_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

PlzNavigate: Release StreamHandle. This CL makes the StreamHandle to be released when the renderer has finished using it. Before this CL, the StreamHandle was not released immediatly. It was released only on RenderFrameHostImpl's destruction or when a new StreamHandle replaced it (during a new navigation). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2932453002 Cr-Commit-Position: refs/heads/master@{#479806} Committed: https://chromium.googlesource.com/chromium/src/+/8283e2a125eac90604b4dd2f67fd34d9c9d50115

Patch Set 1 : Use a callback. #

Patch Set 2 : Experiment: Use a RenderFrameObserver. #

Total comments: 4

Patch Set 3 : Addressed comments (+Rebase) #

Total comments: 9

Patch Set 4 : Nit clamy@ #

Patch Set 5 : Release |stream_handle_| on Renderer crashes. #

Patch Set 6 : Add test. #

Total comments: 4

Patch Set 7 : Add real urls in tests ( rebase...) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+122 lines, -6 lines) Patch
M content/browser/frame_host/render_frame_host_impl.h View 1 2 2 chunks +8 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 4 chunks +13 lines, -2 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl_browsertest.cc View 1 2 3 4 5 6 4 chunks +75 lines, -1 line 0 comments Download
M content/child/web_url_loader_impl.h View 1 3 chunks +6 lines, -2 lines 0 comments Download
M content/child/web_url_loader_impl.cc View 1 2 3 4 5 6 2 chunks +5 lines, -1 line 0 comments Download
M content/common/frame_messages.h View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 67 (53 generated)
arthursonzogni
Hi Camille, I just wanted your thoughts about the whole approach. It looks like the ...
3 years, 6 months ago (2017-06-09 11:55:34 UTC) #37
clamy
Thanks! Concerning the fact that RenderFrameImpl can be gone before the StreamOverrideParams, that seems quite ...
3 years, 6 months ago (2017-06-09 14:57:42 UTC) #38
arthursonzogni
Thanks Camille! Here is a new patch with your suggestions. https://codereview.chromium.org/2932453002/diff/150001/content/browser/frame_host/render_frame_host_impl.h File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2932453002/diff/150001/content/browser/frame_host/render_frame_host_impl.h#newcode640 ...
3 years, 6 months ago (2017-06-12 09:30:16 UTC) #43
clamy
Thanks! Lgtm. +nasko@: PTAL https://codereview.chromium.org/2932453002/diff/170001/content/common/frame_messages.h File content/common/frame_messages.h (right): https://codereview.chromium.org/2932453002/diff/170001/content/common/frame_messages.h#newcode678 content/common/frame_messages.h:678: // Let the browser knows ...
3 years, 6 months ago (2017-06-12 15:15:47 UTC) #45
nasko
Overall the CL looks good, however I have one question about process crash. https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_host/render_frame_host_impl.cc File ...
3 years, 6 months ago (2017-06-12 23:05:04 UTC) #46
arthursonzogni
https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_host/render_frame_host_impl.cc#newcode3188 content/browser/frame_host/render_frame_host_impl.cc:3188: // Released in OnStreamHandleConsumed(). On 2017/06/12 23:05:04, nasko (slow) ...
3 years, 6 months ago (2017-06-13 09:21:48 UTC) #47
nasko
https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_host/render_frame_host_impl.cc#newcode3188 content/browser/frame_host/render_frame_host_impl.cc:3188: // Released in OnStreamHandleConsumed(). On 2017/06/13 09:21:48, arthursonzogni wrote: ...
3 years, 6 months ago (2017-06-13 13:51:09 UTC) #48
arthursonzogni
On 2017/06/13 13:51:09, nasko (slow) wrote: > RenderFrameHostImpl::RenderProcessGone is what you need. I've been searching ...
3 years, 6 months ago (2017-06-13 14:22:08 UTC) #49
arthursonzogni
Hi Nasko, Here is a new test. It checks that the StreamHandle is released when ...
3 years, 6 months ago (2017-06-13 16:49:18 UTC) #52
nasko
LGTM with a couple of nits. https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_host/render_frame_host_impl_browsertest.cc File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right): https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_host/render_frame_host_impl_browsertest.cc#newcode391 content/browser/frame_host/render_frame_host_impl_browsertest.cc:391: EXPECT_TRUE(NavigateToURL(shell(), GURL("about:blank"))); Why ...
3 years, 6 months ago (2017-06-13 16:53:43 UTC) #53
arthursonzogni
Thanks Nasko, Still LGTM? https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_host/render_frame_host_impl_browsertest.cc File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right): https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_host/render_frame_host_impl_browsertest.cc#newcode391 content/browser/frame_host/render_frame_host_impl_browsertest.cc:391: EXPECT_TRUE(NavigateToURL(shell(), GURL("about:blank"))); On 2017/06/13 16:53:43, ...
3 years, 6 months ago (2017-06-14 10:30:53 UTC) #60
nasko
On 2017/06/14 10:30:53, arthursonzogni wrote: > Thanks Nasko, > > Still LGTM? You could start ...
3 years, 6 months ago (2017-06-15 18:56:08 UTC) #61
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/2932453002/250001
3 years, 6 months ago (2017-06-15 18:56:54 UTC) #64
commit-bot: I haz the power
3 years, 6 months ago (2017-06-15 20:18:40 UTC) #67
Message was sent while issue was closed.
Committed patchset #7 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/8283e2a125eac90604b4dd2f67fd...

Powered by Google App Engine
This is Rietveld 408576698