For non-OOPIF case, the LocalFrameView is reset in call to:
LocalFrameClientImpl::TransitionToCommittedForNewPage(). Maybe we should notify the parent process of the OOPIF to do the same.

What's the callstack for the call that's clearing the content view of the embed?

In the scenario explained:
  HTMLFrameOwnerElement::SetEmbeddedContentView()
  HTMLPlugInElement::DetachLayoutTree()
  Node::LazyReattachIfAttached()
  HTMLPlugInElement::LazyReattachIfNeeded()
  HTMLEmbedElement::ParseAttribute()

This is normally fine because if the view is a PluginView then during RequestObject() we end up creating a new plugin and add the view (or use fallback one). If the frame is local it works fine as well given that LFC::TransitionTo... takes care of frame view. Also if navigation is cross origin then WebFrame::Swap will set the EmbeddedContentView. So only remote frame to remote frame is the issue.

One solution could be that in HTMLPlugInElement::RequestObjectInternal() if ObjectContentType is found to be of a frame type, and we do already have a ContentFrame(), then call SetEmbeddedContentView(ContentFrame()->View()) (or maybe do so if ContentFrame()->IsRemoteFrame()).

I think this will make the behavior consistent with <iframe>'s given that we do not call SetEmbeddedContentView(nullptr) in a similar situation (e.g., setting |iframe.src|).

 Issue 843591  has been merged into this issue.

Based on  issue 843591 , there are additional repro steps in https://bugs.chromium.org/p/chromium/issues/detail?id=838760#c15:

test7.html:
<iframe id="printPage" src="test6.html" width="100%" style="min-height:700px;" frameborder="0"></iframe>   

test6.html:
<object width="909" height="1286" data="https://pro.virtualyard.com.au/popups/NSW-Form5/1/1.svg" type="image/svg+xml"></object>   

1) Open network devtools panel.
2) Navigate browser to test7.html.
3) Open print preview.
4) Close print preview.

Starting from (3), I observe a stream of successful network requests for the 1.svg resource [~1-2 a second].
Even after (4), the stream of network requests continues!


Note that this bug occurs whether the iframe with the object tag is an OOPIF or in the same process, but apparently the bug is specific to having Site Isolation enabled.

Given the loop of navigations, I would suggest targeting the https://crrev.com/c/996314 fix for M67 if possible.  (Ehsan mentioned it seems to fix the bug in the discussion on  issue 843591 .)

ekaramad@ and dcheng@, do you agree?

Adding a few notes following comment #7:

1- For the infinite loop bug, <object> does not even need to be inside an <iframe>, i.e., in step 2) using "test6.html" instead of "test7.html" would yield the same result (so empty print preview page for case of "test7.html" ius perhaps unrelated to infinite loop).

2- I do have to have a cross-origin frame inside <object> for this bug to repro. That is object.data should be cross-origin.

3- Instead of steps 3) and 4) you could do "object.data = object.data" in devtools. The underlying problem is that when <object> is embedding cross origin content, any change to 'data' will remove its RemoteFrameView which leads to an infinite update layout phase.

Comment 9: Hmm, I'm not seeing the navigation loop if I load test6.html directly, at least when testing on 68.0.3432.2.  I only see it when the object tag is within an iframe (local or remote).

You are right. My mistake.

With --site-per-process loading "test6.html" and print preview does not trigger it. But Loading "test6.html" and then typing "$('object').data = $('object').data" does.

Comment 11: Agreed!  I can confirm that.

dcheng@ / ekaramad@: Any chance we can get https://crrev.com/c/996314 landed today or tomorrow so it can bake over the weekend?  Would be good to merge on Monday if possible.

This should mostly be dcheng@'s call; both in terms of codereview and the scope of change to PlugInElements.

From what I understand, the change _will_ have significant behavior changes for plugins, but shoud _not_ cause any stability issues. The problem is very are extremely close to stable cut.

Ok.  That sounds like we might be better off getting the fix into M68 and live with the problem on M67.  Part of this is externally reported (from  issue 838760 ), but I'm not sure if the fix for the external reported issue is dependent on this or if this was just found along the way.

Let's still aim to get it landed and baking soon if possible.  Thanks!

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

commit 596e6b3281a8cc2c4b646553e4cde169a29a1d1d
Author: Ehsan Karamad <ekaramad@chromium.org>
Date: Fri May 25 04:26:02 2018

Cleanup plugin element frames when the layout tree is detached

Unlike frames, plugin elements (<embed> and <object>) are updated
when their layout tree is rebuilt. However, due to an oversight,
<embed> or <object> elements displaying subframes would only have
the associated content view (FrameView) torn down, leaving a
dangling content frame.

This led to some interesting side effects:
- When transitioning from a local frame to a plugin, the content
  frame would remain live, with JS still happily running.
- When transitioning from a remote frame to a plugin,
- When navigating a remote frame from one URL to another, the
  element would stop updating since the FrameView would be torn
  down but a new one would never be created. Note that this was
  not (as much of) a problem for local frames, since local frames
  re-create the LocalFrameView on every navigation.
With this CL, if a plugin element has an associated content frame,
use that to clean up the state associated with the element when the
layout tree is detached. This can cause the browser context to be
re-created: this matches the behavior of Edge and Firefox.

Note that there is still one edge case where Blink behaves oddly:
if the navigation fails and should display fallback content, the
content frame still remains attached. This will be addressed in
followups.

Link to whatwg issue: https://github.com/whatwg/html/issues/3576

Bug: 776510, 781880
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: Id18605fbe602ea6c0076c1d579345cdcf28cc984
Reviewed-on: https://chromium-review.googlesource.com/996314
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561768}
[modify] https://crrev.com/596e6b3281a8cc2c4b646553e4cde169a29a1d1d/content/browser/site_per_process_browsertest.cc
[add] https://crrev.com/596e6b3281a8cc2c4b646553e4cde169a29a1d1d/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/resources/test_page.html
[add] https://crrev.com/596e6b3281a8cc2c4b646553e4cde169a29a1d1d/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-embed-element/detach-frame-on-src-change.html
[add] https://crrev.com/596e6b3281a8cc2c4b646553e4cde169a29a1d1d/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-object-element/detach-frame-on-data-change.html
[modify] https://crrev.com/596e6b3281a8cc2c4b646553e4cde169a29a1d1d/third_party/WebKit/LayoutTests/http/tests/security/xss-DENIED-object-element.html
[modify] https://crrev.com/596e6b3281a8cc2c4b646553e4cde169a29a1d1d/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/596e6b3281a8cc2c4b646553e4cde169a29a1d1d/third_party/blink/renderer/core/frame/frame_test_helpers.cc
[modify] https://crrev.com/596e6b3281a8cc2c4b646553e4cde169a29a1d1d/third_party/blink/renderer/core/html/html_plugin_element.cc

Thanks for the fix!  Looks like r561768 missed the M68 branch (at r561733), though.  Maybe once it has some time to bake and we verify the fix, you could look into requesting a merge?  (Is it safe to merge?)

Thanks!

Unfortunately it did miss the branch. But that said, there seems to be a missing piece to the fix which Daniel and I are tracking in this: https://chromium-review.googlesource.com/c/chromium/src/+/1073204

(related bug:  846708 ).

So perhaps I'd need two merge requests but first waiting for the followup fix.

The NextAction date has arrived: 2018-05-29

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

commit 815589c705ec4f012c42cc3cd5e98519227b17e4
Author: Ehsan Karamad <ekaramad@chromium.org>
Date: Sat Jun 02 01:33:34 2018

Revert "Cleanup plugin element frames when the layout tree is detached"

This reverts commit 596e6b3281a8cc2c4b646553e4cde169a29a1d1d.

Reason for revert: DetachLayoutTree could be called at inopportune times
such as style recalc or during layout. This caused several crashes.

Original change's description:
> Cleanup plugin element frames when the layout tree is detached
>
> Unlike frames, plugin elements (<embed> and <object>) are updated
> when their layout tree is rebuilt. However, due to an oversight,
> <embed> or <object> elements displaying subframes would only have
> the associated content view (FrameView) torn down, leaving a
> dangling content frame.
>
> This led to some interesting side effects:
> - When transitioning from a local frame to a plugin, the content
>   frame would remain live, with JS still happily running.
> - When transitioning from a remote frame to a plugin,
> - When navigating a remote frame from one URL to another, the
>   element would stop updating since the FrameView would be torn
>   down but a new one would never be created. Note that this was
>   not (as much of) a problem for local frames, since local frames
>   re-create the LocalFrameView on every navigation.
> With this CL, if a plugin element has an associated content frame,
> use that to clean up the state associated with the element when the
> layout tree is detached. This can cause the browser context to be
> re-created: this matches the behavior of Edge and Firefox.
>
> Note that there is still one edge case where Blink behaves oddly:
> if the navigation fails and should display fallback content, the
> content frame still remains attached. This will be addressed in
> followups.
>
> Link to whatwg issue: https://github.com/whatwg/html/issues/3576
>
> Bug: 776510, 781880
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
> Change-Id: Id18605fbe602ea6c0076c1d579345cdcf28cc984
> Reviewed-on: https://chromium-review.googlesource.com/996314
> Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561768}

TBR=dcheng@chromium.org,creis@chromium.org,alexmos@chromium.org,ekaramad@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 776510, 781880, 847216,  846708 
Change-Id: I762631b92463352bea9bf3102d90a13b72776786
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Reviewed-on: https://chromium-review.googlesource.com/1083500
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563902}
[modify] https://crrev.com/815589c705ec4f012c42cc3cd5e98519227b17e4/content/browser/site_per_process_browsertest.cc
[delete] https://crrev.com/5b3021d75ad1bca4408f05c566de4a92793514f7/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/resources/test_page.html
[delete] https://crrev.com/5b3021d75ad1bca4408f05c566de4a92793514f7/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-embed-element/detach-frame-on-src-change.html
[delete] https://crrev.com/5b3021d75ad1bca4408f05c566de4a92793514f7/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-object-element/detach-frame-on-data-change.html
[modify] https://crrev.com/815589c705ec4f012c42cc3cd5e98519227b17e4/third_party/WebKit/LayoutTests/http/tests/security/xss-DENIED-object-element.html
[modify] https://crrev.com/815589c705ec4f012c42cc3cd5e98519227b17e4/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/815589c705ec4f012c42cc3cd5e98519227b17e4/third_party/blink/renderer/core/frame/frame_test_helpers.cc
[modify] https://crrev.com/815589c705ec4f012c42cc3cd5e98519227b17e4/third_party/blink/renderer/core/html/html_plugin_element.cc

ekaramad@: Just wanted to check in on this bug.  Are there other issues that need to be solved to reland the fix?  Thanks!

The (reverted) fix we had in mind does not quite work. I don't see any immediate fixes to reland the older patch. The only alternative options dcheng@ and I discussed (to fix the underlying problem) were

1- Try to asynchronously detach the plugin frame instead.

2- Make sure after calls to updateLayout... we always check for GetFrame().

Option 1) is still on the table but I have not had the bandwidth to work on it since then. Option 2) is a lot of work to the extent that I don't quite know where to start fixing.

Alternatively we might be able to resolve these OOPIF bugs through a workaround approach. We know the problem is that due to layout changes or plugin updates the EmbeddedContentView of the plugin element is disposed. What could theoretically work is to create a new RemoteFrameView here:
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/html/html_plugin_element.cc?rcl=a3d4e9c3567b9a10087809570a94724160d835bc&l=192

to replace the disposed RemoteFrameView. This will resolve this bug at the very least (and some others perhaps).

The original core bug will not be fixed however, i.e., if we start from <embed src="https://wikipedia.org"> (oopif) and then call embed.src = "some_pdf" the old OOPIF is not detached.

I will try to experiment with option 1) and what I proposed here to see if either could work.