I have investigated and I understand the issue now:

When an iframe starts loading in the renderer:
virtual WebFrameClient::DecidePolicyForNavigation() is called

But WebFrameClient has several subclasses:
* RenderFrameImpl
* PrepareFrameAndViewForPrint

The first one is used for normal navigation, the second one for print preview.
When using the second one, the navigation is not handled by the browser (as if PlzNavigate was not enabled).
It explains why in ResourceDispatcherHostImpl::BeginRequest([...]) the assertion fails and the the Renderer gets killed.

The request that is causing the bad IPC message is a request for one of the iframe of the page.

thestig@: is this request necessary, or could the PrepareFrameAndViewForPrint class be updated to not let nay navigation go through?

I'm also bumping up the priority since this is preventing us from launching PlzNavigate.

+weili
weili, thestig: Let us know if you're not the right people to cc on this bug.

@thestig, weili: can you help us with this one? This is preventing us from launching PlzNavigate in M60, so we really need to get this fixed soon.

As far as we can see, the navigation request is needed to show the iframe in the print selection. If we block the navigation here, the iframe won't appear when the selection is printed, but the rest of the text will.

Right now, the issue is the custom WebFrameClient. Can we remove it or make it depend from RenderFrame instead? This should ensure the navigation goes through the appropriate path in the browser.

I'll take a look soon. Did you mean M60 or M61?

Sorry, I don't understand PlzNavigate, so I have trouble fully understanding this bug and what I can do to help. I don't see "nav" as a keyword anywhere in the PrepareFrameAndViewForPrint code, so its role in (not) blocking navigation is a mystery to me.

I see PrepareFrameAndViewForPrint is a sub-class of blink::WebFrameClient, and it overrides 2 methods:

1) CreateURLLoader() -> Calls blink::Platform::CreateURLLoader() like every other override. No problem, I imagine.
2) CreateChildFrame() -> Nobody has mentioned this, but I'm guessing this is what's causing trouble? If so, I didn't write this override method, have no idea why it's needed, and have no idea what needs to be fixed. It originally came from r242941. Most of the time, it's someone who works with nasko@ that's touching this method. The printing folks just nod and say ok.

If those are not the problem, then is it just the fact that PrepareFrameAndViewForPrint exists?

Thank you for looking into this!

In PrepareFrameAndViewForPrint::CopySelection([...]), a new blink::WebView and a new blink::WebLocalFrame are created to render the selected part of the document (stringified into a data-url). This is a new navigation. PrepareFrameAndViewForPrint is set to be the WebFrameClient of the newly created frame.

The problem is that we expected content::RenderFrameImpl to always be used. So yes, the problem is that PrepareFrameAndViewForPrint exists.

The general problem is that the code here is using the blink objects directly without what is done in the upper layer, in content.

This prevents us to launch PlzNavigate for M60. We target M61.

Looking at r242941, it says "WebFrameClients that expect to ever create child frames must override this method and frameDetached() if they don't want to crash.", yet I don't see an implementation of frameDetached. Adding dcheng@.

Also, to expand a bit more on why things fail with PlzNavigate. In that mode, all navigations are sent to the browser process. We accomplish that by returning HandledByClient from WebFrameClient::DecidePolicyForNavigation override in RenderFrameImpl. This tells blink that content/ will take care of the navigation and to not do anything. Next, content/ sends the actual navigation IPC to the browser process and once the response is received, we send it back to the renderer for parsing. All of this happens with a completely separate set of IPCs from the original navigation code.

Since the PrepareFrameAndViewForPrint class implements WebFrameClient and doesn't override DecidePolicyForNavigation, blink thinks it needs to navigate the original way. We treat such navigations as unexpected when PlzNavigate is enabled and terminate the renderer process due to bad message.

So overall we need to find a path forward to not using the original navigation code, as we will be deleting it once PlzNavigate ships. I'm not sure what the proper way of fixing it, but if there is a way to do it by eliminating the WebFrameClient implementation, that will be great!

nasko@ and I had a chat and we came to the conclusion that doing a load of the iframe is likely to be an issue, regardless of the way the load is initiated (ie with the current navigation path or with PlzNavigate). For example, if the iframe was loaded via a POST navigation, we're worried it could lead to a repost of the data. Another issue is that the frame may have changed between the moment we initially loaded it and the moment we're trying to print it.

So our conclusion is that we need to serialize the iframe data instead of loading it. One possibility would be to export it as a data url, but we thought it would be better (if possible) to save it in an MHTML format.

+carlosk: do you know if it's possible to save as MHTML only a portion of a page (a frame for example) and not the whole page?

When it comes to replacing the custom print WebFrameClient by a RenderFrameImpl, we realized that this would probably be an issue since we won't have the appropriate browser-side classes to bind.

I don't know if MHTML will be able to handle that, but I was thinking how would it even work with OOPIFs if what the user wants to print is a selection with a remote frame. I don't think this is possible to do with the current code and if MHTML was to support this, it will have to be somehow browser-process mediated approach. 

This made me think - weili@, would your work on enabling printing of OOPIFs solve this case? Would you mind testing it with your local work, passing --enable-browser-side-navigation, and checking if this process kill reproes?

Users experienced this crash on the following builds:

Win Canary 61.0.3131.0 -  0.09 CPM, 3 reports, 3 clients (signature [Renderer kill 97] content::ResourceDispatcherHostImpl::BeginRequest)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

It is currently not possible to save only a part of the frame tree but I don't think it would be a complicated change. Serialization already happens per frame but always starts at the root.

MHTML is supposed to mimic what's currently in the page's DOM but there are some issues with achieving a 1-to-1 representation (like style discrepancies). I don't know how "print selection" determines what elements to print and so I can't say if it would be possible to cover with MHTML. And mind that the whole saving operation is coordinated browser side.

Re: C#11
I think I had to add CreateChildFrame(), because it's possible for the selection to contain iframes, and we don't want to crash print preview in that case.

I was talking with thestig@, and it seems like the "print selection" path only ever loads a data URL into the created blink::WebView. The way it seems to work is it serializes the selection as markup and then loads it into the blink::WebView it creates internally. So maybe we can cheat and make it work "enough"? Not sure how hard that'd be. We could try to create a full RenderFrameImpl for this, but it'd be fairly heavy, and we'd have to make sure things like scripting are disabled. I'm also not sure that any of the existing machinery expects to try to paint a page that's not really attached to a tab, but maybe I'm wrong...

Similarly, I'm led to believe that the blink::WebView for headers/footers is static... so maybe we could fake out enough of the system to make it work for printing until we have a more comprehensive solution...

Since dcheng@ asked me about this code offline, I dug into the history a bit and the code is 6-8 years at this point. Since the printing code has done essentially the same thing this entire time, without anyone complaining about it up until now, it has been left along mostly in the "if it's not broken, don't touch it" mode.

For print selection, really I just want the feature to continue working as is. I'll all for tweaking PrepareFrameAndViewForPrint as needed to make it PlzNavigate compatible. As for how well the OOPIF code from weili@ (OOO, BTW) deals with this, there's an experimental CL here: https://codereview.chromium.org/2653963002/ but the last update is a couple months old. I don't see any changes to PrepareFrameAndViewForPrint::CopySelection() there though.

I also noticed the header+footer code in the same file also calls blink::WebView::Create() to load components/printing/resources/print_preview_page.html and draw it into the same canvas as a printed page. Is that going to be a problem for PlzNavigate? I tried --enable-browser-side-navigation with chrome://version/ and toggling header+footer in print preview still works.

Interestingly enough, printing iframes does not work even without plznavigate. It does not crash, but the resulting print doesn't show the iframe. I could reproduce this on M59 stable on mac and windows, and also trunk on windows. Probably this hasn't been noticed before because print-selection isn't used often, and most of the time iframes are just ads.

So it seems that the easiest fix for now to achieve parity between plznavigate and the old navigation code path is to return nullptr in PrepareFrameAndViewForPrint::CreateChildFrame (a dcheck does fire due to generating less frames than expected, but it wouldn't be in release builds). Non-plznavigate blocking bugs could be filed to fix iframe printing, and to not reload the iframes as Camille mentioned which seems sketchy.

Yea, I'm not surprised that iframes don't really work--and it would be relatively easy to disable that.

However, I thought the bigger problem is that print preview has its own implementation of WebFrameClient (which doesn't implement any PlzNavigate logic).

@dcheng: since print preview is building a data URL for the selection case, that works fine (other than iframes per above, which are also broken without plznavigate) since data URLs don't go to the browser for PlzNavigate. So let's keep this bug for the crash, and if new bugs are found that are specific to PlzNavigate let's open new issues to track them.

@jam. Yes selection>print doesn't work with iframes without PlzNavigate either. There is no crash but a blank rectangle instead. I had done the test before and was convinced that it worked well without PlzNavigate, but I am forced to note that no.

So yes, we could disable iframe to achieve parity with non-PlzNavigate.

I will work on this for today. I will also try to add new tests.

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

commit 6a22ae37ea816b3dd750a17fe91d23e562cdb671
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Wed Jun 21 20:32:47 2017

Printing: Disable subframes when printing a selection.

Printing a selection that contains an iframe doesn't work. An empty
rectangle is printed instead. This CL disable creation of subframes in
PrepareFrameForPreviewDocument.

Without this CL, the creation of the subframe lead to a new navigation
to the iframe's src. This can't work with
--enable-browser-side-navigation (aka PlzNavigate) because it expects
the WebFrameClient to be implemented by the RenderFrameImpl. In that
mode, the renderer is not expected to ask the browser for the subframe resource. For this reason, it gets killed in
content::ResourceDispatcherHostImpl::BeginRequest

Note: The main frame navigation works because the url is a data-url
and doesn't have to go to the browser.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel

BUG= 732780 

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

[add] https://crrev.com/6a22ae37ea816b3dd750a17fe91d23e562cdb671/chrome/browser/printing/print_browsertest.cc
[modify] https://crrev.com/6a22ae37ea816b3dd750a17fe91d23e562cdb671/chrome/test/BUILD.gn
[add] https://crrev.com/6a22ae37ea816b3dd750a17fe91d23e562cdb671/chrome/test/data/printing/selection_iframe.html
[modify] https://crrev.com/6a22ae37ea816b3dd750a17fe91d23e562cdb671/components/printing/renderer/print_web_view_helper.cc

BTW, I noticed on the CL that printing the iframe via the selection worked until r414879.

#c23 - it will be very interesting to understand why r414879 made a difference. It basically enabled OOPIFs when we have one page containing both web and extension frames. Does the print preview use any extension functionality?

Filed bug 736001 to track the iframe-selected-print bug, let's move discussion there of that since it's not plznavigate specific.

setting merge-request-60

This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Discussed offline, and we're going to wait for this to go out on Dev before approving for M60 to make sure it doesn't break anyone.

This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This look good in Dev and otherwise meets the bar for merging.  Approved for merge in M60.

I've manually merged this in https://chromium.googlesource.com/chromium/src/+/cf7d269051963582d8e28fc1087e1fff7407b8f4, but didn't add the bug line.