jam, could you take a look please?

elawrence also had an idea (there might be a separate bug for it) that we could put in a DumpWithoutCrashing or UMA histogram for anytime an https:// navigation entry commits without a certificate attached. AFAIK that shouldn't really ever happen in practice. Might help us flush out these bugs.

Tentatively marking PlzNavigate-Blocking, but feel free to adjust if that doesn't seem right.

RE #2, I filed  Issue 736375  on the idea that we should collect reports via Telemetry for this state.

I managed to repro this twice more on Mac Beta. This time, a different set of repro steps:

+ PlzNavigate, Strict Site Isolation and Top Document Isolation all enabled.
- First I did the crazy clicking around, and I could no, reproduce the problem.
- I then closed the tab.
- I then did "Reopen closed tab"
- I then clicked back/forward around in the reopened tab.
the Cert went missing on two of the pages.

@estark @elawrence: I think so far there's no reason to believe that this is plz-navigate specific? I couldn't repro this with plznavigate (on mac and windows), but probably just I didn't get lucky.

@elawrence: if you can repro this in a short time, can you try without plznavigate?

I was able to use the repro steps in #4 to cause this issue in 61.0.3145.0 Canary where "BrowserSideNavigation-Control_50" which I believe means PlzNavigate is off? The affected Canary has enable-site-per-process set to true and TDI set to Default.

I got a repro with the steps in #4 in Windows Canary 61.3145.

Thanks, will try repro'ing this.

Repro'd on ChromeOS 61.3144 with default settings (Site Isolation off, TDI set to default, chrome://version lists "BrowserSideNavigation") 

I'm not sure if it's relevant, but running these steps in debug, I hit a failed DCHECK when I hit "Back" after clicking the Reopen Closed Tab:

[navigation_controller_impl.cc(860)] Check failed: pending_entry_index_ == -1 || pending_entry_->site_instance() || pending_entry_->restore_type() != RestoreType::NONE. 
Backtrace:
	content::NavigationControllerImpl::RendererDidNavigate [0x11EEDD0C+780]
	content::NavigatorImpl::DidNavigate [0x11F1523A+714]
	content::RenderFrameHostImpl::OnDidCommitProvisionalLoad [0x11F633B2+2610]
	content::RenderFrameHostImpl::OnMessageReceived [0x11F65090+1856]
	content::RenderProcessHostImpl::OnMessageReceived [0x1259BBC4+1332]

The failing DCHECK appears to be directly relevant; it fires only for the entries that lose the certificate (which makes sense, given the was_restored assignment directly below the DCHECK).

In the repro, I see ConfigureEntriesForRestore walk each entry in the restored navigation list and set RestoreType::CURRENT_SESSION for each. However, by the time we get to NavigationControllerImpl::RendererDidNavigate, the type has changed to RestoreType::NONE, failing the DCHECK. If the DCHECK is commented out, the lock icon is missing on the resulting page.

Hmm. I've found some other scenarios where the problem occurs and that DCHECK isn't hit; for instance, if I reopen the closed tab and quickly hit back in it before the closed page finishes load, the lock disappears:

****** I click Reopen Closed Tab ******
 navigation_controller_impl.cc(121)] ConfigureEntriesForRestore for [6] entries.
 navigation_entry_impl.h(337)] set_restore_type https://stackoverflow.com/ to type = current session
 navigation_entry_impl.h(337)] set_restore_type https://stackoverflow.com/questions to type = current session
 navigation_entry_impl.h(337)] set_restore_type https://stackoverflow.com/jobs?med=site-ui&ref=jobs-tab to type = current session
 navigation_entry_impl.h(337)] set_restore_type https://stackoverflow.com/documentation to type = current session
 navigation_entry_impl.h(337)] set_restore_type https://stackoverflow.com/tags to type = current session
 navigation_entry_impl.h(337)] set_restore_type https://stackoverflow.com/jobs?med=site-ui&ref=jobs-tab to type = current session
 navigation_controller_impl.cc(2194)] ****** NavigationControllerImpl::FinishRestore
 navigation_controller_impl.cc(1905)] ****** NavigationControllerImpl::NavigateToPendingEntry
 navigation_controller_impl.cc(2020)] ****** NavigationControllerImpl::NavigateToPendingEntryInternal
 navigation_controller_impl.cc(2209)] ****** NavigationControllerImpl::DiscardPendingEntry
 navigation_controller_impl.cc(1905)] ****** NavigationControllerImpl::NavigateToPendingEntry
 navigation_controller_impl.cc(2020)] ****** NavigationControllerImpl::NavigateToPendingEntryInternal
 navigator_impl.cc(1298)] ****** NavigatorImpl::DidStartMainFrameNavigation https://stackoverflow.com/jobs?med=site-ui&ref=jobs-tab
   has_browser_initiated_pending_entry: TRUE
   renderer_provisional_load_to_pending_url: FALSE
   has_transient_entry: FALSE

****** I hit BACK on the mouse; the /tags page loads without the lock ******
 navigation_controller_impl.cc(2209)] ****** NavigationControllerImpl::DiscardPendingEntry
 navigator_impl.cc(1298)] ****** NavigatorImpl::DidStartMainFrameNavigation https://stackoverflow.com/tags
   has_browser_initiated_pending_entry: FALSE
   renderer_provisional_load_to_pending_url: FALSE
   has_transient_entry: FALSE

****** A new NavigationEntry is created for the prior URL, and the RestoreType on that is RestoreType::NONE *******
	
 navigation_controller_impl.cc(211)] ****** NavigationControllerImpl::CreateNavigationEntry https://stackoverflow.com/tags
  Stacktrace:
   content::NavigatorImpl::DidStartMainFrameNavigation [0x10FFAEAD+781]
   content::NavigatorImpl::DidStartProvisionalLoad [0x11F15C72+418]
   content::RenderFrameHostImpl::OnDidStartProvisionalLoad [0x11F63DCB+267]
   base::DispatchToMethodImpl<content::RenderFrameHostImpl *,void (__thiscall content::RenderFrameHostImpl::*)(GURL const 
	
  navigator_impl.cc(1314)] New Navigation Entry Restore Type RestoreType::NONE
	
  navigation_controller_impl.cc(2209)] ****** NavigationControllerImpl::DiscardPendingEntry
  navigation_controller_impl.cc(499)] ****** NavigationControllerImpl::SetPendingEntry https://stackoverflow.com/tags
  navigator_impl.cc(547)] NavigatorImpl::DidNavigate

In the scenario where the DCHECK is hit, it seems like the problem is that there are two navigations to the same URL; the first clears the RestoreType off the entry, although the RestoreType remains set on the navigation_handle.
------------
navigation_controller_impl.cc(2124)] ****** NavigationControllerImpl::NotifyNavigationEntryCommitted https://stackoverflow.com/users
navigation_controller_impl.cc(1905)] ****** NavigationControllerImpl::NavigateToPendingEntry https://stackoverflow.com/tags
navigation_controller_impl.cc(2020)] ****** NavigationControllerImpl::NavigateToPendingEntryInternal
navigator_impl.cc(1298)] ****** NavigatorImpl::DidStartMainFrameNavigation https://stackoverflow.com/tags
  has_browser_initiated_pending_entry: TRUE
  renderer_provisional_load_to_pending_url: FALSE
  has_transient_entry: FALSE

navigator_impl.cc(547)] NavigatorImpl::DidNavigate to params.url=https://ssum-sec.casalemedia.com/usermatchs=183712&cb=https%3A%2F%2Fengine.adzerk.net%2Fudb%2F22%2Fsync%2Fi.gif%3FpartnerId%3D1%26userId%3D

navigation_controller_impl.cc(868)]  NavigationControllerImpl::RendererDidNavigate to pending entry index: 4. Has No Site Instance. Navigation handle RestoreType: 3; https://stackoverflow.com/tags entry has RestoreType 2

navigation_entry_impl.h(337)] set_restore_type for https://stackoverflow.com/tags to type = none
navigator_impl.cc(547)] NavigatorImpl::DidNavigate to params.url=https://stackoverflow.com/tags

*** HERE'S THE UNEXPECTED SECOND CALL TO RENDERERDIDNAVIGATE. Note that the NavigationHandle now has a RestoreType=2, but the NavigationEntry lost that value ****
navigation_controller_impl.cc(868)]  NavigationControllerImpl::RendererDidNavigate to pending entry index: 4.  Has No Site InstanceNavigation handle RestoreType: 2; https://stackoverflow.com/tags entry has RestoreType 3
--------------

If we set "was_restored=true" when the navigation_handle->GetRestoreType()!=RestoreType::NONE, this seems to fix this version of the problem.

In the case where we are not hitting the DCHECK (where NavigatorImpl::DidStartMainFrameNavigation calls NavigationControllerImpl::CreateNavigationEntry without setting the RestoreType on it), the "if (was_restored) test" at https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_controller_impl.cc?l=1302&rcl=ae9aca60b693134e1fa75f1f63b443df04b7ea17 fails, so we don't copy over the SSLStatus. If we hackily add the following

    // It's HTTPS but we lost the cert. Grab one from the handle.
    if (entry->GetURL().SchemeIsCryptographic() && 
        !entry->GetSSL().certificate) {
      // ::dog_doing_chemistry_meme:: here
      entry->GetSSL() = handle->ssl_status();
    }

... then all repros seem to go away.

Thanks for the repro. I managed to write a browser test that repros this using the above instructions. However, before I work on a fix I was trying to get a repro test for  bug 727892  so that the fix for this issue doesn't regress the other one. I'ms till having a tough time with that though.

+clamy & nasko FYI

I now understand this bug better. The lost security chip is one symptom of a bigger issue: the NavigationHandle being reset when an old didstoploading or didcommitprovisionalload IPC arrive. Other data on NavigationHandle is lost, and we recreate it in RenderFrameHostImpl::TakeNavigationHandleForCommit. The new NH doesn't have the SSLStatus, and also a bunch of other fields which are reset to default values. For one thing, this could affect timing related UMA histograms.

It's a bit rare because this only occurs when a navigation is in the same site instance and the RenderFrameHost is reused, and the user navigates very quickly.

Also to add, this doesn't happen without PlzNavigate because the NavigationHandle is created later in that case (FrameHostMsg_DidStartProvisionalLoad) and so the IPC pipe had been flushed out of old IPCs.

Camille will take this on as other improvements being worked on for the navigation code paths will fix this.

Also to clarify: the specific bug that elawrence found (repro in comment 4) is only with session restore. The fix from  bug 727892  isolated the effects of a lost NavigationHandle to only the session restore case, since otherwise the existing NavigationEntry's SSLStatus is used.

So the bug, as is, only happens with:
-plznavigate
-session restore
-rapid navigations in same site instance

Given the last two points, and that the effect is a lost chip (i.e. as compared to other bugs where we would incorrectly mark an insecure page as secure), I think we can drop the M60 designation of this bug?

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

commit 3abacb6860a7a604643b302094ca1b30a96d4314
Author: jam <jam@chromium.org>
Date: Wed Jul 12 16:36:03 2017

Add a browsertest for the navigation handle getting lost when doing rapid navigations after a session restore.

BUG=738177

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

[modify] https://crrev.com/3abacb6860a7a604643b302094ca1b30a96d4314/chrome/browser/ssl/ssl_browser_tests.cc

As discussed in person, since we think this is limited to the session restore case and is not a security bug per se, moving to M62.

to follow up, the UMA stats show that in this particular history case, Navigation.SecureSchemeHasSSLStatus.ExistingPageRestoredBrowserInitiated, shows that the SSL cert is available 99.81% of the time with PlzNavigate. Without PlzNavigate, it's 99.84% so almost the same. This bug needs to still be fixed of course, but looks like data shows that in practice it doesn't come up.

And for the general navigation case, Navigation.SecureSchemeHasSSLStatus
 shows that SSL cert is available 99.4% of the time without PlzNavigate and 99.44% of the time with PlzNavigate.

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

commit 6b92fcfb1cf296130f6cb38213aa52a0324c9e46
Author: clamy <clamy@chromium.org>
Date: Fri Jun 01 13:51:37 2018

Do not reset NavigationRequest in DidStopLoading

This CL fixes a race condition in which a NavigationRequest pending
commit is wrongly deleted when the RenderFrameHost receives a
DidStopLoading IPC. The NavigationRequest is now never deleted in
DidStopLoading. Instead, the RenderFrameHost passes a callback to

be called when the commit has been processed or cancelled. It's this
callback that deletes the NavigationRequest when the commit has been
cancelled.

FrameNavigationControl: :CommitNavigation and CommitFailedNavigation to
Bug: 789111,738177
Change-Id: Ib34b4b2d189ece0b23988f9bee6c5814b2e58356
Reviewed-on: https://chromium-review.googlesource.com/1064114
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563610}
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/common/frame.mojom
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/public/test/navigation_simulator.cc
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/public/test/navigation_simulator.h
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/renderer/navigation_client.cc
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/renderer/navigation_state_impl.cc
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/renderer/navigation_state_impl.h
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/renderer/render_frame_impl.h
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/test/test_render_frame.cc
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/test/test_render_frame_host.cc
[modify] https://crrev.com/6b92fcfb1cf296130f6cb38213aa52a0324c9e46/content/test/test_render_frame_host.h

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

commit d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec
Author: clamy <clamy@chromium.org>
Date: Thu Jul 12 13:52:18 2018

Store multiple NavigationRequests in RenderFrameHost

This CL makes it so that RenderFrameHost can store several
NavigationRequests waiting to commit instead of just one. This avoids
deleting a NavigationRequest that would later commit (which can happen
in quick navigations with a renderer process that is slow to process the
commit).

The regression test was originally written by arthursonzogni@chromium.org in
https://chromium-review.googlesource.com/c/chromium/src/+/671351.

The background for this change can be found in the following design doc:
https://docs.google.com/document/d/1mXjxYJptb_bZ_EqGMF-c4LTSnhjt6Gn_WVvSrsinpq8/edit#.

Bug: 738177
Change-Id: Idd666e41bd2094c90fa576c77cdc26fb7b54cd7f
Reviewed-on: https://chromium-review.googlesource.com/1082440
Commit-Queue: Camille Lamy <clamy@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574553}
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/browser_side_navigation_browsertest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/frame_host/navigation_controller_impl_unittest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/frame_host/render_frame_host_impl_browsertest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/renderer_host/render_widget_host_view_browsertest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/security_exploit_browsertest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/web_contents/web_contents_impl_unittest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/browser/webauth/webauth_browsertest.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/public/test/navigation_simulator.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/public/test/test_renderer_host.cc
[add] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/test/data/infinite_load_1.html
[add] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/test/data/infinite_load_2.html
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/test/did_commit_provisional_load_interceptor.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/test/did_commit_provisional_load_interceptor.h
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/test/test_render_frame_host.cc
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/test/test_render_frame_host.h
[modify] https://crrev.com/d3bfdb09875f6b6cdadd5075d7c1f7305b5118ec/content/test/test_web_contents.cc