Do not create proxy hosts in the subtree of navigating frame.

Do not create proxy hosts in the subtree of navigating frame.

When a frame is navigating cross-process, its existing document will be completely destroyed and all child frames will be gone. This means that we don't need to create proxy objects for the new SiteInstance in the subtree of the navigating frame.

BUG=357747
Committed: https://crrev.com/faab4288197b20b73610aad3e5579751b0c3a97b
Cr-Commit-Position: refs/heads/master@{#294520}

[awong, 2014-09-03 21:58:37]

ajwong@chromium.org changed reviewers:
+ ajwong@chromium.org

[awong, 2014-09-03 21:58:38]

looks pretty solid. Added some API-level comments.

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:125: const
base::Callback<bool(FrameTreeNode*)>& on_node) const {
Reimplement this in terms of your new function?

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:212: // When creating proxies in the
FrameTree, it is in reponse to a node
"Proxies are created in the FrameTree in response to a node navigating to a new
SiteInstance."

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:213: // navigating to a new
SiteInstance. Since that node's navigation will replace
"this nodes" -> |source|'s

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:214: // the currently loaded document,
the entire subtree under the node will be
the node -> |source|

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.h:74: void ForEach(const
base::Callback<bool(FrameTreeNode*)>& on_node,
Does this need to be public?

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.h:75: FrameTreeNode* skip_node) const;
skip_node -> skip_this_subtree?

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1361:
switches::kSitePerProcess) && !is_main_frame) {
Document why we care that it's not a mainframe?

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1405:
CHECK(proxy_hosts_.find(render_frame_host_->GetSiteInstance()->GetId()) ==
Explain why this check is sensible?

[nasko, 2014-09-03 23:50:18]

nasko@chromium.org changed reviewers:
+ kenrb@chromium.org

[nasko, 2014-09-03 23:50:19]

Hey Ken,
Can you review this CL for me? I've asked Albert for some comments on the
FrameTree API, since he wrote it and I've addressed the comments. It still needs
a full review though.
Thanks!
Nasko

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:125: const
base::Callback<bool(FrameTreeNode*)>& on_node) const {
On 2014/09/03 21:58:38, awong wrote:
> Reimplement this in terms of your new function?

Done.

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:212: // When creating proxies in the
FrameTree, it is in reponse to a node
On 2014/09/03 21:58:38, awong wrote:
> "Proxies are created in the FrameTree in response to a node navigating to a
new
> SiteInstance."

Done.

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:213: // navigating to a new
SiteInstance. Since that node's navigation will replace
On 2014/09/03 21:58:38, awong wrote:
> "this nodes" -> |source|'s

Done.

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:214: // the currently loaded document,
the entire subtree under the node will be
On 2014/09/03 21:58:38, awong wrote:
> the node -> |source|

Done.

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.h:74: void ForEach(const
base::Callback<bool(FrameTreeNode*)>& on_node,
On 2014/09/03 21:58:38, awong wrote:
> Does this need to be public?

No.

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.h:75: FrameTreeNode* skip_node) const;
On 2014/09/03 21:58:38, awong wrote:
> skip_node -> skip_this_subtree?

Done.

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1361:
switches::kSitePerProcess) && !is_main_frame) {
On 2014/09/03 21:58:38, awong wrote:
> Document why we care that it's not a mainframe?

Done.

https://codereview.chromium.org/536143002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1405:
CHECK(proxy_hosts_.find(render_frame_host_->GetSiteInstance()->GetId()) ==
On 2014/09/03 21:58:38, awong wrote:
> Explain why this check is sensible?

Done.

[kenrb, 2014-09-05 17:23:38]

lgtm with nit

https://codereview.chromium.org/536143002/diff/20001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/536143002/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:648: {
This section probably warrants another comment. It takes some effort to work out
exactly what is being checked.

[nasko, 2014-09-05 18:39:27]

nasko@chromium.org changed reviewers:
+ jam@chromium.org

[nasko, 2014-09-05 18:39:27]

Hey John,
I need a content/ OWNER for this CL. Can you review it please?
Thanks in advance!
Nasko

[Charlie Reis, 2014-09-10 07:39:09]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2014-09-10 07:39:10]

Just a few nits and one question about the CommitPending change, which I don't
yet follow.

https://codereview.chromium.org/536143002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/536143002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.h:139: // starting at |skip_node| will not
be recursed into.
nit: |skip_this_subtree|

https://codereview.chromium.org/536143002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/536143002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1360: // In
--site-per-process, delete the proxy for the SiteInstance we are
I'm having trouble understanding this part.  Is this a consequence of the
ForEach changes, or something unrelated?  (I'd like to understand it deeply
because I'll need to move it as part of my CommitPending rewrite.)

https://codereview.chromium.org/536143002/diff/60001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/536143002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:573:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, ProxiesCreationSkipsSubtree) {
nit: ProxyCreationSkipsSubtree

https://codereview.chromium.org/536143002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:630: for (size_t i = 0; i <
child->child_count(); ++i){
nit: Space after )

https://codereview.chromium.org/536143002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:639:
EXPECT_TRUE(observer.navigation_succeeded());
Are the children FTNs now deleted?  Can we verify that?

[nasko, 2014-09-10 19:36:35]

https://codereview.chromium.org/536143002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/536143002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.h:139: // starting at |skip_node| will not
be recursed into.
On 2014/09/10 07:39:09, Charlie Reis wrote:
> nit: |skip_this_subtree|

Done.

https://codereview.chromium.org/536143002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/536143002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1360: // In
--site-per-process, delete the proxy for the SiteInstance we are
On 2014/09/10 07:39:10, Charlie Reis wrote:
> I'm having trouble understanding this part.  Is this a consequence of the
> ForEach changes, or something unrelated?  (I'd like to understand it deeply
> because I'll need to move it as part of my CommitPending rewrite.)

Added a TODO as per our chat.

https://codereview.chromium.org/536143002/diff/60001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/536143002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:573:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, ProxiesCreationSkipsSubtree) {
On 2014/09/10 07:39:10, Charlie Reis wrote:
> nit: ProxyCreationSkipsSubtree

Done.

https://codereview.chromium.org/536143002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:630: for (size_t i = 0; i <
child->child_count(); ++i){
On 2014/09/10 07:39:10, Charlie Reis wrote:
> nit: Space after )

Done.

https://codereview.chromium.org/536143002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:639:
EXPECT_TRUE(observer.navigation_succeeded());
On 2014/09/10 07:39:10, Charlie Reis wrote:
> Are the children FTNs now deleted?  Can we verify that?

Not here, since I load a page that has subframes. If I were to check here, it
will be flaky, since the frames might load quickly and child count won't be 0.
But this can be checked in the subsequent navigation, so I've added it there.

[Charlie Reis, 2014-09-10 19:44:17]

LGTM

https://codereview.chromium.org/536143002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/536143002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1412:
switches::kSitePerProcess) && !is_main_frame) {
nit: indent 4 more

[nasko, 2014-09-11 05:22:52]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-09-11 05:23:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/536143002/140001

[commit-bot: I haz the power, 2014-09-11 07:15:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-11 07:15:17]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nasko, 2014-09-11 14:27:03]

nasko@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[nasko, 2014-09-11 14:27:07]

Adding dmazzoni@ as FYI, as I'm making changes to the accessibility test.

[dmazzoni, 2014-09-11 15:09:31]

lgtm

https://codereview.chromium.org/536143002/diff/160001/content/browser/accessi...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/536143002/diff/160001/content/browser/accessi...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:116:
ASSERT_EQ(2U, ax_group->PlatformChildCount());
Could you either access ax_group->PlatformGetChild(1) below and assert what it
is, or else comment what it is and why we should ignore it?

Use the VLOGs above for a nice text dump of the accessibility tree so you
shouldn't have to guess the role.

[dmazzoni, 2014-09-11 15:10:40]

https://codereview.chromium.org/536143002/diff/160001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/536143002/diff/160001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:197:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, DISABLED_CrossSiteIframe) {
Any chance you could enable this on even one platform? I think it's stable on
Linux at least...

[nasko, 2014-09-11 17:10:36]

https://codereview.chromium.org/536143002/diff/160001/content/browser/accessi...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/536143002/diff/160001/content/browser/accessi...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:116:
ASSERT_EQ(2U, ax_group->PlatformChildCount());
On 2014/09/11 15:09:31, dmazzoni wrote:
> Could you either access ax_group->PlatformGetChild(1) below and assert what it
> is, or else comment what it is and why we should ignore it?
> 
> Use the VLOGs above for a nice text dump of the accessibility tree so you
> shouldn't have to guess the role.
> 

As discussed, I'll update this test in a separate CL to check for the entire
frame tree.

https://codereview.chromium.org/536143002/diff/160001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/536143002/diff/160001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:197:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, DISABLED_CrossSiteIframe) {
On 2014/09/11 15:10:39, dmazzoni wrote:
> Any chance you could enable this on even one platform? I think it's stable on
> Linux at least...

Your wish has been granted:

https://crrev.com/bd6b2977815dfded89735335f0d63a0c382bd322

[nasko, 2014-09-11 17:11:10]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:13:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/536143002/180001

[commit-bot: I haz the power, 2014-09-11 19:57:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-11 19:57:55]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nasko, 2014-09-11 20:11:31]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-09-11 20:18:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/536143002/200001

[commit-bot: I haz the power, 2014-09-12 02:00:58]

Committed patchset #11 (id:200001) as 1f11e0a4a29aa9c0816203fca86a2f9f9b9d9662

[commit-bot: I haz the power, 2014-09-12 02:04:38]

Patchset 11 (id:??) landed as
https://crrev.com/faab4288197b20b73610aad3e5579751b0c3a97b
Cr-Commit-Position: refs/heads/master@{#294520}

[engedy, 2014-09-12 08:54:43]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/565103002/ by engedy@chromium.org.

The reason for reverting is: SitePerProcessBrowserTest.ProxyCreationSkipsSubtree
repeatedly fails on Mac with ASAN reporting use-after-free, see:

http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%...
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%2010.8%20Bui...

@Nasko, as there are some simultaneous breakages right now, so I could not
investigate this further and opted to revert the whole thing to get the tree
green ASAP.

Snippets from test log:

ASSERTION FAILED: !localFrame || (localFrame->isLocalFrame())
../../third_party/WebKit/Source/core/frame/LocalFrame.h(253) : blink::LocalFrame
*blink::toLocalFrame(blink::Frame *)
../../content/browser/site_per_process_browsertest.cc:690: Failure
Value of: child->child_count()
  Actual: 2
Expected: 0U
Which is: 0.