New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 872803 link

Starred by 1 user
Status: Fixed
Owner: ----
Closed: Aug 15
Cc:
clamy@chromium.org
creis@chromium.org
alex...@chromium.org
arthurso...@chromium.org
█ nasko@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 784904
Blocking:
issue 848821



Sign in to add a comment

Error page commits leaks NavigationRequest/Handle

Project Member Reported by lukasza@chromium.org, Aug 9 
When RenderFrameHostImpl::TakeNavigationHandleForCommit finds out that the committed url doesn't match (can happen in XFO/deny case when we commit data:, in an error page, can probably also happen for other error pages) it will conjure a new NavigationHandleImpl and abandon the old one.  This has the following undesirable consequences:

1) the old NavigationHandleImpl leaks AFAICT - e.g. frame->GetNavigationEntryIdsPendingCommit().size() == 1

2) WebContentsObserver::ReadyToCommitNavigation will be called with a different (old) NavigationHandle than WebContentsObserver::DidFinishNavigation.

The first of the issues above is indirectly responsible for  issue 871704  (the commit timeout is cancelled when the NHI commits or is destroyed).

Repro test:

    IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
                           XFrameOptionsDenyShouldntLeakNavigationHandles) {
      GURL main_url(embedded_test_server()->GetURL("foo.com", "/frame_tree/page_with_one_frame.html"));
      EXPECT_TRUE(NavigateToURL(shell(), main_url));

      GURL xfo_url(embedded_test_server()->GetURL("xfo.com", "/x-frame-options-deny.html"));
      EXPECT_TRUE(NavigateIframeToURL(shell()->web_contents(), "child0", xfo_url));

      // THIS ASSERTION WILL FAIL - THERE WILL STILL BE 1 PENDING COMMIT HERE.
      RenderFrameHostImpl* frame = static_cast<RenderFrameHostImpl*>(shell()->web_contents()->GetAllFrames()[1]);
      EXPECT_EQ(0u, frame->GetNavigationEntryIdsPendingCommit().size());

      // TODO: Verify that ReadyToCommitNavigation and DidFinishNavigation are called with the same NHI?
    }

Comment 1 by lukasza@chromium.org, Aug 9 

FWIW, this repro will probably stop working quite soon - after issue 870815 is fixed.  This means that maybe I should use something else than XFO to induce a commit with a URL that differs from the URL seen in the browser at ReadyToCommit time.  alexmos@ suggested CSP/frame-ancestors (although this would also start committing the right URL in the long-term [once handled in the browser process?]).

Comment 2 by lukasza@chromium.org, Aug 9 

alexmos@ has kindly pointed out that in the long-term (or maybe even today?) we should be able to identify the NavigationHandle/Request that is being committed using |navigation_handle->GetNavigationId()| (which is for example used as a key in |RenderFrameHostImpl::navigation_requests_|).  Unfortunately, I don't think the navigation id is currently available in the following callstack (where we incorrectly think that the commit doesn't apply to the current navigation handle because the URLs differ):

#3 0x7f5463b20bc1 content::RenderFrameHostImpl::TakeNavigationHandleForCommit()
#4 0x7f5463b1110b content::RenderFrameHostImpl::DidCommitNavigationInternal()
#5 0x7f5463b10cd8 content::RenderFrameHostImpl::DidCommitProvisionalLoad()

Comment 3 by lukasza@chromium.org, Aug 9

Blockedon: 784904
Maybe the memory leak can be fixed in the short term, but I think proper long-term fix requires matching commit IPCs to the right NavigationRequest/Handle - AFAIK this would be solved by the work tracked in issue 784904 (Merge navigation IPCs in a per-navigation Mojo interface).
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/087c060668f8d534355ebebbd04fb6abc70e6898

commit 087c060668f8d534355ebebbd04fb6abc70e6898
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Aug 10 19:37:27 2018

Avoid leaking NavigationRequest when committing an error page.

Sometimes the commit IPC really is for |navigation_request_| even when
the URL checks at the top of RFHI::TakeNavigationHandleForCommit
indicate a mismatched URL.  Examples of when this can happen are XFO
checks (see https://crbug.com/870815) and CSP/frame-ancestors checks
(see https://crbug.com/759184).

Before this CL, in the cases above |navigation_request_| would be
leaked.  This CL fixes the leak.

Bug:  872803 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:ios-simulator-full-configs;luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.perf:obbs_fyi
Change-Id: Ia09f355e69f182479386b3deeea1f7f0c8996813
Reviewed-on: https://chromium-review.googlesource.com/1169865
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582289}
[modify] https://crrev.com/087c060668f8d534355ebebbd04fb6abc70e6898/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/087c060668f8d534355ebebbd04fb6abc70e6898/content/browser/frame_host/render_frame_host_impl_browsertest.cc

Comment 5 by lukasza@chromium.org, Aug 10

Summary: Error page commits leaks NavigationRequest/Handle (was: XFO/deny leaks NavigationRequest/Handle)

Comment 6 by lukasza@chromium.org, Aug 15

Status: Fixed (was: Untriaged)
►

Sign in to add a comment