Fix View Frame Source for iframe of a file:// URL shows main frame's filename

Fix View Frame Source for iframe of a file:// URL shows main frame's filename

View Frame Source for iframe of a file:// URL used to show the main frame's
filename instead of the expected title derived from the iframe's page URL in
the form of "view-source:url". This CL fixes this and adds a browser test.

BUG=659040
TEST=browser_tests --gtest_filter=ChromeNavigationBrowserTest.TestViewFrameSource

Committed: https://crrev.com/4d116bd4f43359ce057a60c82c2188c93d49a638
Cr-Commit-Position: refs/heads/master@{#435021}

[afakhry, 2016-11-16 01:37:08]

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

[commit-bot: I haz the power, 2016-11-16 01:37:27]

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-16 02:48:24]

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

[commit-bot: I haz the power, 2016-11-16 02:48:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Reis, 2016-11-17 23:04:23]

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

[Charlie Reis, 2016-11-17 23:04:24]

Thanks for looking into a way forward!  I'm not sure if this one will work,
though.

Feel free to add a test while we discuss alternatives, since we'll want one
either way.

https://codereview.chromium.org/2501083004/diff/1/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/2501083004/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_commands.cc:1207:
last_committed_entry->SetURL(GURL());
Hmm, I'm not sure this is safe.  There are other places that we check the
NavigationEntry's URL to make decisions, and it looks like this would leave it
empty.  Does it get set again later, like at commit?  That still might do the
wrong thing while it's a pending entry (e.g., if it's a slow URL, what would we
show in the address bar before commit?).

[afakhry, 2016-11-19 01:25:32]

Description was changed from

==========
Fix View Frame Source for iframe of a file:// URL shows main frame's filename

BUG=659040
==========

to

==========
Fix View Frame Source for iframe of a file:// URL shows main frame's filename

View Frame Source for iframe of a file:// URL used to show the main frame's   
filename instead of the expected title derived from the iframe's page URL in  
the form of "view-source:url". This CL fixes this and adds a browser test.

BUG=659040
TEST=browser_tests
--gtest_filter=ChromeNavigationBrowserTest.TestViewFrameSource
==========

[afakhry, 2016-11-19 01:48:40]

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

[commit-bot: I haz the power, 2016-11-19 01:49:07]

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

[afakhry, 2016-11-19 01:49:29]

I added a browsertest that fails without the fix.

https://codereview.chromium.org/2501083004/diff/1/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/2501083004/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_commands.cc:1207:
last_committed_entry->SetURL(GURL());
On 2016/11/17 23:04:24, Charlie Reis (away) wrote:
> Hmm, I'm not sure this is safe.  There are other places that we check the
> NavigationEntry's URL to make decisions, and it looks like this would leave it
> empty.  Does it get set again later, like at commit?  That still might do the
> wrong thing while it's a pending entry (e.g., if it's a slow URL, what would
we
> show in the address bar before commit?).

Yes, it gets set again later as a result of receiving a message:

|#2 0x2b8f44714a17 content::NavigationEntryImpl::SetURL()
|#3 0x2b8f447061e5
content::NavigationControllerImpl::RendererDidNavigateToNewPage()
|#4 0x2b8f44704fec content::NavigationControllerImpl::RendererDidNavigate()
|#5 0x2b8f44732d15 content::NavigatorImpl::DidNavigate()
|#6 0x2b8f4473d567 content::RenderFrameHostImpl::OnDidCommitProvisionalLoad()
|#7 0x2b8f4473a798 content::RenderFrameHostImpl::OnMessageReceived()
|#8 0x2b8f44c5fd12 content::RenderProcessHostImpl::OnMessageReceived()
|#9 0x2b8f42fd5178 IPC::ChannelProxy::Context::OnDispatchMessage()

I've just tried it. If it's a slow page in the iframe, viewing frame source
still shows the right title with this fix.

Isn't this the last committed entry of the newly-cloned webcontents that will be
used to show the source? Do we really care about it's URL? Isn't it only used to
show the source?
If using an empty URL could cause problems, how about using the virtual url?

I'm sorry I don't have much knowledge of this area of the code.

[commit-bot: I haz the power, 2016-11-19 03:32:27]

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

[commit-bot: I haz the power, 2016-11-19 03:32:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Reis, 2016-11-23 23:52:19]

On 2016/11/19 01:49:29, afakhry wrote:
> I added a browsertest that fails without the fix.

Thanks!

> 
>
https://codereview.chromium.org/2501083004/diff/1/chrome/browser/ui/browser_c...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/2501083004/diff/1/chrome/browser/ui/browser_c...
> chrome/browser/ui/browser_commands.cc:1207:
> last_committed_entry->SetURL(GURL());
> On 2016/11/17 23:04:24, Charlie Reis (away) wrote:
> > Hmm, I'm not sure this is safe.  There are other places that we check the
> > NavigationEntry's URL to make decisions, and it looks like this would leave
it
> > empty.  Does it get set again later, like at commit?  That still might do
the
> > wrong thing while it's a pending entry (e.g., if it's a slow URL, what would
> we
> > show in the address bar before commit?).
> 
> Yes, it gets set again later as a result of receiving a message:
> 
> |#2 0x2b8f44714a17 content::NavigationEntryImpl::SetURL()
> |#3 0x2b8f447061e5
> content::NavigationControllerImpl::RendererDidNavigateToNewPage()
> |#4 0x2b8f44704fec content::NavigationControllerImpl::RendererDidNavigate()
> |#5 0x2b8f44732d15 content::NavigatorImpl::DidNavigate()
> |#6 0x2b8f4473d567 content::RenderFrameHostImpl::OnDidCommitProvisionalLoad()
> |#7 0x2b8f4473a798 content::RenderFrameHostImpl::OnMessageReceived()
> |#8 0x2b8f44c5fd12 content::RenderProcessHostImpl::OnMessageReceived()
> |#9 0x2b8f42fd5178 IPC::ChannelProxy::Context::OnDispatchMessage()
> 
> I've just tried it. If it's a slow page in the iframe, viewing frame source
> still shows the right title with this fix.
> 
> Isn't this the last committed entry of the newly-cloned webcontents that will
be
> used to show the source? Do we really care about it's URL? Isn't it only used
to
> show the source?
> If using an empty URL could cause problems, how about using the virtual url?
> 
> I'm sorry I don't have much knowledge of this area of the code.

The more I look at it, the more that a good portion of this code looks wrong for
the "view frame source" case.  There's lots of places in this path that assume
the title comes from the main frame, and that assume the NavigationEntry's URL
is the one we're loading (when it isn't).

While we don't need to fix all of the problems in this CL, we should try to move
toward something that's more accurate, rather than a workaround which might
break if we later improve things like UpdateTitleForEntry (and would thus be
fragile).  One idea for a fix below, which seems to work for me locally.

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/chrome_n...
File chrome/browser/chrome_navigation_browsertest.cc (right):

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:143:
StartServerWithExpiredCert();
Why do you need an expired cert?  The test above only needed it for the
interstitial.

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:156:
content::TestNavigationManager nav(web_contents, iframe_target_url);
It looks like a TestNavigationObserver might be a better fit here, since we
don't appear to be pausing or resuming the navigation.

nit: Use a more complete name, like observer or manager.

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:163:
WaitForLoadStop(web_contents);
I don't think we need this line (especially if we use TestNavigationObserver).

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:185:
embedded_test_server()->GetURL("/title1.html").spec());
Use iframe_target_url here?

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:1237:
last_committed_entry->SetURL(GURL());
Maybe it would make more sense to set this to |url| instead?  If I understand
correctly, it's set to the main frame's URL, but we end up loading the subframe
|url|.  That seems like a closer match to what's happening in practice, and
would avoid the file:// bug in this case.

[afakhry, 2016-11-24 00:25:10]

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

[commit-bot: I haz the power, 2016-11-24 00:25:43]

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

[afakhry, 2016-11-24 01:21:09]

Thank you for the helpful comments!

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/chrome_n...
File chrome/browser/chrome_navigation_browsertest.cc (right):

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:143:
StartServerWithExpiredCert();
On 2016/11/23 23:52:20, Charlie Reis (slow) wrote:
> Why do you need an expired cert?  The test above only needed it for the
> interstitial.

Sorry, that was a copy-paste line.

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:156:
content::TestNavigationManager nav(web_contents, iframe_target_url);
On 2016/11/23 23:52:20, Charlie Reis (slow) wrote:
> It looks like a TestNavigationObserver might be a better fit here, since we
> don't appear to be pausing or resuming the navigation.
> 
> nit: Use a more complete name, like observer or manager.

Done.

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:163:
WaitForLoadStop(web_contents);
On 2016/11/23 23:52:20, Charlie Reis (slow) wrote:
> I don't think we need this line (especially if we use TestNavigationObserver).

Done.

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/chrome_n...
chrome/browser/chrome_navigation_browsertest.cc:185:
embedded_test_server()->GetURL("/title1.html").spec());
On 2016/11/23 23:52:20, Charlie Reis (slow) wrote:
> Use iframe_target_url here?

Done.

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/2501083004/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:1237:
last_committed_entry->SetURL(GURL());
On 2016/11/23 23:52:20, Charlie Reis (slow) wrote:
> Maybe it would make more sense to set this to |url| instead?  If I understand
> correctly, it's set to the main frame's URL, but we end up loading the
subframe
> |url|.  That seems like a closer match to what's happening in practice, and
> would avoid the file:// bug in this case.

Thanks for the suggestion! It works.
Done.

[commit-bot: I haz the power, 2016-11-24 02:27:00]

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

[commit-bot: I haz the power, 2016-11-24 02:27:01]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[afakhry, 2016-11-28 17:07:41]

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

[commit-bot: I haz the power, 2016-11-28 17:08:09]

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-28 18:30:05]

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

[commit-bot: I haz the power, 2016-11-28 18:30:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2016-11-28 18:35:12]

On 2016/11/28 18:30:06, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

I'm not sure why my new test keeps failing on linux_chromium_rel_ng in
"site_per_process_browser_tests".

../../chrome/browser/chrome_navigation_browsertest.cc:183: Failure
Value of: new_web_contents->GetTitle()
  Actual: 127.0.0.1:44917/title1.html
Expected: url_formatter::FormatUrl(view_frame_source_url)
Which is: view-source:127.0.0.1:44917/title1.html

Is that expected in OOPIF on Linux?

[Charlie Reis, 2016-11-28 21:55:56]

On 2016/11/28 18:35:12, afakhry wrote:
> On 2016/11/28 18:30:06, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> I'm not sure why my new test keeps failing on linux_chromium_rel_ng in
> "site_per_process_browser_tests".
> 
> ../../chrome/browser/chrome_navigation_browsertest.cc:183: Failure
> Value of: new_web_contents->GetTitle()
>   Actual: 127.0.0.1:44917/title1.html
> Expected: url_formatter::FormatUrl(view_frame_source_url)
> Which is: view-source:127.0.0.1:44917/title1.html
> 
> Is that expected in OOPIF on Linux?

Interesting!  That looks like a real bug that happens when you run with
--site-per-process.  Certainly not expected.

I notice that it happens with either version of the fix (calling SetURL with url
or GURL()).  Actually, it happens without the fix as well-- appears to be a
pre-existing bug!  I can repro on 57.0.2931.0 with any OOPIF.  Both the title
and omnibox URL lack the view-source prefix when using View Frame Source,
suggesting that the virtual URL must be wrong.

Since your CL isn't introducing the bug, let's just exclude your test from that
mode by adding it to site-per-process.browser_tests.filter and land this anyway.
 Can you file a bug for it and assign to me?

LGTM apart from that.  Thanks!

[afakhry, 2016-11-29 00:49:00]

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

[commit-bot: I haz the power, 2016-11-29 00:50:01]

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

[afakhry, 2016-11-29 00:50:46]

afakhry@chromium.org changed reviewers:
+ sky@chromium.org

[afakhry, 2016-11-29 00:51:37]

On 2016/11/28 21:55:56, Charlie Reis (slow) wrote:
> On 2016/11/28 18:35:12, afakhry wrote:
> > On 2016/11/28 18:30:06, commit-bot: I haz the power wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > 
> > I'm not sure why my new test keeps failing on linux_chromium_rel_ng in
> > "site_per_process_browser_tests".
> > 
> > ../../chrome/browser/chrome_navigation_browsertest.cc:183: Failure
> > Value of: new_web_contents->GetTitle()
> >   Actual: 127.0.0.1:44917/title1.html
> > Expected: url_formatter::FormatUrl(view_frame_source_url)
> > Which is: view-source:127.0.0.1:44917/title1.html
> > 
> > Is that expected in OOPIF on Linux?
> 
> Interesting!  That looks like a real bug that happens when you run with
> --site-per-process.  Certainly not expected.
> 
> I notice that it happens with either version of the fix (calling SetURL with
url
> or GURL()).  Actually, it happens without the fix as well-- appears to be a
> pre-existing bug!  I can repro on 57.0.2931.0 with any OOPIF.  Both the title
> and omnibox URL lack the view-source prefix when using View Frame Source,
> suggesting that the virtual URL must be wrong.
> 
> Since your CL isn't introducing the bug, let's just exclude your test from
that
> mode by adding it to site-per-process.browser_tests.filter and land this
anyway.
>  Can you file a bug for it and assign to me?
> 
> LGTM apart from that.  Thanks!

Done. Filed crbug.com/669299. Thanks!

+sky for OWNERS review.

[commit-bot: I haz the power, 2016-11-29 02:48:50]

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

[commit-bot: I haz the power, 2016-11-29 02:48:51]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-29 04:18:12]

LGTM

[afakhry, 2016-11-29 17:16:10]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2016-11-29 17:16:11]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2501083004/#ps60001
(title: "Excluding test in site-per-process")

[commit-bot: I haz the power, 2016-11-29 17:16:27]

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-29 17:25:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480439770803300,
"parent_rev": "00574b55a5638f0122dac2a1a7cd539d00a26f2f", "commit_rev":
"0a3a86de4f5118de202d2eee7dc55796ab069e41"}

[commit-bot: I haz the power, 2016-11-29 17:26:02]

Description was changed from

==========
Fix View Frame Source for iframe of a file:// URL shows main frame's filename

View Frame Source for iframe of a file:// URL used to show the main frame's   
filename instead of the expected title derived from the iframe's page URL in  
the form of "view-source:url". This CL fixes this and adds a browser test.

BUG=659040
TEST=browser_tests
--gtest_filter=ChromeNavigationBrowserTest.TestViewFrameSource
==========

to

==========
Fix View Frame Source for iframe of a file:// URL shows main frame's filename

View Frame Source for iframe of a file:// URL used to show the main frame's   
filename instead of the expected title derived from the iframe's page URL in  
the form of "view-source:url". This CL fixes this and adds a browser test.

BUG=659040
TEST=browser_tests
--gtest_filter=ChromeNavigationBrowserTest.TestViewFrameSource
==========

[commit-bot: I haz the power, 2016-11-29 17:26:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-29 17:28:38]

Description was changed from

==========
Fix View Frame Source for iframe of a file:// URL shows main frame's filename

View Frame Source for iframe of a file:// URL used to show the main frame's   
filename instead of the expected title derived from the iframe's page URL in  
the form of "view-source:url". This CL fixes this and adds a browser test.

BUG=659040
TEST=browser_tests
--gtest_filter=ChromeNavigationBrowserTest.TestViewFrameSource
==========

to

==========
Fix View Frame Source for iframe of a file:// URL shows main frame's filename

View Frame Source for iframe of a file:// URL used to show the main frame's
filename instead of the expected title derived from the iframe's page URL in
the form of "view-source:url". This CL fixes this and adds a browser test.

BUG=659040
TEST=browser_tests
--gtest_filter=ChromeNavigationBrowserTest.TestViewFrameSource

Committed: https://crrev.com/4d116bd4f43359ce057a60c82c2188c93d49a638
Cr-Commit-Position: refs/heads/master@{#435021}
==========

[commit-bot: I haz the power, 2016-11-29 17:28:39]

Patchset 4 (id:??) landed as
https://crrev.com/4d116bd4f43359ce057a60c82c2188c93d49a638
Cr-Commit-Position: refs/heads/master@{#435021}