Fix the page's SSL status not being set on restore.

Fix the page's SSL status not being set on restore.

This regressed in r415186.

Also fix it for same page navigations (i.e. user pressing enter in omnibox) since the URL might change there if the server redirects.

BUG=642838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d208b90ce731f26eaadf615e082b269d753ba2d3
Cr-Commit-Position: refs/heads/master@{#415977}

[jam, 2016-08-31 23:45:41]

Description was changed from

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.
==========

to

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2016-08-31 23:45:45]

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

[commit-bot: I haz the power, 2016-08-31 23:46:25]

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-09-01 00:25:55]

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

[commit-bot: I haz the power, 2016-09-01 00:25:56]

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_...)

[jam, 2016-09-01 01:01:54]

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

[commit-bot: I haz the power, 2016-09-01 01:03:02]

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

[jam, 2016-09-01 01:04:48]

jam@chromium.org changed reviewers:
+ nasko@chromium.org

[jam, 2016-09-01 01:05:28]

Description was changed from

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.

BUG=598073, 504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-09-01 01:35:30]

https://codereview.chromium.org/2299843002/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/2299843002/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:3136: NavigationEntry* entry =
tab->GetController().GetActiveEntry();
GetLastCommittedEntry(). GetActiveEntry is deprecated.

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

https://codereview.chromium.org/2299843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1193: entry->GetSSL() =
handle->ssl_status();
I think we might need this to happen also in RendererDidNavigateToSamePage. It
is called when the user presses enter in the omnibox. However, if the server
does a redirect to different URL, we still will go through there. This happens
in cases where the user session has expired and the server redirects to the
login page.

[commit-bot: I haz the power, 2016-09-01 01:52:52]

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

[commit-bot: I haz the power, 2016-09-01 01:52:53]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-01 02:58:56]

https://codereview.chromium.org/2299843002/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/2299843002/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:3136: NavigationEntry* entry =
tab->GetController().GetActiveEntry();
On 2016/09/01 01:35:29, nasko (slow) wrote:
> GetLastCommittedEntry(). GetActiveEntry is deprecated.

Done.

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

https://codereview.chromium.org/2299843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1193: entry->GetSSL() =
handle->ssl_status();
On 2016/09/01 01:35:29, nasko (slow) wrote:
> I think we might need this to happen also in RendererDidNavigateToSamePage. It
> is called when the user presses enter in the omnibox. However, if the server
> does a redirect to different URL, we still will go through there. This happens
> in cases where the user session has expired and the server redirects to the
> login page.

I tried pressing enter in the omnibox and it goes through
RendererDidNavigateToSamePage, however the SSL certificate is correct in that
case (since the previous one is used). Do you mean something else?

[jam, 2016-09-01 02:59:02]

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

[commit-bot: I haz the power, 2016-09-01 02:59:19]

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-09-01 04:59:33]

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

[commit-bot: I haz the power, 2016-09-01 04:59:34]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jam, 2016-09-01 06:18:38]

Description was changed from

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.

BUG=598073, 504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.

BUG=642838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-09-01 06:27:55]

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

https://codereview.chromium.org/2299843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1193: entry->GetSSL() =
handle->ssl_status();
On 2016/09/01 02:58:56, jam wrote:
> On 2016/09/01 01:35:29, nasko (slow) wrote:
> > I think we might need this to happen also in RendererDidNavigateToSamePage.
It
> > is called when the user presses enter in the omnibox. However, if the server
> > does a redirect to different URL, we still will go through there. This
happens
> > in cases where the user session has expired and the server redirects to the
> > login page.
> 
> I tried pressing enter in the omnibox and it goes through
> RendererDidNavigateToSamePage, however the SSL certificate is correct in that
> case (since the previous one is used). Do you mean something else?

They key part is for the server to do a redirect. Similar test exists in
content/
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co...,
though it doesn't test SSL state, maybe we can add those checks there?

In general, is there a good reason not to do this at RendererDidNavigate
generically? Maybe around here -
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co....

[jam, 2016-09-01 15:41:30]

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

[commit-bot: I haz the power, 2016-09-01 15:41:50]

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

[jam, 2016-09-01 15:44:10]

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

https://codereview.chromium.org/2299843002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1193: entry->GetSSL() =
handle->ssl_status();
On 2016/09/01 06:27:55, nasko (slow) wrote:
> On 2016/09/01 02:58:56, jam wrote:
> > On 2016/09/01 01:35:29, nasko (slow) wrote:
> > > I think we might need this to happen also in
RendererDidNavigateToSamePage.
> It
> > > is called when the user presses enter in the omnibox. However, if the
server
> > > does a redirect to different URL, we still will go through there. This
> happens
> > > in cases where the user session has expired and the server redirects to
the
> > > login page.
> > 
> > I tried pressing enter in the omnibox and it goes through
> > RendererDidNavigateToSamePage, however the SSL certificate is correct in
that
> > case (since the previous one is used). Do you mean something else?
> 
> They key part is for the server to do a redirect. Similar test exists in
> content/
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co...,
> though it doesn't test SSL state, maybe we can add those checks there?

Got it, thanks. Ok I added a test and a fix.

> 
> In general, is there a good reason not to do this at RendererDidNavigate
> generically? Maybe around here -
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co....

Yep I agree that would be the easiest. However the previous change that added
this code already couldn't do it in all the codepaths of
RendererDidNavigateToNewPage. Then this fix couldn't do it in all codepaths of
RendererDidNavigateToExistingPage either (see patchset 1 which initially tried
this, but there were test failures).

[nasko, 2016-09-01 15:48:46]

LGTM

https://codereview.chromium.org/2299843002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2299843002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1271:
existing_entry->GetSSL() = rfh->navigation_handle()->ssl_status();
Just out of curiousity, does the new test you added fail without this?

[jam, 2016-09-01 15:52:31]

Description was changed from

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.

BUG=642838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.

Also fix it for same page navigations (i.e. user pressing enter in omnibox)
since the URL might change there if the server redirects.

BUG=642838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2016-09-01 15:53:01]

https://codereview.chromium.org/2299843002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2299843002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1271:
existing_entry->GetSSL() = rfh->navigation_handle()->ssl_status();
On 2016/09/01 15:48:45, nasko (slow) wrote:
> Just out of curiousity, does the new test you added fail without this?

of course :)

[jam, 2016-09-01 15:53:13]

The CQ bit was unchecked by jam@chromium.org

[jam, 2016-09-01 15:53:29]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2016-09-01 15:53:58]

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-09-01 16:58:33]

Description was changed from

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.

Also fix it for same page navigations (i.e. user pressing enter in omnibox)
since the URL might change there if the server redirects.

BUG=642838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.

Also fix it for same page navigations (i.e. user pressing enter in omnibox)
since the URL might change there if the server redirects.

BUG=642838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-01 16:58:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-01 17:00:30]

Description was changed from

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.

Also fix it for same page navigations (i.e. user pressing enter in omnibox)
since the URL might change there if the server redirects.

BUG=642838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix the page's SSL status not being set on restore.

This regressed in r415186.

Also fix it for same page navigations (i.e. user pressing enter in omnibox)
since the URL might change there if the server redirects.

BUG=642838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d208b90ce731f26eaadf615e082b269d753ba2d3
Cr-Commit-Position: refs/heads/master@{#415977}
==========

[commit-bot: I haz the power, 2016-09-01 17:00:32]

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