|
|
Created:
3 years, 6 months ago by arthursonzogni Modified:
3 years, 6 months ago 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. |
DescriptionPlzNavigate: 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...) #
Messages
Total messages: 67 (53 generated)
Description was changed from ========== 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). ========== to ========== 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 ==========
The CQ bit was checked by arthursonzogni@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...
Description was changed from ========== 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 ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was unchecked by arthursonzogni@chromium.org
The CQ bit was checked by arthursonzogni@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 checked by arthursonzogni@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_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:50001) has been deleted
Patchset #3 (id:30001) has been deleted
Patchset #3 (id:70001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by arthursonzogni@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by arthursonzogni@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by arthursonzogni@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_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by arthursonzogni@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...
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:110001) has been deleted
Patchset #1 (id:90001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
arthursonzogni@chromium.org changed reviewers: + clamy@chromium.org
Hi Camille, I just wanted your thoughts about the whole approach. It looks like the StreamOverrideParameters may be deleted after the RenderFrameImpl that created it. Does it looks strange to you? There is a few tests that exhibit this behavior. I still need to investigate more to know if it could happens in practice. I fixed the problem by using the RenderFrameObserver, but I am not very proud of it :) If you are okay with this patch, I probably still need to convert struct StreamOverrideParameters into a class.
Thanks! Concerning the fact that RenderFrameImpl can be gone before the StreamOverrideParams, that seems quite possible to me. I certainly wouldn't assume that the StreamOverrideParams are deleted first. https://codereview.chromium.org/2932453002/diff/150001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2932453002/diff/150001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:640: const StreamHandle* stream_handle() const { return stream_handle_.get(); } This is only used in tests right? If so, call it stream_handle_for_testing. It makes it clear that it is only meant to be used in tests, and it is also enforced at compile time. https://codereview.chromium.org/2932453002/diff/150001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2932453002/diff/150001/content/renderer/rende... content/renderer/render_frame_impl.cc:5169: stream_handle_deleter->OnStreamHandleConsumed(url); Why not binding the RenderFrameImpl directly with this RenderFrameImpl as a weak_ptr?
The CQ bit was checked by arthursonzogni@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.
Thanks Camille! Here is a new patch with your suggestions. https://codereview.chromium.org/2932453002/diff/150001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2932453002/diff/150001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:640: const StreamHandle* stream_handle() const { return stream_handle_.get(); } On 2017/06/09 14:57:42, clamy (slow) wrote: > This is only used in tests right? If so, call it stream_handle_for_testing. It > makes it clear that it is only meant to be used in tests, and it is also > enforced at compile time. Right! Done. I didn't know it would be enforced at compile time. Nice! https://codereview.chromium.org/2932453002/diff/150001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2932453002/diff/150001/content/renderer/rende... content/renderer/render_frame_impl.cc:5169: stream_handle_deleter->OnStreamHandleConsumed(url); On 2017/06/09 14:57:42, clamy (slow) wrote: > Why not binding the RenderFrameImpl directly with this RenderFrameImpl as a > weak_ptr? I thought base::weak_ptr was sort of the the same thing as std::weak_ptr. That meant I would need the RenderFrameHostImpl lifetime to be managed by an std::shared_ptr equivalent. So yes, base::weak_ptr is the right tool here thanks!
clamy@chromium.org changed reviewers: + nasko@chromium.org
Thanks! Lgtm. +nasko@: PTAL https://codereview.chromium.org/2932453002/diff/170001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2932453002/diff/170001/content/common/frame_m... content/common/frame_messages.h:678: // Let the browser knows a StreamHandle has been consumed and can be released. nit: s/knows/know https://codereview.chromium.org/2932453002/diff/170001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2932453002/diff/170001/content/renderer/rende... content/renderer/render_frame_impl.cc:5137: // PlzNavigate: Notify the browser that it can released its |stream_handle_| nit: no PlzNavigate needed (it's a full PlzNavigate function). I would prefer the comment as: // Used to notify the browser that it can release its... object isn't used anymore. TODO(clamy): Remove this when we switch to Mojo streams.
Overall the CL looks good, however I have one question about process crash. https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:3188: // Released in OnStreamHandleConsumed(). Shouldn't we also release the stream handle if the renderer process crashes? In that case, there will be no way to consume the stream and we always navigate again any newly recreated process. https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:4057: if (stream_handle_ && stream_handle_->GetURL() == stream_url) { nit: No need for {}
https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:3188: // Released in OnStreamHandleConsumed(). On 2017/06/12 23:05:04, nasko (slow) wrote: > Shouldn't we also release the stream handle if the renderer process crashes? In > that case, there will be no way to consume the stream and we always navigate > again any newly recreated process. Yes, but I do not know if it's worth knowing that the previous StreamHandle is removed/replaced with each new navigations. Is there any way to get notified of the RenderFrameImpl's crashes? I think I can use RenderFrameHostImpl::CreateRenderFrame() which get called upon re-creation of the renderer process, but it might not be ideal. https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:4057: if (stream_handle_ && stream_handle_->GetURL() == stream_url) { On 2017/06/12 23:05:04, nasko (slow) wrote: > nit: No need for {} Done. https://codereview.chromium.org/2932453002/diff/170001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2932453002/diff/170001/content/common/frame_m... content/common/frame_messages.h:678: // Let the browser knows a StreamHandle has been consumed and can be released. On 2017/06/12 15:15:47, clamy (slow) wrote: > nit: s/knows/know Done. https://codereview.chromium.org/2932453002/diff/170001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2932453002/diff/170001/content/renderer/rende... content/renderer/render_frame_impl.cc:5137: // PlzNavigate: Notify the browser that it can released its |stream_handle_| On 2017/06/12 15:15:47, clamy (slow) wrote: > nit: no PlzNavigate needed (it's a full PlzNavigate function). > I would prefer the comment as: > // Used to notify the browser that it can release its... object isn't used > anymore. > TODO(clamy): Remove this when we switch to Mojo streams. Done.
https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2932453002/diff/170001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:3188: // Released in OnStreamHandleConsumed(). On 2017/06/13 09:21:48, arthursonzogni wrote: > On 2017/06/12 23:05:04, nasko (slow) wrote: > > Shouldn't we also release the stream handle if the renderer process crashes? > In > > that case, there will be no way to consume the stream and we always navigate > > again any newly recreated process. > > Yes, but I do not know if it's worth knowing that the previous StreamHandle is > removed/replaced with each new navigations. It is just clean code to release the stream handle when the renderer process crashes. We can easily reason about resource usage. > Is there any way to get notified of the RenderFrameImpl's crashes? I think I can > use RenderFrameHostImpl::CreateRenderFrame() which get called upon re-creation > of the renderer process, but it might not be ideal. RenderFrameHostImpl::RenderProcessGone is what you need.
On 2017/06/13 13:51:09, nasko (slow) wrote: > RenderFrameHostImpl::RenderProcessGone is what you need. I've been searching for a long time yet ;-) Thanks! Here is a new patch. It release |stream_handle_| on RenderFrameImpl's crashes.
The CQ bit was checked by arthursonzogni@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...
Hi Nasko, Here is a new test. It checks that the StreamHandle is released when the renderer crashes.
LGTM with a couple of nits. https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right): https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl_browsertest.cc:391: EXPECT_TRUE(NavigateToURL(shell(), GURL("about:blank"))); Why not navigate to a real URL, such as GetURL("/title1.html")? https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl_browsertest.cc:402: EXPECT_TRUE(NavigateToURL(shell(), GURL("data:text/html,hello"))); Same here, I think we are better off using an http(s) URL instead of data. Using GetURL("/title2.html") will guarantee us that we don't do process swap, therefore we can continue monitoring the same process.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by arthursonzogni@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.
Thanks Nasko, Still LGTM? https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right): https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_... 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, nasko (slow) wrote: > Why not navigate to a real URL, such as GetURL("/title1.html")? It was because the embedded_test_server was not started. I need to update the RenderFrameHostImplBrowserTest classe... ...Done. https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl_browsertest.cc:402: EXPECT_TRUE(NavigateToURL(shell(), GURL("data:text/html,hello"))); On 2017/06/13 16:53:43, nasko (slow) wrote: > Same here, I think we are better off using an http(s) URL instead of data. Using > GetURL("/title2.html") will guarantee us that we don't do process swap, > therefore we can continue monitoring the same process. Done.
On 2017/06/14 10:30:53, arthursonzogni wrote: > Thanks Nasko, > > Still LGTM? You could start it just in that test case itself, but what you have still LGTM. > https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_... > File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right): > > https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_... > 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, nasko (slow) wrote: > > Why not navigate to a real URL, such as GetURL("/title1.html")? > > It was because the embedded_test_server was not started. > I need to update the RenderFrameHostImplBrowserTest classe... > ...Done. > > https://codereview.chromium.org/2932453002/diff/230001/content/browser/frame_... > content/browser/frame_host/render_frame_host_impl_browsertest.cc:402: > EXPECT_TRUE(NavigateToURL(shell(), GURL("data:text/html,hello"))); > On 2017/06/13 16:53:43, nasko (slow) wrote: > > Same here, I think we are better off using an http(s) URL instead of data. > Using > > GetURL("/title2.html") will guarantee us that we don't do process swap, > > therefore we can continue monitoring the same process. > > Done.
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org Link to the patchset: https://codereview.chromium.org/2932453002/#ps250001 (title: "Add real urls in tests ( rebase...)")
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": 250001, "attempt_start_ts": 1497552981399780, "parent_rev": "89f066e7d382f7f2f9e74b1afbe1fcf1649615cf", "commit_rev": "8283e2a125eac90604b4dd2f67fd34d9c9d50115"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/8283e2a125eac90604b4dd2f67fd... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:250001) as https://chromium.googlesource.com/chromium/src/+/8283e2a125eac90604b4dd2f67fd... |