Fix SSL indicator missing when doing fast back/forward navigations.

Fix SSL indicator missing when doing fast back/forward navigations.

This regressed in r415977, which was fixing the session restore case where a serialized NavigationEntry wouldn't have the SSLStatus. The race in the non-restore case happens because a previous DidStopLoading IPC clears the new NavigationHandle which does have the SSLStatus. In this case, the fix is to only copy the SSLStatus specifically in the restore case.

BUG=727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
R=estark@chromium.org

Review-Url: https://codereview.chromium.org/2926803002 .
Cr-Commit-Position: refs/heads/master@{#477837}
Committed: https://chromium.googlesource.com/chromium/src/+/db06e65dcd8da3f5b8cab60ecb32e9ba89c1598c

[jam, 2017-06-07 06:08:39]

Description was changed from

==========
Fix SSL indicator missing when doing fast back/forward navigations.

This regressed in r415977, which was fixing the session restore case where a
serialized NavigationEntry wouldn't have the SSLStatus. The race in the
non-restore case happens because a previous DidStopLoading IPC clears the new
NavigationHandle which does have the SSLStatus. In this case, the fix is to only
copy the SSLStatus specifically in the restore case.

BUG=727892
==========

to

==========
Fix SSL indicator missing when doing fast back/forward navigations.

This regressed in r415977, which was fixing the session restore case where a
serialized NavigationEntry wouldn't have the SSLStatus. The race in the
non-restore case happens because a previous DidStopLoading IPC clears the new
NavigationHandle which does have the SSLStatus. In this case, the fix is to only
copy the SSLStatus specifically in the restore case.

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

[jam, 2017-06-07 06:08:43]

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

[commit-bot: I haz the power, 2017-06-07 06:09:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-06-07 06:09:15]

Description was changed from

==========
Fix SSL indicator missing when doing fast back/forward navigations.

This regressed in r415977, which was fixing the session restore case where a
serialized NavigationEntry wouldn't have the SSLStatus. The race in the
non-restore case happens because a previous DidStopLoading IPC clears the new
NavigationHandle which does have the SSLStatus. In this case, the fix is to only
copy the SSLStatus specifically in the restore case.

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

to

==========
Fix SSL indicator missing when doing fast back/forward navigations.

This regressed in r415977, which was fixing the session restore case where a
serialized NavigationEntry wouldn't have the SSLStatus. The race in the
non-restore case happens because a previous DidStopLoading IPC clears the new
NavigationHandle which does have the SSLStatus. In this case, the fix is to only
copy the SSLStatus specifically in the restore case.

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

[jam, 2017-06-07 06:09:15]

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

[commit-bot: I haz the power, 2017-06-07 07:20:50]

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

[commit-bot: I haz the power, 2017-06-07 07:20:51]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-07 17:23:54]

I tried but not sure how to test this because of the raciness. Suggestions
welcome.

[estark, 2017-06-08 00:19:59]

On 2017/06/07 17:23:54, jam wrote:
> I tried but not sure how to test this because of the raciness. Suggestions
> welcome.

Thanks for the fix, lgtm. I don't have any ideas at the moment for a test
either; maybe Charlie or Nasko can suggest ideas as a follow-up...

[estark, 2017-06-08 00:20:40]

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

https://codereview.chromium.org/2926803002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1300: // Only copy the
cert from the NavigationHandle since this code path can
Actually, hang on, sorry, this comment is a little confusing. Is this supposed
to say "Only copy in the restore case since this code path..." ?

[jam, 2017-06-08 00:43:58]

Patchset #2 (id:20001) has been deleted

[jam, 2017-06-08 00:46:06]

Description was changed from

==========
Fix SSL indicator missing when doing fast back/forward navigations.

This regressed in r415977, which was fixing the session restore case where a
serialized NavigationEntry wouldn't have the SSLStatus. The race in the
non-restore case happens because a previous DidStopLoading IPC clears the new
NavigationHandle which does have the SSLStatus. In this case, the fix is to only
copy the SSLStatus specifically in the restore case.

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

to

==========
Fix SSL indicator missing when doing fast back/forward navigations.

This regressed in r415977, which was fixing the session restore case where a
serialized NavigationEntry wouldn't have the SSLStatus. The race in the
non-restore case happens because a previous DidStopLoading IPC clears the new
NavigationHandle which does have the SSLStatus. In this case, the fix is to only
copy the SSLStatus specifically in the restore case.

BUG=727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
R=estark@chromium.org

Review-Url: https://codereview.chromium.org/2926803002 .
Cr-Commit-Position: refs/heads/master@{#477837}
Committed:
https://chromium.googlesource.com/chromium/src/+/db06e65dcd8da3f5b8cab60ecb32...
==========

[jam, 2017-06-08 00:46:07]

Committed patchset #2 (id:40001) manually as
db06e65dcd8da3f5b8cab60ecb32e9ba89c1598c (presubmit successful).

[jam, 2017-06-08 16:29:51]

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

https://codereview.chromium.org/2926803002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1300: // Only copy the
cert from the NavigationHandle since this code path can
On 2017/06/08 00:20:40, estark (slow thru Jun8) wrote:
> Actually, hang on, sorry, this comment is a little confusing. Is this supposed
> to say "Only copy in the restore case since this code path..." ? 

Done.