Fix Self-Referencing OOPIF Infinite Loop

content/browser/find_request_manager_browsertest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/command_line.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
@@ skipping 301 matching lines @@
  private:
   // Helper function for LoadMultiFramePage. Loads child frames until the frame
   // tree rooted at |root| is a full binary tree of height |height|.
   void LoadMultiFramePageChildFrames(int height,
                                      bool cross_process,
                                      FrameTreeNode* root) {
     if (height == 0)
       return;
 
     std::string hostname = root->current_origin().host();
-    if (cross_process)

[alexmos, 2016/12/28 00:25:59]

Can you explain why this change is needed?  I remember you mentioned some tests
were somehow affected by self-referential iframes, but don't recall the details.
 But this seems to mess with the way find-in-page tests work (i.e., always
create cross-process frames), which is unexpected.

[davidsac (gone - try alexmos), 2017/01/03 19:29:35]

Sorry for the miscommunication.  I sent a description of why I did this when I
requested that you review this.  I was correctly concerned that it could easily
be missed.  Sorry about that.  Let me know if you think this approach or my
understanding is incorrect or if you need further clarification.

[alexmos, 2017/01/03 22:12:05]

Ah, yes, I missed your explanation in the initial message, sorry!  (A tip is
that you can add a self-review comment like that on the right line (i.e., by
double-clicking), and then it'll be easy for reviewers to see.)

So, IIUC, the tree or origins that test creates for height=2 seems to be:
With --site-per-process: a(aa(aaa,aaa),aa(aaa,aaa))
Without --site-per-process: a(a(a,a),a(a,a))

And indeed, it seems without --site-per-process, we'll indeed hit the
self-referencing case with each grandchild frame for height >= 2.  I suppose
previously that was not a problem because this is a browser-initiated frame
navigation (using NavigateFrameToURL), and the Blink check only happened for
renderer-initiated navigations.  But now that your moved check applies to both
browser-initiated and renderer-initiated navigations, we need to worry about
this.  I don't think we should just remove |cross_process| though, since then
we'll lose coverage for non-cross-process subframes.  A couple of other options
are (1) fix the test to not rely on nesting the same URL arbitrarily deep in the
non-SPP case (e.g., by appending a query string or something), or (2) not
enforce the self-reference check for browser-initiated navigations.

[davidsac (gone - try alexmos), 2017/01/03 23:14:18]

That is a good point.  Let me know which approach you think will work better. 
In the mean time, I will admit that I am a bit confused by what you mean when
you say that we will lose test coverage.  As far as I can tell, we are not
losing coverage.  The test still runs and passes in both non-spp and spp mode. 
The only change is that the two trees look identical in both non-spp and spp,
or:  a(aa(aaa,aaa),aa(aaa,aaa)).  In other words, I believe that by removing
that line, I am performing the fix that you mentioned in 1).  Let me know if
this is incorrect.

[alexmos, 2017/01/04 22:35:10]

Let's go with option (2), since we've confirmed with Nasko that
browser-initiated subframe navigations should only happen in tests or history
navigations (and the latter shouldn't have problematic self-references as we
would've blocked them before they get into history).  That better matches the
scope of the existing Blink check anyway.

For losing test coverage, I was just worried that this will remove same-origin
iframes from the tests, so we won't be testing find-in-page for those cases
anymore (with or without OOPIFs turned on).  I don't know if that really matters
(paulmeyer@ would know), but regardless, I hope that we won't need to modify
this test anymore once we add the check for (2).

[davidsac (gone - try alexmos), 2017/01/06 00:44:57]

Done.

       hostname.insert(0, 1, 'a');
-    GURL url(embedded_test_server()->GetURL(hostname,
-                                            "/find_in_page_multi_frame.html"));
+      GURL url(embedded_test_server()->GetURL(

[alexmos, 2016/12/28 00:25:59]

Please fix the indent in lines 322-336.

[davidsac (gone - try alexmos), 2017/01/06 00:44:57]

Done.

+          hostname, "/find_in_page_multi_frame.html"));
 
-    TestNavigationObserver observer(shell()->web_contents());
+      TestNavigationObserver observer(shell()->web_contents());
 
-    FrameTreeNode* child = root->child_at(0);
-    NavigateFrameToURL(child, url);
-    EXPECT_TRUE(observer.last_navigation_succeeded());
-    LoadMultiFramePageChildFrames(height - 1, cross_process, child);
+      FrameTreeNode* child = root->child_at(0);
+      NavigateFrameToURL(child, url);
+      EXPECT_TRUE(observer.last_navigation_succeeded());
+      LoadMultiFramePageChildFrames(height - 1, cross_process, child);
 
-    child = root->child_at(1);
-    NavigateFrameToURL(child, url);
-    EXPECT_TRUE(observer.last_navigation_succeeded());
-    LoadMultiFramePageChildFrames(height - 1, cross_process, child);
+      child = root->child_at(1);
+      NavigateFrameToURL(child, url);
+      EXPECT_TRUE(observer.last_navigation_succeeded());
+      LoadMultiFramePageChildFrames(height - 1, cross_process, child);
   }
 
   TestWebContentsDelegate test_delegate_;
   WebContentsDelegate* normal_delegate_;
 
   // The ID of the last find request requested.
   int last_request_id_;
 
   DISALLOW_COPY_AND_ASSIGN(FindRequestManagerTest);
 };
@@ skipping 218 matching lines @@
   blink::WebFindOptions options;
   Find("result", options);
   options.findNext = true;
   options.forward = false;
   Find("result", options);
   Find("result", options);
   Find("result", options);
   delegate()->WaitForFinalReply();
 
   FindResults results = delegate()->GetFindResults();
+

[alexmos, 2016/12/28 00:25:58]

new blank line not needed

[davidsac (gone - try alexmos), 2017/01/06 00:44:57]

Done.

   EXPECT_EQ(last_request_id(), results.request_id);
   EXPECT_EQ(21, results.number_of_matches);
   EXPECT_EQ(19, results.active_match_ordinal);
 
   // Navigate one of the empty frames to a page with 5 matches.
   FrameTreeNode* root =
       static_cast<WebContentsImpl*>(shell()->web_contents())->
       GetFrameTree()->root();
   GURL url(embedded_test_server()->GetURL(
       GetParam() ? "b.com" : "a.com", "/find_in_simple_page.html"));
@@ skipping 227 matching lines @@
     delegate()->MarkNextReply();
     contents()->ActivateNearestFindResult(
         rects[order[i]].CenterPoint().x(), rects[order[i]].CenterPoint().y());
     delegate()->WaitForNextReply();
     EXPECT_EQ(order[i] + 1, delegate()->GetFindResults().active_match_ordinal);
   }
 }
 #endif  // defined(OS_ANDROID)
 
 }  // namespace content