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

Issue 609963 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 417518
issue 851882
issue 236848



Sign in to add a comment

Keep track of pending delete RenderFrameHosts

Project Member Reported by creis@chromium.org, May 6 2016

Issue description

Version: 52.0.2726.0
OS: All

RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation is failing when run with --isolate-sites-for-testing=*.is or --site-per-process.  (Same in any mode that sets UseSubframeNavigationEntries to true.)

../../content/browser/frame_host/render_frame_host_manager_browsertest.cc:1948: Failure
Value of: files.size()
  Actual: 0
Expected: 1U
Which is: 1

This happens because RenderFrameImpl::OnSwapOut only sends an UpdateState message for the frame being swapped.  The test navigates the main frame and expects a PageState to be sent for the subframe (which is detached as a result of the swap).  It fails because we don't update the subframe's FrameNavigationEntry.  (This isn't an issue in default Chrome because the UpdateState message contains info for all frames in that mode.)

It's fairly easy to send the UpdateState message for any RenderFrame before its deleted, but that's only part of the problem.  In the browser process, we call FrameTreeNode::ResetForNewProcess at commit time and delete all subframe FrameTreeNodes and RenderFrameHosts, before giving them a chance to run their unload handlers or send UpdateState IPCs.  This means no one is listening in the browser process when the subframe's UpdateState IPC arrives, so the test continues to fail.

In a practical sense, this is fairly low impact to the user-- only the last 1 second of changes to a subframe's PageState are lost if its parent navigates cross-process.  (We batch up and send UpdateState changes after 1 second, so this will rarely impact users.)

However, we've known that the ResetForNewProcess issue could lead to problems like this, and we should investigate ways to deal with it.
 

Comment 1 by creis@chromium.org, May 6 2016

Some ideas considered by Nick and Nasko:

1) We could keep the FTNs and RFHs around in some kind of flattened pending delete state on FrameTree, until we either hear their Ack or the timeout occurs.

2) We could keep the list of children FTNs on RFH rather than FTN, such that they don't go away until the RFH does.  We'll still have to mark all the RFHs in that tree as pending deletion, and we'll have to deal with parents that send Acks before their children.

3) We could leave the subframe FTNs and RFHs around until their Acks are received or the timeout fires.  This seems dangerous, as it mixes current and former children FTNs in the same list.

4) We could avoid depending on RFH and FTN for anything that needs to be done after unload.  Here, we could use something like a Mojo connection between the document in the renderer and the FrameNavigationEntry, allowing it to send an UpdateState even if the RFH is gone.

Unclear yet which direction to pursue.

Comment 2 by creis@chromium.org, May 6 2016

Blocking: 236848 417518
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 20 2016

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

commit 8aa4ed83d329e038980afa0fd10d880415b853a9
Author: creis <creis@chromium.org>
Date: Mon Jun 20 23:24:04 2016

Remove RenderFrameImpl::is_detaching_, since it appears no longer needed.

ajwong@ added this in r219965 when making RFHs routable, without an
explanation at the time.
https://codereview.chromium.org/22876014/diff/76001/content/renderer/render_frame_impl.cc

ajwong@ later added a comment in r234992 about how it was necessary
to prevent observers from sending IPC messages after detach:
https://codereview.chromium.org/67313010/diff/160001/content/renderer/render_frame_impl.cc

nasko@ preserved this comment in r244023 when switching us over to
always have subframe RFHs:
https://codereview.chromium.org/117603002/diff/960001/content/renderer/render_frame_impl.cc

However, gcasto removed the observer behavior in r320838.  Since
that CL, observers have been able to send IPCs during detach and
(apparently) almost nothing is disabled by is_detaching_:
https://codereview.chromium.org/1014703003/diff/20001/content/renderer/render_frame_impl.cc

As a result, this appears to be mostly dead code at this point,
for over a year.  Since I'd like to add more IPCs after detach,
I'm removing this restriction.

BUG=609963
TEST=Existing tests still pass.

Review-Url: https://codereview.chromium.org/2073413002
Cr-Commit-Position: refs/heads/master@{#400829}

[modify] https://crrev.com/8aa4ed83d329e038980afa0fd10d880415b853a9/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/8aa4ed83d329e038980afa0fd10d880415b853a9/content/renderer/render_frame_impl.h

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 22 2016

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

commit dab7c8f09b323e70dbcd457963e84d35276e0cea
Author: creis <creis@chromium.org>
Date: Wed Jun 22 03:41:46 2016

Cache RenderFrameHost's parent so it doesn't change over time.

RenderFrameHost::GetParent() should always return the same value
for a given frame.  However, we will soon allow RenderFrameHosts
to continue to exist briefly in the background in a pending
deletion state, such that we can wait for their unload handlers,
PageState, etc.  In these cases, the parent FrameTreeNode will
have already moved to a new current RenderFrameHost.

Rather than asking the parent FrameTreeNode, just cache the parent
RenderFrameHost in the child.

BUG=609963
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2086083003
Cr-Commit-Position: refs/heads/master@{#401198}

[modify] https://crrev.com/dab7c8f09b323e70dbcd457963e84d35276e0cea/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/dab7c8f09b323e70dbcd457963e84d35276e0cea/content/browser/frame_host/render_frame_host_impl.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 24 2016

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

commit 75504cc6c988e7765a7c61e7c0251b9886cd84dc
Author: creis <creis@chromium.org>
Date: Fri Jun 24 19:20:07 2016

Make RestoreSubframeFileAccessForHistoryNavigation pass in OOPIF modes.

Fixing the test properly will require tracking FrameTreeNodes that are
pending deletion, so that we can hear UpdateState messages from
subframes after doing a cross-process navigation in their ancestor.

Since we're only losing the last second of PageState changes at the
moment, we can work around this for now to let us proceed with moving
to the new navigation path.  Adding an in-page navigation ensures that
the PageState arrives in the browser process, letting the test pass.

BUG=609963
TEST=RestoreSubframeFileAccessForHistoryNavigation passes in
     --isolate-sites-for-testing=*.is or --site-per-process.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2093123002
Cr-Commit-Position: refs/heads/master@{#401924}

[modify] https://crrev.com/75504cc6c988e7765a7c61e7c0251b9886cd84dc/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/75504cc6c988e7765a7c61e7c0251b9886cd84dc/testing/buildbot/filters/isolate-extensions.content_browsertests.filter
[modify] https://crrev.com/75504cc6c988e7765a7c61e7c0251b9886cd84dc/testing/buildbot/filters/site-per-process.content_browsertests.filter

Comment 6 by creis@chromium.org, Sep 12 2017

Cc: lukasza@chromium.org creis@chromium.org
Owner: ----
Status: Available (was: Assigned)
Summary: Keep track of pending delete RenderFrameHosts (was: RestoreSubframeFileAccessForHistoryNavigation failing in OOPIF modes due to missing RFH)
It's unclear if this is causing issues in practice, but lukasza@ found it may be useful for issue 763549.

Comment 7 by laforge@google.com, Nov 7 2017

Components: Internals>Network>Service

Comment 8 by laforge@google.com, Nov 7 2017

Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.

Comment 9 by jam@chromium.org, Nov 7 2017

Components: -Internals>Services>Network
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 14 2018

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

commit 1bfc151cd3805a81e6488dd7b4cd65fd08f3584e
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Jun 14 20:49:51 2018

Delay subframe process shutdown to allow subframe unload handlers to execute.

Currently, if a page with an OOPIF navigates cross-process, we
immediately destroy the OOPIF's RenderFrameHost when its parent
commits (see FrameTreeNode::ResetForNewProcess()).  The RFH
destruction triggers a message to the renderer to delete the
RenderFrame, which also causes it to run its unload handler.  However,
if the subframe was the last active frame in its process, the process
will proceed to shut down without waiting for the unload handler to
run at all, and in such a case, the subframe unload handler won't
really be able to do anything, including termination pings.

This CL fixes this by delaying subframe process shutdown in such
situations by one second (which matches the existing unload timeout
that we use for pending delete RenderFrameHosts).

Bug:  852204 , 609963
Change-Id: I142ef4e6f59df465c6ed213df02daf492e989c2d
Reviewed-on: https://chromium-review.googlesource.com/1094917
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567395}
[modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/content/public/browser/render_process_host.h
[modify] https://crrev.com/1bfc151cd3805a81e6488dd7b4cd65fd08f3584e/tools/metrics/histograms/histograms.xml

Comment 11 by creis@chromium.org, Jun 15 2018

Cc: alex...@chromium.org
Labels: -Pri-2 -OS-All M-69 Target-69 OS-Chrome OS-Linux OS-Mac OS-Windows Pri-1
Owner: nasko@chromium.org
Status: Started (was: Available)
Thanks Alex!  Temporarily assigning to Nasko since he's working on plan #2 from comment 1 in the following CLs:
https://chromium-review.googlesource.com/c/chromium/src/+/1098562
https://chromium-review.googlesource.com/c/chromium/src/+/1101297

Nasko, feel free to reassign if needed.

Comment 12 by creis@chromium.org, Jun 15 2018

Blocking: 851882
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 18 2018

Labels: merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/16e6577a8fa81b76b044373c5116f73fc89d09f6

commit 16e6577a8fa81b76b044373c5116f73fc89d09f6
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Mon Jun 18 21:31:26 2018

Delay subframe process shutdown to allow subframe unload handlers to execute. (Merge to M68)

Currently, if a page with an OOPIF navigates cross-process, we
immediately destroy the OOPIF's RenderFrameHost when its parent
commits (see FrameTreeNode::ResetForNewProcess()).  The RFH
destruction triggers a message to the renderer to delete the
RenderFrame, which also causes it to run its unload handler.  However,
if the subframe was the last active frame in its process, the process
will proceed to shut down without waiting for the unload handler to
run at all, and in such a case, the subframe unload handler won't
really be able to do anything, including termination pings.

This CL fixes this by delaying subframe process shutdown in such
situations by one second (which matches the existing unload timeout
that we use for pending delete RenderFrameHosts).

TBR=alexmos@chromium.org

(cherry picked from commit 1bfc151cd3805a81e6488dd7b4cd65fd08f3584e)

Bug:  852204 , 609963
Change-Id: I142ef4e6f59df465c6ed213df02daf492e989c2d
Reviewed-on: https://chromium-review.googlesource.com/1094917
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#567395}
Reviewed-on: https://chromium-review.googlesource.com/1105179
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#422}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/content/public/browser/render_process_host.h
[modify] https://crrev.com/16e6577a8fa81b76b044373c5116f73fc89d09f6/tools/metrics/histograms/histograms.xml

Project Member

Comment 14 by bugdroid1@chromium.org, Jun 22 2018

Labels: merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf

commit 92b72c7ca498e61a6e7afacb300b0246a0d5b9cf
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Jun 22 00:30:25 2018

Delay subframe process shutdown to allow subframe unload handlers to execute. (Merge to M67)

Currently, if a page with an OOPIF navigates cross-process, we
immediately destroy the OOPIF's RenderFrameHost when its parent
commits (see FrameTreeNode::ResetForNewProcess()).  The RFH
destruction triggers a message to the renderer to delete the
RenderFrame, which also causes it to run its unload handler.  However,
if the subframe was the last active frame in its process, the process
will proceed to shut down without waiting for the unload handler to
run at all, and in such a case, the subframe unload handler won't
really be able to do anything, including termination pings.

This CL fixes this by delaying subframe process shutdown in such
situations by one second (which matches the existing unload timeout
that we use for pending delete RenderFrameHosts).

TBR=alexmos@chromium.org

(cherry picked from commit 1bfc151cd3805a81e6488dd7b4cd65fd08f3584e)

Bug:  852204 , 609963
Change-Id: I142ef4e6f59df465c6ed213df02daf492e989c2d
Reviewed-on: https://chromium-review.googlesource.com/1094917
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#567395}
Reviewed-on: https://chromium-review.googlesource.com/1111331
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#789}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/content/public/browser/render_process_host.h
[modify] https://crrev.com/92b72c7ca498e61a6e7afacb300b0246a0d5b9cf/tools/metrics/histograms/histograms.xml

Owner: arthurso...@chromium.org
Nasko, Arthur and I chatted today, and Arthur graciously agreed to keep pushing the plan from comment #11 forward while Nasko is on vacation.  
I have a WIP patch here:
https://chromium-review.googlesource.com/c/chromium/src/+/1122629
This is a continuation of Nasko's patch, but with all the tests fixed except one:
https://chromium-review.googlesource.com/c/chromium/src/+/1101297/3

The remaining test is:
```
  SitePerProcessBrowserTest.ParentOriginDoesNotChangeInUnloadHandler
  // Ensure that after a main frame with an OOPIF is navigated cross-site, the
  // unload handler in the OOPIF sees correct main frame origin, namely the old
  // and not the new origin.  See https://crbug.com/825283.
```

When navigating from A(B) to C, when B executes its unload handler. Its parents (WebRemoteFrame) origin is now C instead of A.

Doing it correctly was done in:
https://chromium-review.googlesource.com/c/chromium/src/+/984729
In NavigationImpl::DidNavigate(), removing the iframes's RenderFrameHosts was done before calling FrameTreeNode::SetCurrentOrigin(). They are no more removed, so this test regressed.

This one looks harder to fix. I am not really sure how to fix it.
Project Member

Comment 17 by bugdroid1@chromium.org, Aug 20

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

commit 4973b39587712fa4f01c88efc9069ffa1aa43421
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Aug 20 18:14:45 2018

Remove renderer-initiated Shutdown.

The browser process is the sole responsible for shutting down its
renderer processes. Surprisingly, there was some logic in the renderer
process to request self shutdown.

There were race conditions. For instance:
1) The renderer process requests to shutdown itself.
2) The browser process accepts the request, but don't update any of its
   internal state.
3) The browser process reuse that process.
4) The renderer process shutdown itself.

The CL removes the capability for a renderer process to shutdown
itself.

This CL is part of https://crbug.com/609963.
This CL may also fix bug https://crbug.com/873541.

Bug: 873541, 609963,  535246 
Change-Id: Idd498497acc6106fe8c87e994239bbde9b3453c3
Reviewed-on: https://chromium-review.googlesource.com/1174713
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584502}
[modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/browser/renderer_host/render_process_host_browsertest.cc
[modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/common/renderer_host.mojom
[modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/public/browser/render_process_host_observer.h
[modify] https://crrev.com/4973b39587712fa4f01c88efc9069ffa1aa43421/content/renderer/render_thread_impl.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Aug 23

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

commit 22e314b01f55988d41b17f831c313cb82b3f1d4d
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Thu Aug 23 13:55:56 2018

Fix fullscreen not removed for frame in pending deletion.

RenderFrameHostImpl are removed from the set of frame in fullscreen when
they are deleted. It means they are still in this set in between their
swap out and their deletion. As a result this test was flaky on slow
devices: WebContentsImplbrowserTest.NotifyFullscreenAcquired_Navigate

This CL:
 * Fix this issue by removing the frame immediatly when it is swapped
   out.
 * Renable the flaky test on Android.
 * Add a regression test.

Bug:  855018 , 609963
Change-Id: Ic858acd462cd8f80f39b76c09d5fbd8f9c3110b1
Reviewed-on: https://chromium-review.googlesource.com/1181433
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585473}
[modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/test/content_browser_test_utils_internal.cc
[modify] https://crrev.com/22e314b01f55988d41b17f831c313cb82b3f1d4d/content/test/content_browser_test_utils_internal.h

Project Member

Comment 19 by bugdroid1@chromium.org, Sep 5

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

commit c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Wed Sep 05 08:51:33 2018

Remove reference to AddRefProcess/ReleaseProcess() in the renderer.

The browser process is the sole responsible for shutting down its
renderer processes. Surprisingly, there was some logic in the renderer
process to request self shutdown.

Previous CL disabled renderer process initiated shutdown. This CL
removes associated logic to increment/decrement the reference counter.

Bug: 873541, 609963,  535246 
Change-Id: I06bc6670d84dcad597bf98eea6c3e07242f341b6
Reviewed-on: https://chromium-review.googlesource.com/1177748
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588799}
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/child/child_process.h
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/render_process_impl.cc
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/render_process_impl.h
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/render_widget.cc
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/renderer_blink_platform_impl.cc
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/renderer_blink_platform_impl.h
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/service_worker/embedded_worker_instance_client_impl.cc
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/service_worker/embedded_worker_instance_client_impl.h
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/shared_worker/embedded_shared_worker_stub.cc
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/content/renderer/shared_worker/embedded_shared_worker_stub.h
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/third_party/blink/public/platform/platform.h
[modify] https://crrev.com/c4f8dee31d67c8abf89ad4f43e9b52bd2d56d75a/third_party/blink/renderer/platform/blob/blob_bytes_provider.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Sep 17

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

commit 929d29fa8985277d1846d9a96807ec1e1b862edd
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Sep 17 14:19:44 2018

Refactor the set of frames in fullscreen.

Instead of tracking FrameTreeNode, tracks RenderFrameImpl.
There are several reasons:

1) WebContentsImpl::FullscreenStateChanged()
   takes RenderFrameHostImpl* in input and notifies its observers
   using a RenderFrameHostImpl*. There is no real need to convert
   from/to a FrameTreeNode.

2) This function traverses the tree of FrameTreeNode, but is called in
   RenderFrameHostImpl* destructor. It means the tree is in the middle
   of modifying itself. This is hard to reason about. For instance:
   rfh->frame_tree_node()->current_frame_host() may be null here, even
   when rfh is the current frame host.
   Traversing the tree using only rfh->GetParent() is easier.

Bug: 609963, 881849
Change-Id: I2e74324f83ddb2800b4dd177b561ff987c2c0a5f
Reviewed-on: https://chromium-review.googlesource.com/1207753
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591667}
[modify] https://crrev.com/929d29fa8985277d1846d9a96807ec1e1b862edd/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/929d29fa8985277d1846d9a96807ec1e1b862edd/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/929d29fa8985277d1846d9a96807ec1e1b862edd/content/browser/web_contents/web_contents_impl_browsertest.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Sep 21

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

commit da90ece84fb07b4c548b127580428b7f5378c78f
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Fri Sep 21 09:09:05 2018

Update GetRootRenderWidgetHostView() to explore the tree using RFH.

GetRootRenderWidgetHostView() and GetParentRenderWidget() explore the
FrameTree using FrameTreeNode. Once it is done, it returns the
RenderWidgetHostView associated with the current RenderFrameHost on
that FrameTreeNode.
The issue: there is no 1:1 association between the FrameTreeNode and the
RenderFrameHost. A FrameTreeNode, contains several RenderframeHost:
 * The current one.
 * The pending one, not displayed and used for loading a new document,
   but this one has still not committed.
 * The ones in pending deletion. Those are no more displayed and are
   in progress executing their javascript unload handlers.

Instead of exploring the tree using FrameTreeNode::parent(), explore the
tree using RenderFrameHost::GetParent(). That way, it is guaranteed the
functions will always returns the same object.

Bug: 609963
Change-Id: Ic8972180b94763bd6a831f2754d1e3c661c0bbc4
Reviewed-on: https://chromium-review.googlesource.com/1210942
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593130}
[modify] https://crrev.com/da90ece84fb07b4c548b127580428b7f5378c78f/content/browser/frame_host/cross_process_frame_connector.cc

Project Member

Comment 22 by bugdroid1@chromium.org, Oct 4

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

commit 252ae04baf912b8d734b03f29d0be98a5244a5ac
Author: Nasko Oskov <nasko@chromium.org>
Date: Thu Oct 04 21:44:12 2018

Move subframe FrameTreeNode ownership RenderFrameHost.

In order to better handle messages sent from RenderFrames in
the renderer process during unload event, we should keep
the browser side objects alive as long as possible.

This CL moves ownership of subframes to RenderFrameHost, which
can allow the subframes to live until the unload handler of the
top parent has completed running. The CL is just a mechanical
move of the ownership and does not change the lifetime of
subframes for now, which will come in a follow up CL.

Bug: 609963
Change-Id: I52456dc1bf68cb15caab527c16c96ddd13a80838
Reviewed-on: https://chromium-review.googlesource.com/c/1098562
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596869}
[modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/find_request_manager_browsertest.cc
[modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/frame_host/frame_tree.cc
[modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/frame_host/frame_tree_node.h
[modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/252ae04baf912b8d734b03f29d0be98a5244a5ac/content/browser/frame_host/render_frame_host_impl.h

Project Member

Comment 23 by bugdroid1@chromium.org, Oct 5

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

commit 97739b2d9b25f3b77e5f3ab658e2c4061926c761
Author: Nasko Oskov <nasko@chromium.org>
Date: Fri Oct 05 21:14:18 2018

Use the RenderFrameHost SiteInstance when creating subframes.

This is a follow up on a comment from
https://chromium-review.googlesource.com/c/chromium/src/+/1098562

Bug: 609963
Change-Id: I8ba97bcbb8961f1dbacd4d3fe5c1766c44c20042
Reviewed-on: https://chromium-review.googlesource.com/c/1265023
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597299}
[modify] https://crrev.com/97739b2d9b25f3b77e5f3ab658e2c4061926c761/content/browser/frame_host/render_frame_host_impl.cc

Project Member

Comment 24 by bugdroid1@chromium.org, Oct 11

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

commit 969a794e014a8851806cc6792d7b453a070c1de7
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Thu Oct 11 08:56:09 2018

Replace FTN::IsDescendantOf by RFH::IsDescendantOf

A FrameTreeNode hosts several RenderFrameHost like:
 * The current RenderFrameHost.
 * The future one that is loading but hasn't committed yet.
 * The ones pending deletion.

On its turn a RenderFrameHost contains several children frames, represented
by FrameTreeNode.

This CL replace FTN::IsDescendantOf by RFH::IsDescendantOf.
  * The new form of IsDescendantOf is used in follow up CLs.
  * In the case of one RenderFrameHost being pending deletion, the meaning is a
    bit different. For instance during a navigation from A(B) to C. Both
    RenderFrameHost A and C are on the same FrameTreeNode. The
    FrameTreeNode of B is a child of the FrameTreeNode of C, but B is
    not a child of C, only a child of A.

Add a content/public API so that embedders won't have to manually
iterate on RenderFrameHost::GetParent() themselves to get this
information. This will be used in a follow-up CL.

Bug: 609963
Change-Id: Iedb4a7acf17c7782cd9e91dd529a2736c1fd51d4
Reviewed-on: https://chromium-review.googlesource.com/c/1269874
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598702}
[modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/browser/frame_host/frame_tree_node.h
[modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/969a794e014a8851806cc6792d7b453a070c1de7/content/public/browser/render_frame_host.h

Project Member

Comment 25 by bugdroid1@chromium.org, Oct 11

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

commit 3bcb8b871a8a6adb3db213b591e830ab2ed98441
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Thu Oct 11 12:35:10 2018

Task manager: Make it work with iframes in pending deletion.

This CL is a sub-part of: [Don't delete subframes on CommitPending]
(https://chromium-review.googlesource.com/c/chromium/src/+/1122629/18)
RenderFrameHosts (subframes) are allowed to stay alive longer in the
background, the time needed for them to execute their unload handler.

The WebContentsTaskProvider needs to stop tracking all the frames in
pending deletion, not only the navigating one, but also its subframes.

Not doing it properly triggers a DCHECK:
---
// Whenever we have a task, we should have a main frame site instance.
DCHECK(tasks_by_frames_.empty() == (main_frame_site_instance_ == nullptr));
---
With tasks_by_frames_ not being empty, but main_frame_site_instance being
null after removing the old_frame in RenderFrameHostChanged.


Bug: 609963
Change-Id: I6e4ad45fb0d47ab378396e9ea1ad97bfc27c9c31
Reviewed-on: https://chromium-review.googlesource.com/c/1163502
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598732}
[modify] https://crrev.com/3bcb8b871a8a6adb3db213b591e830ab2ed98441/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Oct 11

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

commit 5e34dd12472f056e7f8833da88c1a3c412d187a5
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Thu Oct 11 16:04:56 2018

FileSelectHelper: Don't give file access to a frame in pending deletion.

This is a prerequisite to:
https://chromium-review.googlesource.com/c/chromium/src/+/1122629

When a file chooser dialog is opened for an active iframe, but is closed
when the iframe is in pending deletion, it should not continue.

Bug: 609963
Change-Id: I897258f642e812de4b3abf117a2a83815568aba8
Reviewed-on: https://chromium-review.googlesource.com/c/1253783
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598785}
[modify] https://crrev.com/5e34dd12472f056e7f8833da88c1a3c412d187a5/chrome/browser/file_select_helper.cc

Cc: dgozman@chromium.org
Reading through two last patches, I am concerned that current WebContentsObserver API is becoming unclear. I think it's hard to understand that RenderFrameHostChanged implies all children are pending deletion, and that client might need to do something about it.

We might have some tests which fail for _some_ components, but there can easily be many related hidden bugs, which we just don't know about. Should we consider some other way?

It would be much more convenient, for example, if RenderFrameHostChanged would be immediately followed (or preceeded) by FrameDeleted on all the child frames. Even if the RFH objects won't be deleted immediately - as a client of this API, I don't really care. WDYT? Maybe something similar?
Project Member

Comment 28 by bugdroid1@chromium.org, Oct 15

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

commit 06b336896d5384a8df0b3affc2424a97ce372759
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Oct 15 17:56:52 2018

FindRequestManager: Stop tracking subframe.

This is a prerequisite for:
https://chromium-review.googlesource.com/c/chromium/src/+/1122629
It makes subframes to stay alive after the navigation commit.

Find-in-page needs to stop tracking these frames immediately when they
are in pending deletion, not when they are deleted.

Bug: 609963
Change-Id: I9c8fa05ec3d571e79ecc22c1609ee59ff90b2b45
Reviewed-on: https://chromium-review.googlesource.com/c/1254262
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Paul Meyer <paulmeyer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599679}
[modify] https://crrev.com/06b336896d5384a8df0b3affc2424a97ce372759/content/browser/find_request_manager.cc

Cc: -nick@chromium.org
On comment 27, we had discussed having a RenderFrameHostStateChanged event to signal the move from being an active RFH to one pending deletion. This would make it very clear what is happening and can be extended in the future to account for more lifecycle state transitions. Would this make the API more clear?
I also think we should update the current documentation on WCO::RenderFrameHostChanged, which currently doesn't explain what happens to old_rfh's subframe RFHs at all.  The current behavior is that they're deleted (and will get FrameDeleted notifications) at the same time as RenderFrameHostChanged, but this has always been kind of implicit in the public API and not guaranteed AFAICT.  With Arthur's change, the FrameDeleted for subframes will just happen later, after they complete running their unload handlers.

> We might have some tests which fail for _some_ components, but there can easily be many related hidden bugs, which we just don't know about. Should we consider some other way?

We were concerned about this too, which is why Arthur audited all uses of RenderFrameHostChanged and has a spreadsheet somewhere.  There actually aren't that many uses, so it was feasible to go through them all.  I think most observers he's updating now weren't actually resulting in test failures.

> On comment 27, we had discussed having a RenderFrameHostStateChanged event to signal the move from being an active RFH to one pending deletion. This would make it very clear what is happening and can be extended in the future to account for more lifecycle state transitions. Would this make the API more clear?

Perhaps something like that will help. From my (limited) experience, most users just want to know when to stop tracking something related to RFH, and use RenderFrameHostChanged + FrameDeleted as instructed by WCO. If all of that can be replaced with a single callback, perhaps it could work out for most usecases.

> I also think we should update the current documentation on WCO::RenderFrameHostChanged, which currently doesn't explain what happens to old_rfh's subframe RFHs at all.  The current behavior is that they're deleted (and will get FrameDeleted notifications) at the same time as RenderFrameHostChanged, but this has always been kind of implicit in the public API and not guaranteed AFAICT.  With Arthur's change, the FrameDeleted for subframes will just happen later, after they complete running their unload handlers.

Out of curiosity, is it possible to keep this contract (and add comments to WCO): call FrameDeleted immediately, and delete later? Such a change definitely keeps API backwards-compatible. Would it break something else?

> We were concerned about this too, which is why Arthur audited all uses of RenderFrameHostChanged and has a spreadsheet somewhere.  There actually aren't that many uses, so it was feasible to go through them all.

Glad to hear that, thank you! I didn't find the spreadsheet, that's why I've asked.
Here is the spreadsheet I made to check all users of RenderFrameHost:
https://docs.google.com/spreadsheets/d/1T3COzhjzXXJplGE46FTniNI7fFRiBV30cUbOtJVsnv0/edit?usp=sharing

I had failing tests for two of them (task manager + find-in-page). For the other, there was not failing tests.

[Dmitry]
> From my (limited) experience, most users just want to know when to stop tracking something related to RFH, and use RenderFrameHostChanged + FrameDeleted as instructed by WCO. If all of that can be replaced with a single callback, perhaps it could work out for most usecases.

+1: Most users just want this. It would be very nice for them to have only one method to watch for instead of two. I think it might be possible to call FrameDeleted each time. What do you think? I think I am going to try this and see.

[Dmitry]
> Out of curiosity, is it possible to keep this contract (and add comments to WCO): call FrameDeleted immediately, and delete later? Such a change definitely keeps API backwards-compatible. Would it break something else?

I don't know if it breaks something, but if it is possible, I would prefer avoiding "not to tell the truth" to embedders.
Ideally I would like something like:
 * RenderFrameHostChange(_, new_host) to know when a new RenderFrameHost starts to be used. (!= created)
 * FrameDeleted(old_host) to know when it is deleted. (Always called)
 * [Optionally] RenderFrameHostStateChange(host, new_state) to know when it goes in pending deletion.

Better, but I think this is very unlikely to happen. Instead of having all of this in WebContentsObserver, having a RenderFrameHostObserver. It will allow embedders to track a particular RenderFrameHost.
Project Member

Comment 33 by bugdroid1@chromium.org, Oct 17

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

commit c6ada4673960c873c61318f5ee6ab850bea49490
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Wed Oct 17 11:07:18 2018

WebNavigationAPI. Do not track subframe in pending deletion.

Current state:
==============
After a cross-process navigation, content/ swaps the old and the new
RenderFrameHost. The old RenderFrameHost goes into pending deletion.
Its subframe are immediately deleted.

So after a RenderFrameHost swap, WebNavigationAPI stops tracking:
 * the navigating frame in RenderFrameHostChanged().
 * its subframe in FrameDeleted()

Future:
======
In a near future, the subframe of |old_host| will go into pending
deletion as well. As a result, FrameDeleted() will not be immediately
called.
See: https://chromium-review.googlesource.com/1122629

What this CL does:
==================
Stop tracking the subframes of |old_host| when it goes into pending
deletion.

Bug: 609963
Change-Id: Ifba038af87a74f3b8e68794b62d3d44055efecad
Reviewed-on: https://chromium-review.googlesource.com/c/1278170
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600346}
[modify] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
[modify] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/browser/extensions/api/web_navigation/web_navigation_api.h
[modify] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
[modify] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/framework.js
[add] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/pendingDeletion/iframe_slow_to_unload.html
[add] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/pendingDeletion/manifest.json
[add] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/pendingDeletion/slow_to_unload.html
[add] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/pendingDeletion/test_pendingDeletion.html
[add] https://crrev.com/c6ada4673960c873c61318f5ee6ab850bea49490/chrome/test/data/extensions/api_test/webnavigation/pendingDeletion/test_pendingDeletion.js

> +1: Most users just want this. It would be very nice for them to have only one method to watch for instead of two. I think it might be possible to call FrameDeleted each time. What do you think? I think I am going to try this and see.

Could you elaborate on "call FrameDeleted each time"? This sounds similar to my proposal, but you seem to imply something else.

> Better, but I think this is very unlikely to happen. Instead of having all of this in WebContentsObserver, having a RenderFrameHostObserver. It will allow embedders to track a particular RenderFrameHost.

My understanding is that embedders don't want to track RenderFrameHost. Basically, they want a signal "RenderFrameHost is not useful anymore". I don't see an issue if that doesn't particularly correspond to the destruction of RenderFrameHost object itself, as long as it does not appear in other APIs anymore. Strictly speaking, FrameDeleted(RFH*) is called before RFH is deleted anyway, so we won't change anything.

> Could you elaborate on "call FrameDeleted each time"? This sounds similar to my proposal, but you seem to imply something else.

I was just approving your idea. I am talking about the same thing.

> My understanding is that embedders don't want to track RenderFrameHost. Basically, they want a signal "RenderFrameHost is not useful anymore".

I see. Maybe notifying them earlier than at deletion time is okay for all of them.

Project Member

Comment 36 by bugdroid1@chromium.org, Oct 18

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

commit 9f370484d7256ce8e6b3d6955175edd4cc1faeb6
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Thu Oct 18 10:44:54 2018

RenderFrameHostDelegate: Fix documentation.

Update the documentation of RenderFrame{Created,Deleted}

The comments said it was about the creation/deletion of RenderFrameHost,
but in reality, it is about its RenderFrame. This is different.

Bug: 609963
Change-Id: Id83af9e82e7d544e68759dff2ffcefabb62c3374
Reviewed-on: https://chromium-review.googlesource.com/c/1286143
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600720}
[modify] https://crrev.com/9f370484d7256ce8e6b3d6955175edd4cc1faeb6/content/browser/frame_host/render_frame_host_delegate.h

Project Member

Comment 37 by bugdroid1@chromium.org, Oct 18

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

commit 5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Thu Oct 18 13:16:00 2018

Remove FrameTreeNode::ResetForNewProcess().

Currently, this method is simply calling ResetChildren() on the current
RenderFrameHost. It used to do more, but this is no longer the case.

This CL removes this method. Seeing RenderFrameHostImpl::ResetChildren()
in the code is more explicit about what happens. It is easier to
understand.

Bug: 609963
Change-Id: Ia82726f445b127ccca3758d7578e71eb32a49cbc
Reviewed-on: https://chromium-review.googlesource.com/c/1286421
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600742}
[modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/frame_host/frame_tree_node.h
[modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/frame_host/interstitial_page_impl.cc
[modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/5a9a88c1b0b1e6913ed2e5b0349b17bfcd4f4c30/content/browser/web_contents/web_contents_impl.cc

Project Member

Comment 38 by bugdroid1@chromium.org, Nov 7

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

commit f8840b9771e2686d3054a311440abb48ac636e65
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Wed Nov 07 14:10:35 2018

Keep subframe alive in pending deletion.

After a cross process navigation, the old RenderFrameHostImpl
is kept alive the time needed for executing its unload handler.

This CL generalize this by also keeping its subframes alive.
This fixes unload handler not always being run.

RenderFrameHostImpl is now delete as soon as:
 * All of its children have been deleted.
 * In case of an unload handler, wait FrameHostMsg_Detach.

Explainer: https://goo.gl/85KSff

Bug: 609963
Change-Id: Ie5b5585e54b9d1740798d649fcfac5dd2666920f
Reviewed-on: https://chromium-review.googlesource.com/c/1122629
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606036}
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/frame_tree_node.h
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/frame_host/render_frame_proxy_host.cc
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/renderer/render_frame_impl.h
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/test/test_render_frame_host.cc
[modify] https://crrev.com/f8840b9771e2686d3054a311440abb48ac636e65/content/test/web_contents_observer_sanity_checker.cc

Project Member

Comment 39 by bugdroid1@chromium.org, Jan 21 (2 days ago)

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

commit 549302586868ec5254ac03dcc3181051f3858990
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Jan 21 12:37:48 2019

Properly execute unload handlers on iframe after parent destruction.

On iframe destruction, wait for its children to delete properly before
deleting their parent.

This CL is simply reusing what was introduced in :
https://chromium-review.googlesource.com/1122629

Bug: 609963
Change-Id: Ic5825f586d73f76551c2ad7cb161e636f383d37f
Reviewed-on: https://chromium-review.googlesource.com/c/1354922
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624568}
[modify] https://crrev.com/549302586868ec5254ac03dcc3181051f3858990/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/549302586868ec5254ac03dcc3181051f3858990/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/549302586868ec5254ac03dcc3181051f3858990/content/browser/frame_host/render_frame_proxy_host.cc
[modify] https://crrev.com/549302586868ec5254ac03dcc3181051f3858990/content/browser/site_per_process_browsertest.cc

Sign in to add a comment