I was able to find some more reliable steps to provoke this hang.

(1) Browse to https://www.att.com/shop/u-verse/offers.html
(2) Click on any "View package details" which opens a new iframe.
(4) Press Command+F "Find in page" and search for "s", and hold Enter until the search advances to the last occurrence on the page 152/152 (in the iframe), and at that point it will hang.

This pattern makes me less sure it's an issue with the frame hierarchy so much as a bug in "Find in page" itself.  I'm building a local version so I can debug when this hang happens.

+paulmeyer@ who also worked in this code before.

The infinite loop seems to be happening in FindRequestManager::Traverse() here:

  while ((node = TraverseNode(node, forward, wrap)) != nullptr) {
    if (!CheckFrame(node->current_frame_host()))
      continue;
    ...
  }

TraverseNode returns the same 'node', as there are no siblings or parent and 'wrap' is 'true'.  CheckFrame() returns false, so we continue and loop forever.

Some similar check as below is missing?

    if (wrap && node->current_frame_host() == from_rfh)
      return nullptr;

Friendly ping to look into this issue and to provide further update on this issue as it has been marked as a stable blocker.

Requesting someone from CC'd to look into this issue.

Thanks!

Mac triage: assigning directly to paulmeyer@

As stable already moved to M70 10 days ago, removing M69 label.
Thanks..!

Adding release blocker label for this issue.Please reduce priority or remove if not the case.

Thank You!

M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable  ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.

M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable  ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.

I have a CL to fix in review now: https://chromium-review.googlesource.com/c/chromium/src/+/1307247

Actually, regarding #4, that fix does not work, as the current frame host is never equal to the original frame host (which should be impossible). This leads me to think that there is no problem in the find code (which hasn't changed in the last couple years), and there is some bug in the frame tree code that may have regressed more recently.

Also, this has been broken since at least Chrome 62, so I don't think this makes sense to be a release blocker.

To be fair, site isolation didn't launch until M67 so from a user perspective, this is a fairly recent regression.

Regression has been bisected to this range: https://chromium.googlesource.com/chromium/src/+log/c998cf3857d76d0773b330adb2d9098453c53050..1ef89f762ea3021311302243113bb0e8876028b4

I can now see what is happening. The find code itself is fine, and does what it's always done, based on assumptions. The problem in this case is a corrupted frame tree.

The find algorithm is getting into a state where:

1. It knows there are matches on the page.
2. It has traversed past the end of a frame.
3. While looking for the next frame with matches (traversing the frame tree), the frame traversal algorithm has encountered a FrameTreeNode that has (according to that node) no parent, no children, and no siblings, even though it was linked to from some other node.
4. Because this frame also has no matches, the algorithm will continuously try to find one that does, and never find it.

Punting to creis@ to investigate further how the frame tree is becoming corrupted.

vmiura@ or paulmeyer@: Can you elaborate on how to repro this?  I can't get to step 2 in comment 3, since you apparently need a valid address for U-Verse to proceed, and it doesn't work in my area.

Without that, I can't comment on whether the frame tree is corrupted or if the find-in-page code needs to be robust to this case.  The description in comment 19 sounds like a lone main frame (no parent, children, or siblings) or maybe a FrameTreeNode that is pending deletion (except I don't think that's possible before https://chromium-review.googlesource.com/c/chromium/src/+/1122629 lands).

creis@: I used the address: 1065 AVENUE OF THE AMERICAS, FRNT 1, NEW YORK, NY 10018 to get "View package details"

I will try to reproduce.

I can reproduce.

FYI. I instrumented the code like this:
┌──┬──────────────────────────────────────────────────────────────────────────┐
│1 │  RenderFrameHost* FindRequestManager::Traverse(RenderFrameHost* from_rfh,│
│2 │                                                bool forward,             │
│3 │                                                bool matches_only,        │
│4 │                                                bool wrap) const {        │
│5 │    DCHECK(from_rfh);                                                     │
│6 │    FrameTreeNode* node =                                                 │
│7 │        static_cast<RenderFrameHostImpl*>(from_rfh)->frame_tree_node();   │
│8 │                                                                          │
│9 │+   std::list<RenderFrameHostImpl*> seen_list;                            │
│10│+   std::map<RenderFrameHostImpl*, int> seen_count;                       │
│11│+   auto print_rfh = [&](RenderFrameHostImpl* rfh) {                      │
│12│+     LOG(ERROR) << "RFH:";                                               │
│13│+     LOG(ERROR) << " * this = " << rfh;                                  │
│14│+     LOG(ERROR) << " * URL = " << rfh->GetLastCommittedURL();            │
│15│+     LOG(ERROR) << " * parent = " << rfh->GetParent();                   │
│16│+     LOG(ERROR) << " * is_active = " << rfh->is_active();                │
│17│+   };                                                                    │
│18│+                                                                         │
│19│    while ((node = TraverseNode(node, forward, wrap)) != nullptr) {       │
│20│+     seen_list.push_back(node->current_frame_host());                    │
│21│+     if (++seen_count[node->current_frame_host()] == 2) {                │
│22│+       for(auto* it : seen_list) {                                       │
│23│+         print_rfh(it);                                                  │
│24│+       }                                                                 │
│25│+     }                                                                   │
│26│+                                                                         │
│27│      if (!CheckFrame(node->current_frame_host()))                        │
│28│        continue;                                                         │
└──┴──────────────────────────────────────────────────────────────────────────┘

I got:

RFH:
 * this = 0x369958a04200
 * URL = about:blank
 * parent = 0x369958a04b00
 * is_active = 1
RFH:
 * this = 0x369956b08200
 * URL = about:blank
 * parent = 0x369958a04b00
 * is_active = 1
RFH:
 * this = 0x369958a03000
 * URL = about:blank
 * parent = 0x369958a04b00
 * is_active = 1
RFH:
 * this = 0x369958a03900
 * URL = https://www.att.com/scripts/touchcommerce/inqChat.html?IFRAME
 * parent = 0x369958a04b00
 * is_active = 1
RFH:
 * this = 0x369958aced00
 * URL = https://att.inq.com/tagserver/postToServer.min.htm
 * parent = 0x369958a03900
 * is_active = 1
RFH:
 * this = 0x369958ace400
 * URL = about:blank
 * parent = 0x369958a04b00
 * is_active = 1
RFH:
 * this = 0x3699567eb400
 * URL = about:blank
 * parent = 0x369958a04b00
 * is_active = 1
RFH:
 * this = 0x369958cc8200
 * URL = https://att.inq.com/chatskins/sites/10004119/assets/local-storage/storage.html
 * parent = 0x3699567eb400
 * is_active = 1
RFH:
 * this = 0x369958acc900
 * URL = 
 * parent = (nil)
 * is_active = 1
RFH:
 * this = 0x369958acc900
 * URL = 
 * parent = (nil)
 * is_active = 1

It is looping on the last one (0x369958acc900). I will try to understand what happens.

I took a look.

The looping frame lives in a different WebContents.

find-in-page is traversing several WebContents:
┌──┬──────────────────────────────────────────────────────────────────────────────┐
│1 │std::vector<FrameTreeNode*> GetChildren(FrameTreeNode* node) {                │
│2 │  std::vector<FrameTreeNode*> children(node->child_count());                  │
│3 │  for (size_t i = 0; i != node->child_count(); ++i)                           │
│4 │    children[i] = node->child_at(i);                                          │
│5 │                                                                              │
│6 │  if (auto* contents = WebContentsImpl::FromOuterFrameTreeNode(node)) {       │
│7 │    children.push_back(contents->GetMainFrame()->frame_tree_node());          │
│8 │  } else {                                                                    │
│9 │    contents = WebContentsImpl::FromFrameTreeNode(node);                      │
│10│    if (node->IsMainFrame() && contents->GetBrowserPluginEmbedder()) {        │
│11│      for (auto* inner_contents : contents->GetInnerWebContents()) {          │
│12│        children.push_back(inner_contents->GetMainFrame()->frame_tree_node());│
│13│      }                                                                       │
│14│    }                                                                         │
│15│  }                                                                           │
│16│                                                                              │
│17│  return children;                                                            │
│18│}                                                                             │
└──┴──────────────────────────────────────────────────────────────────────────────┘

The looping frame came from line 12 [contents->GetInnerWebContents()]

When clicking on "View package details", It looks like a "plugin" is created:

#2 0x55ca6472c99c extensions::MimeHandlerViewContainerBase::CreateMimeHandlerViewGuestIfNecessary()
#3 0x55ca6472b17b extensions::MimeHandlerViewContainer::DidResizeElement()
#4 0x55ca6865b21c content::BrowserPlugin::SynchronizeVisualProperties()
#5 0x55ca68bbc11c content::RenderWidget::RegisterBrowserPlugin()
#6 0x55ca6865b805 content::BrowserPlugin::Initialize()
#7 0x55ca67abc544 blink::LocalFrameClientImpl::CreatePlugin()
#8 0x55ca67b8a355 blink::HTMLPlugInElement::LoadPlugin()
#9 0x55ca67b89cd8 blink::HTMLPlugInElement::RequestObjectInternal()
#10 0x55ca67b8b36b blink::HTMLPlugInElement::RequestObject()
#11 0x55ca67b8747d blink::HTMLObjectElement::UpdatePluginInternal()
#12 0x55ca67b8a942 blink::HTMLPlugInElement::UpdatePlugin()
#13 0x55ca67a6d444 blink::LocalFrameView::UpdatePlugins()
#14 0x55ca67a6d692 blink::LocalFrameView::FlushAnyPendingPostLayoutTasks()
#15 0x55ca6742b14e blink::Document::UpdateStyleAndLayoutIgnorePendingStylesheets()
#16 0x55ca67b8b0eb blink::HTMLPlugInElement::LayoutEmbeddedContentForJSBindings()
#17 0x55ca67b8ae84 blink::HTMLPlugInElement::PluginWrapper()

It creates a MimeHandlerViewGuest. That's the second WebContents. It looks like find-in-page is looping forever inside it.

+CC dcheng@ FYI. You are knowledgeable about site isolation and plugins.
~~~
// TODO(dcheng): Consider removing this, since HTMLEmbedElementLegacyCall
// and HTMLObjectElementLegacyCall usage is extremely low.
v8::Local<v8::Object> PluginWrapper();
~~~

I didn't reproduced the bug, but found something strange:
If I embed a plugin like this:
~~~
<embed src="https://www.att.com/ecms/dam/att/consumer/upperfunnel/2018/pdf/350782-dtv-world-direct-channel-lineup.pdf"></embed>

Base
Base
Base
Base
Base
~~~

and find-in-page for "Base". I will first highlight "Base" in the main frame and then in the plugin. Once it is in the plugin, it will never goes away. (See video)
It looks like a bug to me. I think I am very close to make a reproducer.

Deleted: out-44.ogv 938 KB

	out-44.ogv 938 KB View Download

arthursonzogni@: That's awesome!  Thanks for digging in and finding that.  It sounds like this is perhaps a GuestView + find-in-page issue.  If you're able to finish the minimal repro of the hang, let us know.  (Feel free to grab ownership and finish the bug as well, if you're on a roll.)  :) 

Otherwise, paulmeyer@: Would you be able to take another look at the find-in-page code with respect to inner WebContents?  Arthur, Ehsan, or Lucas may be able to help as well? 

This could be a general GuestView bug (not only MHVG).

I could repro this bug (and more) using OOPIF-based <webview> as
 well.

Common Steps:
1- Inspect the top-level page in chrome://signin (not the <webview>).
2- Run this code in console:
document.write("<body>Base Base Base <webview src='data:text/html, Base Base Base'></webview>");

bug #1:
 3- Make sure the top-level is focused (e.g., double click the first "Base").
 4- Search for "Base".
 5- Observe all 6 instances are highlighted.
 6- Press "Return" key multiple times.
 7- Note all the "Base" instances inside parent get highlighted in orange but only one of the ones inside <webview> get highlighted.

 bug #2 :
  4- Focus <webview> i.e., double click inside <webview>
  5- Search for "Base".
  6- Observe all instances all 6 instances are highlighted with yellow.
  7- Press "Return" key multiple times and observe the orange highlight never leaves <webview>.

Bonus  Bug #3 :
  3- Focus <webview> and start Find-in-page.
  4- Pressing arrow down on the popup loops inside <webview>.
  5- Pressing arrow up on the popup eventually leaves <webview>.

Deleted: repro.mp4 5.7 MB

	repro.mp4 5.7 MB View Download

PS:
1- By reproducing "this" I meant what "arthursonzogni@" found initially.
2- The "bug links" to the first 3 bugs reported for chromium are unrelated to this bug :P.

I think it looks like this:
A embedded B. B is a pdf plugin.

A->GetInnerWebContents() return {B}
B->GetOuterWebContents() return nullptr instead of A.

I took a look. Browser plugin seems to be a weird beast.
They aren't using ConnectToOuterWebContents / AttachInnerWebContents / DetachInnerWebContents at all.
Because they aren't using the "normal" mechanism, there are hooks in GetOuterWebContents() / GetInnerWebContents()

Here is WebContentsImpl::GetOuterWebContents() implementation:
┌─┬───────────────────────────────────────────────────────────────┐
│1│WebContentsImpl* WebContentsImpl::GetOuterWebContents() const {│
│2│  if (GuestMode::IsCrossProcessFrameGuest(this))               │
│3│    return node_.outer_web_contents();                         │
│4│                                                               │
│5│  if (browser_plugin_guest_)                                   │
│6│    return browser_plugin_guest_->embedder_web_contents();     │
│7│                                                               │
│8│  return node_.outer_web_contents();                           │
│9│}                                                              │
└─┴───────────────────────────────────────────────────────────────┘

Line 5 and 6 is the hook for plugin. browser_plugin_guest_->owner_web_contents() return nullptr because the plugin still not attached.
In BrowserPluginGuest:
┌─┬─────────────────────────────────────────────────────┐
│1│  WebContentsImpl* embedder_web_contents() const {   │
│2│    return attached_ ? owner_web_contents_ : nullptr;│
│3│  }                                                  │
└─┴─────────────────────────────────────────────────────┘

If I replace line 6 by browser_plugin_guest_->owner_web_contents() then the bug do not reproduce anymore.
Example: https://chromium-review.googlesource.com/1322873
However, that's probably not the way to fix this bug.

I don't have any suggestions how to make it work properly. It should be assigned to someone owning this code.

+owner fsamuel@. Do you think you could help with GuestView?

#32: I am a bit surprised GetInnerWebContents() is returning a MimeHandlerViewGuest's WebContents (PDF viewer which uses BrowserPlugin). Looking at the code the only way I see AttachInnerWebContents is called is through calling ConnectToOuterWebContents which shouldn't happen for a BrowserPlugin-based GuestView.

If the problem is sending find requests while |attached_| is still false, then I think we should not be sending find requests to begin with.

I haven't been actively working in GuestView in 3 years. ekaramad@ is a more appropriate OWNER for this bug. Reassigning.

Re #32.

As I said it is not using ConnectToOuterWebContents / AttachInnerWebContents / DetachInnerWebContents at all.

There is a hook in WebContentsImpl::GetInnerWebContents() lines [2,7] for plugins.
┌───┬────────────────────────────────────────────────────────────────────────────────┐
│1  │std::vector<WebContentsImpl*> WebContentsImpl::GetInnerWebContents() {          │
│2 +│  if (browser_plugin_embedder_) {                                               │
│3 +│    std::vector<WebContentsImpl*> inner_contents;                               │
│4 +│    GetBrowserContext()->GetGuestManager()->ForEachGuest(                       │
│5 +│        this, base::BindRepeating(&GetInnerWebContentsHelper, &inner_contents));│
│6 +│    return inner_contents;                                                      │
│7 +│  }                                                                             │
│8  │                                                                                │
│9  │  return node_.GetInnerWebContents();                                           │
│10 │}                                                                               │
└───┴────────────────────────────────────────────────────────────────────────────────┘

Right thanks. I was looking at the wrong "WebContentsTreeNode::GetInnerWebContents()". I am taking a look now.

So this is the problem:

// Helper for GetInnerWebContents().
bool GetInnerWebContentsHelper(
    std::vector<WebContentsImpl*>* all_guest_contents,
    WebContents* guest_contents) {
  all_guest_contents->push_back(static_cast<WebContentsImpl*>(guest_contents));
  return false;
}

The |guest_contents| is not necessarily "attached" (i.e., at this point it might return nullptr for GetOuterWebContents()).

I have a CL to fix this (appears to fix the browser freeze bug):
https://chromium-review.googlesource.com/c/chromium/src/+/1323904

***UI Mass Triage***

Since work is in progress, adding appropriate labels.

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

commit 6beb2ea93baed495d6eb0a1d2b78b4b1b4f8830b
Author: Ehsan Karamad <ekaramad@chromium.org>
Date: Sun Nov 25 18:15:13 2018

Guest InnerWebContentses should always be attached

The API WebContentsImpl::GetInnerWebContents() returns two types of
WebContentses: the first type which are attached using
AttachToOuterContentsFrame, and legacy type which are based on
BrowserPlugin.

Similarly, WebContentsImpl::GetOuterWebContents() has a different
behavior depending on the type of the GuestView. Specifcally, if the
WebContents is for a BrowserPlugin-based GuestView, then
GetOuterWebContents() will return nullptr until BrowserPluginGuest is
attached. This means GetInnerWebContents() might return WebContentsImpls
whose BrowserPluginGuest is not still attached which is incorrect.

This issue seems to have caused a bug in find-in-page where there is a
MimeHandlerViewGuest in the page.

Bug:  897465 
Change-Id: I1fc9b209cf5d1835e2aee311e36da15526c249c4
Reviewed-on: https://chromium-review.googlesource.com/c/1323904
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610735}
[modify] https://crrev.com/6beb2ea93baed495d6eb0a1d2b78b4b1b4f8830b/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/6beb2ea93baed495d6eb0a1d2b78b4b1b4f8830b/chrome/browser/guest_view/mime_handler_view/chrome_mime_handler_view_browsertest.cc
[modify] https://crrev.com/6beb2ea93baed495d6eb0a1d2b78b4b1b4f8830b/content/browser/find_request_manager.h
[modify] https://crrev.com/6beb2ea93baed495d6eb0a1d2b78b4b1b4f8830b/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/6beb2ea93baed495d6eb0a1d2b78b4b1b4f8830b/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/6beb2ea93baed495d6eb0a1d2b78b4b1b4f8830b/content/public/test/find_test_utils.cc
[modify] https://crrev.com/6beb2ea93baed495d6eb0a1d2b78b4b1b4f8830b/content/public/test/find_test_utils.h

Marking as fixed since I cannot repro using the steps in the bug nor the ones in comment #3.

Should this be merged to M71?