Add is_srcdoc to FrameNavigationEntry and restore about::srcdoc URL.

Add is_srcdoc to FrameNavigationEntry and restore about::srcdoc URL.

Bug description:
  Iframes with the srcdoc attribute have the "about::srcdoc" URL inside the
  renderer and the "about::blank" URL inside the browser. The conversion is
  made in RenderFrameHostImpl::OnDidCommitProvisionalLoad for security reason.
  A boolean 'is_srcdoc' is created and transmitted accross classes to keep track
  of the conversion.
  The problem was that this boolean was lost and nothing was there to
  restore the about::srcdoc URL. On a back-forward navigation, there is an URL
  mismatch in the renderer. Instead of restoring the previous content,
  it navigates to about::blank. The bug occurs only with PlzNavigate because it
  relies on the data sent by the browser and not by the renderer.

What the CL does:
  * The is_srcdoc attribute is stored on each FrameNavigationEntry.
  * The URL is restored to about::srcdoc when the navigation is committed to the
  renderer.

TEST=NavigationControllerBrowserTest.FrameNavigationEntry_RecreatedInjectedSrcdocSubframe
     RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
     with --enable-browser-side-navigation.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061

[arthursonzogni, 2016-11-07 13:24:14]

Description was changed from

==========
Add is_srcdoc to FrameNavigationEntry and restore about::srcdoc URL.

Bug description:
  Iframes with the srcdoc attribute have the "about::srcdoc" URL inside the
  renderer and the "about::blank" URL inside the browser. The conversion is
  made in RenderFrameHostImpl::OnDidCommitProvisionalLoad for security reason.
  A boolean 'is_srcdoc' is created and transmitted accross classes to keep track
  of the conversion.
  The problem was that this boolean was lost and nothing was there to
  restore the about::srcdoc URL. On a back-forward navigation, there is an URL
  mismatch in the renderer. Instead of restoring the previous content,
  it navigates to about::blank.

What the CL does:
  * The is_srcdoc attribute is stored on each FrameNavigationEntry.
  * The URL is restored to about::srcdoc when the navigation is committed to the
  renderer.

TEST=NavigationControllerBrowserTest.FrameNavigationEntry_RecreatedInjectedSrcdocSubframe
     RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
     with --enable-browser-side-navigation.
==========

to

==========
Add is_srcdoc to FrameNavigationEntry and restore about::srcdoc URL.

Bug description:
  Iframes with the srcdoc attribute have the "about::srcdoc" URL inside the
  renderer and the "about::blank" URL inside the browser. The conversion is
  made in RenderFrameHostImpl::OnDidCommitProvisionalLoad for security reason.
  A boolean 'is_srcdoc' is created and transmitted accross classes to keep track
  of the conversion.
  The problem was that this boolean was lost and nothing was there to
  restore the about::srcdoc URL. On a back-forward navigation, there is an URL
  mismatch in the renderer. Instead of restoring the previous content,
  it navigates to about::blank.

What the CL does:
  * The is_srcdoc attribute is stored on each FrameNavigationEntry.
  * The URL is restored to about::srcdoc when the navigation is committed to the
  renderer.

TEST=NavigationControllerBrowserTest.FrameNavigationEntry_RecreatedInjectedSrcdocSubframe
     RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
     with --enable-browser-side-navigation.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[arthursonzogni, 2016-11-07 13:25:58]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 13:26:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 14:28:14]

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

[commit-bot: I haz the power, 2016-11-07 14:28:15]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2016-11-07 15:20:33]

arthursonzogni@chromium.org changed reviewers:
+ alexmos@chromium.org

[arthursonzogni, 2016-11-07 15:20:33]

Hi Alex,

Please, could you take a look at this patch?

+@clamy, @creis [CC]

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:895: -1, false);
Here and below in this function, I assumed that RequestTransferURL doesn't
happen with iframe that has navigated to its srcdoc.

[arthursonzogni, 2016-11-07 15:26:53]

Description was changed from

==========
Add is_srcdoc to FrameNavigationEntry and restore about::srcdoc URL.

Bug description:
  Iframes with the srcdoc attribute have the "about::srcdoc" URL inside the
  renderer and the "about::blank" URL inside the browser. The conversion is
  made in RenderFrameHostImpl::OnDidCommitProvisionalLoad for security reason.
  A boolean 'is_srcdoc' is created and transmitted accross classes to keep track
  of the conversion.
  The problem was that this boolean was lost and nothing was there to
  restore the about::srcdoc URL. On a back-forward navigation, there is an URL
  mismatch in the renderer. Instead of restoring the previous content,
  it navigates to about::blank.

What the CL does:
  * The is_srcdoc attribute is stored on each FrameNavigationEntry.
  * The URL is restored to about::srcdoc when the navigation is committed to the
  renderer.

TEST=NavigationControllerBrowserTest.FrameNavigationEntry_RecreatedInjectedSrcdocSubframe
     RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
     with --enable-browser-side-navigation.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add is_srcdoc to FrameNavigationEntry and restore about::srcdoc URL.

Bug description:
  Iframes with the srcdoc attribute have the "about::srcdoc" URL inside the
  renderer and the "about::blank" URL inside the browser. The conversion is
  made in RenderFrameHostImpl::OnDidCommitProvisionalLoad for security reason.
  A boolean 'is_srcdoc' is created and transmitted accross classes to keep track
  of the conversion.
  The problem was that this boolean was lost and nothing was there to
  restore the about::srcdoc URL. On a back-forward navigation, there is an URL
  mismatch in the renderer. Instead of restoring the previous content,
  it navigates to about::blank. The bug occurs only with PlzNavigate because it
  relies on the data sent by the browser and not by the renderer.

What the CL does:
  * The is_srcdoc attribute is stored on each FrameNavigationEntry.
  * The URL is restored to about::srcdoc when the navigation is committed to the
  renderer.

TEST=NavigationControllerBrowserTest.FrameNavigationEntry_RecreatedInjectedSrcdocSubframe
     RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
     with --enable-browser-side-navigation.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-11-07 21:32:58]

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

[Charlie Reis, 2016-11-07 21:32:58]

[+nasko]

Thanks for taking this on!  I'd like to review it as well.

Structurally, this looks basically ok, but I feel like we might want to take a
different approach.  Rather than having to plumb this extra bit around
everywhere, I wonder if it's easier to just allow about:srcdoc as a real URL
from the renderer.  I'm including Nasko on the review, because I think he had to
add params.is_srcdoc for the NavHandle / WebNavAPI logic.

If I understand correctly, I think this line in RPHI::FilterURL (where we
canonicalize all about: URLs to about:blank) is the source of the problem (or at
least one of them):
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...

It might be much simpler if we just made an exception there and allowed
about:srcdoc as a valid URL.  (I gave that a quick try and it looks like there
might be something else involved as well, but maybe that's still a better
direction to pursue?)

Side note: Please try to always include a bug number in the CL description.  It
looks like this one is https://crbug.com/660061?  (If not, please file a new one
with the bug text you have in the description.)

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_navigation_entry.h (right):

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_navigation_entry.h:139: // Whether the
navigation is for an iframe with srcdoc attribute
nit: End with period.

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_navigation_entry.h:170: bool is_srcdoc_;
Does this value get restored properly if you quit and restart, restoring all
your tabs?  It's not persisted directly, but it does look like you might be able
to recover from that in RecursivelyGenerateFrameEntries.

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:884: // Be careful not to
include iframe srcdoc URLs in this check, which do
Note: This code also depended on detecting the srcdoc case.  I can update it
later if needed, but it's something that either the current or alternative
approach might be able to clean up.

[alexmos, 2016-11-07 22:51:46]

+1 to Charlie's suggestion of allowing about:srcdoc as a valid URL, if it works
out.  Other than that, a couple of other minor comments below.

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6789: //
Verifies that FrameNavigationEntry's srcdoc attribute is valid.
nit: might be useful to include a reference to https://crbug.com/660061, if
that's the right bug.

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6816: //
the entries if there is an inversion.
Instead of reordering, can you get the FNEs via FrameTreeNodes, via something
like entry->GetFrameEntry(root->child_at(0)), which should have the right order?
 (E.g., see EnsureFrameNavigationEntriesClearedOnMismatch which does this).

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6828:
EXPECT_EQ(frame_entry_2->url(), iframe_2_url);
Is the part with restoring a srcdoc iframe after a back-forward navigation
already tested in FrameNavigationEntry_RecreatedInjectedSrcdocSubframe that
you're re-enabling?

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:233: } else
nit: need { for else

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:895: -1, false);
On 2016/11/07 15:20:33, arthursonzogni wrote:
> Here and below in this function, I assumed that RequestTransferURL doesn't
> happen with iframe that has navigated to its srcdoc.

Yes, that seems right.

[arthursonzogni, 2016-11-08 10:12:23]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 10:12:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 11:52:53]

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

[commit-bot: I haz the power, 2016-11-08 11:52:54]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2016-11-08 12:15:23]

Description was changed from

==========
Add is_srcdoc to FrameNavigationEntry and restore about::srcdoc URL.

Bug description:
  Iframes with the srcdoc attribute have the "about::srcdoc" URL inside the
  renderer and the "about::blank" URL inside the browser. The conversion is
  made in RenderFrameHostImpl::OnDidCommitProvisionalLoad for security reason.
  A boolean 'is_srcdoc' is created and transmitted accross classes to keep track
  of the conversion.
  The problem was that this boolean was lost and nothing was there to
  restore the about::srcdoc URL. On a back-forward navigation, there is an URL
  mismatch in the renderer. Instead of restoring the previous content,
  it navigates to about::blank. The bug occurs only with PlzNavigate because it
  relies on the data sent by the browser and not by the renderer.

What the CL does:
  * The is_srcdoc attribute is stored on each FrameNavigationEntry.
  * The URL is restored to about::srcdoc when the navigation is committed to the
  renderer.

TEST=NavigationControllerBrowserTest.FrameNavigationEntry_RecreatedInjectedSrcdocSubframe
     RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
     with --enable-browser-side-navigation.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add is_srcdoc to FrameNavigationEntry and restore about::srcdoc URL.

Bug description:
  Iframes with the srcdoc attribute have the "about::srcdoc" URL inside the
  renderer and the "about::blank" URL inside the browser. The conversion is
  made in RenderFrameHostImpl::OnDidCommitProvisionalLoad for security reason.
  A boolean 'is_srcdoc' is created and transmitted accross classes to keep track
  of the conversion.
  The problem was that this boolean was lost and nothing was there to
  restore the about::srcdoc URL. On a back-forward navigation, there is an URL
  mismatch in the renderer. Instead of restoring the previous content,
  it navigates to about::blank. The bug occurs only with PlzNavigate because it
  relies on the data sent by the browser and not by the renderer.

What the CL does:
  * The is_srcdoc attribute is stored on each FrameNavigationEntry.
  * The URL is restored to about::srcdoc when the navigation is committed to the
  renderer.

TEST=NavigationControllerBrowserTest.FrameNavigationEntry_RecreatedInjectedSrcdocSubframe
     RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
     with --enable-browser-side-navigation.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=660061
==========

[arthursonzogni, 2016-11-08 12:21:30]

Thanks for your comments!

I strongly agree with Charlie's suggestion. I don't know if converting the
about::srcdoc URL to about::blank URL was really done on purpose. If it is not
done for a security reason, many pieces of code could be simplified/removed. A
lot of unrelated tests should be modified too because they expect to have the
about::blank URL instead.

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_navigation_entry.h (right):

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_navigation_entry.h:139: // Whether the
navigation is for an iframe with srcdoc attribute
On 2016/11/07 21:32:58, Charlie Reis (OOO til 11-11) wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_navigation_entry.h:170: bool is_srcdoc_;
On 2016/11/07 21:32:58, Charlie Reis (OOO til 11-11) wrote:
> Does this value get restored properly if you quit and restart, restoring all
> your tabs?  It's not persisted directly, but it does look like you might be
able
> to recover from that in RecursivelyGenerateFrameEntries.

Yes, is_srcdoc is restored properly because of the three lines I added in
RecursivelyGenerateFrameEntries.

But if these lines weren't there, there wouldn't be any problems for the user
either. |is_srcdoc| would be false instead of true and the |url| would be
about::srcdoc instead of about::blank in the browser. Finally the renderer would
see about::srcdoc as it is supposed to do.

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6789: //
Verifies that FrameNavigationEntry's srcdoc attribute is valid.
On 2016/11/07 22:51:46, alexmos wrote:
> nit: might be useful to include a reference to https://crbug.com/660061, if
> that's the right bug.

Yes, this CL is related to this bug. But this test doesn't fail because of this
bug. It simply tests that the new attribute that was added in the patch
|is_srcdoc| is correctly set.
I don't think it should be added there.

At most, maybe the reference could be added with
FrameNavigationEntry_RecreatedInjectedSrcdocSubframe. What do you think?

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6816: //
the entries if there is an inversion.
On 2016/11/07 22:51:45, alexmos wrote:
> Instead of reordering, can you get the FNEs via FrameTreeNodes, via something
> like entry->GetFrameEntry(root->child_at(0)), which should have the right
order?
>  (E.g., see EnsureFrameNavigationEntriesClearedOnMismatch which does this).

Thanks! It is better now.

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6828:
EXPECT_EQ(frame_entry_2->url(), iframe_2_url);
On 2016/11/07 22:51:46, alexmos wrote:
> Is the part with restoring a srcdoc iframe after a back-forward navigation
> already tested in FrameNavigationEntry_RecreatedInjectedSrcdocSubframe that
> you're re-enabling?

I am not sure I fully understand the question. Theses two tests are unrelated.
One tests that iframe with srcdoc are recreated within a back-forward
navigation. The other is simply an unit test that ensure that the new
|is_srcdoc| attribute is correctly set.

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:884: // Be careful not to
include iframe srcdoc URLs in this check, which do
On 2016/11/07 21:32:58, Charlie Reis (OOO til 11-11) wrote:
> Note: This code also depended on detecting the srcdoc case.  I can update it
> later if needed, but it's something that either the current or alternative
> approach might be able to clean up.

Yes, this code could be cleaned here by the two approachs. I will wait the
decision to follow one of them and then clean this part.

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2482873002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:233: } else
On 2016/11/07 22:51:46, alexmos wrote:
> nit: need { for else

Done.

[nasko, 2016-11-09 01:41:01]

Let's try possibly in another CL an approach where we keep about:srcdoc intact
as the URL. It has the potential to simplify a lot of code and it is actually
going to reflect reality anyway.

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:49: bool is_srcdoc =
(renderer_url == GURL(content::kAboutSrcDocURL));
Is the URL in the state indeed "about:srcdoc"?

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:245: is_srcdoc_ = false;
Why not initialize is_srcdoc_ in the initializer list and then only keep the
overwriting case on line 232?

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:292: // Iframes with the srcdoc
attribute have the "about::srcdoc" URL inside the
nit: Only one colon in "about:srcdoc".

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:293: // renderer and the
about::blank URL inside the browser. The conversion is
Ditto for about:blank.

[arthursonzogni, 2016-11-09 09:23:13]

Thanks!

I will make another CL and try to remove is_srcdoc boolean from the browser and
see what happen.

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:49: bool is_srcdoc =
(renderer_url == GURL(content::kAboutSrcDocURL));
On 2016/11/09 01:41:01, nasko wrote:
> Is the URL in the state indeed "about:srcdoc"?

Yes, because the FrameState has been built coming by the renderer.

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:245: is_srcdoc_ = false;
On 2016/11/09 01:41:01, nasko wrote:
> Why not initialize is_srcdoc_ in the initializer list and then only keep the
> overwriting case on line 232?

You are right.

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:292: // Iframes with the srcdoc
attribute have the "about::srcdoc" URL inside the
On 2016/11/09 01:41:01, nasko wrote:
> nit: Only one colon in "about:srcdoc".

Done.

https://codereview.chromium.org/2482873002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:293: // renderer and the
about::blank URL inside the browser. The conversion is
On 2016/11/09 01:41:01, nasko wrote:
> Ditto for about:blank.

Done.

[Charlie Reis, 2016-11-24 00:17:18]

This one can be closed in favor of https://codereview.chromium.org/2494633004/,
right?

[arthursonzogni, 2016-11-24 09:12:42]

On 2016/11/24 00:17:18, Charlie Reis (slow) wrote:
> This one can be closed in favor of
https://codereview.chromium.org/2494633004/,
> right?

Yes, that's right.