Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache it on the browser …

Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache it on the browser side.

This is along the earlier patchsets of stopping to send SSLStatus to the renderer. This will help network service (since currently it's what sends this data, and it shouldn't know about UI state) and PlzNavigate (since this is a cursor for removing cert_ids which depend on having a renderer process during navigate). After this change, there's only one use left in the renderer that (devtools).

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

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

[jam, 2016-08-12 06:40:48]

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

[commit-bot: I haz the power, 2016-08-12 06:40:56]

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-08-12 07:01:26]

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

[commit-bot: I haz the power, 2016-08-12 07:01:27]

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

[jam, 2016-08-13 03:40:35]

Description was changed from

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

BUG=
==========

to

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

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

[jam, 2016-08-13 03:40:41]

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

[commit-bot: I haz the power, 2016-08-13 03:40:46]

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-08-13 04:30:23]

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

[commit-bot: I haz the power, 2016-08-13 04:30:24]

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

[jam, 2016-08-13 17:56:40]

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

[commit-bot: I haz the power, 2016-08-13 17:56:45]

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-08-13 17:59:34]

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

[commit-bot: I haz the power, 2016-08-13 17:59:35]

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

[jam, 2016-08-13 21:13:57]

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

[commit-bot: I haz the power, 2016-08-13 21:14:00]

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

[jam, 2016-08-13 21:14:47]

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

[jam, 2016-08-13 21:14:48]

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
precursor for removing cert_ids which depend on having a renderer process during
navigate). After this change, there's only one use left in the renderer that
I'll look at next (devtools).

The change has some minor cleanups to do (i.e. moving code out of
resource_loader.cc, adding more documentation). But before I do that, I wanted
to get your feedback if this is the right approach. Currently the SSLStatus is
set on a NavigationEntry based on data that was serialized as part of a frame's
network load, and it's sent back in FrameHostMsg_DidCommitProvisionalLoad. This
patch changes things so that in general, the ResourceLoader saves this data on
the pending NavigationEntry. This works for all cases (at least tests pass!)
other than
-in-process redirects (i.e. the SSLUITest failures in patchset 2): the problem
was that the pending NavigationEntry was thrown out because it's url
(pre-redirect) didn't match the final url. the solution is to store the
SSLStatus on the pending RFH, and NavigationController will read it when the
navigation commit
-cross process redirects when the response is transferred (i.e. the
content_browsertest failure in patchset 1): the ResourceDispatcherHost, which
knows when this happens, saves the SSLStatus on the RFH

[commit-bot: I haz the power, 2016-08-13 22:07:04]

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

[commit-bot: I haz the power, 2016-08-13 22:07:05]

Dry run: This issue passed the CQ dry run.

[estark, 2016-08-15 08:14:08]

Cool, it will be awesome if this works. I left a few questions inline. I don't
understand navigation very well so I can't quite convince myself that there are
no possible races or other weirdnesses.

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

https://codereview.chromium.org/2239273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1130: if (params.url !=
new_entry->GetURL()) {
Is this check just an optimization? I'm concerned that two different navigations
to the same URL could have different SSL properties.

https://codereview.chromium.org/2239273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1140: if
(rfh->GetSSLStatusForPendingNavigate(params.url, &ssl_status))
Just checking -- we're guaranteed that the returned SSLStatus corresponds to
*this* navigation? Is there any way we could call
SetSSLStatusForPendingNavigate() but then not Get() it until some later
navigation (different than the one that we called Set() for)?

https://codereview.chromium.org/2239273002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2239273002/diff/60001/content/browser/loader/...
content/browser/loader/resource_loader.cc:79: pending_entry->GetSSL() =
ssl_status;
Does this change the behavior of the omnibox during a navigation? Currently I
think the omnibox won't get updated to a lock icon until the navigation commits
(i.e. if you navigate to https://example.com, you won't get a lock in the
omnibox while waiting for the response, only when the navigation has committed).
I'm wondering if, with this change, you'll see a lock earlier.

If so, I'm not sure that's a bad thing, but I'll want to check with some omnibox
and UI people because I think it might affect an omnibox redesign that's in
progress (https://crbug.com/622531).

(I would patch this in and check it out myself, but I'm away from my workstation
this week so it's a little hard to test subtle UI changes like this.)

https://codereview.chromium.org/2239273002/diff/60001/content/browser/loader/...
content/browser/loader/resource_loader.cc:145: if (info->IsMainFrame()) {
1. The comment on ResourceRequestInfo::IsMainFrame() makes me think that this is
true for any request issued by the main frame. What you really want here is to
somehow check if this is the main resource request for the main frame, isn't it?
Maybe `info->GetResourceType() == MAIN_FRAME` is what you want?

2. I'm not sure if this is racy or not -- maybe you or Nasko know for sure -- is
there any way the pending navigation entry could change before this makes it to
the UI thread?

[jam, 2016-08-15 18:24:30]

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

[commit-bot: I haz the power, 2016-08-15 18:25:10]

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-08-15 18:34:12]

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

[commit-bot: I haz the power, 2016-08-15 18:34:13]

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

[jam, 2016-08-15 19:17:33]

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

[jam, 2016-08-15 19:17:48]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-08-15 19:18:00]

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

[jam, 2016-08-15 19:23:59]

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

https://codereview.chromium.org/2239273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1130: if (params.url !=
new_entry->GetURL()) {
On 2016/08/15 08:14:08, estark wrote:
> Is this check just an optimization? 

It wasn't; looking at it again it was poorly written. I've reworked it a bit to
make it clearer.

When the SSLState is saved on a pending NavigationEntry from ResourceLoader, we
want to use it. That's what the url check was effectively doing.


> I'm concerned that two different navigations
> to the same URL could have different SSL properties.

You mean that the server would return different certificates for the same url?

https://codereview.chromium.org/2239273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1140: if
(rfh->GetSSLStatusForPendingNavigate(params.url, &ssl_status))
On 2016/08/15 08:14:08, estark wrote:
> Just checking -- we're guaranteed that the returned SSLStatus corresponds to
> *this* navigation? Is there any way we could call
> SetSSLStatusForPendingNavigate() but then not Get() it until some later
> navigation (different than the one that we called Set() for)?

I don't think this is guaranteed. i.e. in the
SitePerProcessIgnoreCertErrorsBrowserTest.CrossSiteRedirectCertificateStore
case, the navigation IDs aren't the same for the pending naventry and the new
process's commit message, but we do want to use this cached SSLStatus on the RFH

https://codereview.chromium.org/2239273002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2239273002/diff/60001/content/browser/loader/...
content/browser/loader/resource_loader.cc:79: pending_entry->GetSSL() =
ssl_status;
On 2016/08/15 08:14:08, estark wrote:
> Does this change the behavior of the omnibox during a navigation? 

I just tried adding breakpoints and looking at the UI, and it doesn't appear
that the lock icon changes until the commit message arrives.

> Currently I
> think the omnibox won't get updated to a lock icon until the navigation
commits
> (i.e. if you navigate to https://example.com, you won't get a lock in the
> omnibox while waiting for the response, only when the navigation has
committed).
> I'm wondering if, with this change, you'll see a lock earlier.
> 
> If so, I'm not sure that's a bad thing, but I'll want to check with some
omnibox
> and UI people because I think it might affect an omnibox redesign that's in
> progress (https://crbug.com/622531).
> 
> (I would patch this in and check it out myself, but I'm away from my
workstation
> this week so it's a little hard to test subtle UI changes like this.)

https://codereview.chromium.org/2239273002/diff/60001/content/browser/loader/...
content/browser/loader/resource_loader.cc:145: if (info->IsMainFrame()) {
On 2016/08/15 08:14:08, estark wrote:
> 1. The comment on ResourceRequestInfo::IsMainFrame() makes me think that this
is
> true for any request issued by the main frame. What you really want here is to
> somehow check if this is the main resource request for the main frame, isn't
it?
> Maybe `info->GetResourceType() == MAIN_FRAME` is what you want?

ah, thanks, yes that's what I wanted. fixed.

> 
> 2. I'm not sure if this is racy or not -- maybe you or Nasko know for sure --
is
> there any way the pending navigation entry could change before this makes it
to
> the UI thread?

I think it's possible, that's why I send the URL to check that it matches.

[commit-bot: I haz the power, 2016-08-15 20:24:16]

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

[commit-bot: I haz the power, 2016-08-15 20:24:17]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-08-15 22:18:22]

Overall it will be great to have a bit more strong binding between the status
and the navigation, though I think that will basically come with PlzNavigate.
Some initial thoughts.

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1135:
new_entry->GetSSL() = ssl_status;
I think if we go that route, we need to also put the same update code in other
RendererDidNavigate* methods. SamePage, ExistingPage an do a redirect on hitting
the same URL, so they can change the URL and SSL status.

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2653:
pending_navigate_ssl_status_url_ = GURL();
Why would we not match the URL? I'd rather not return empty information from
here.

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:1065: SSLStatus
pending_navigate_ssl_status_;
If the SSLStatus is used only for the main frame, does it make sense to store it
in the RFH itself?

[mmenke, 2016-08-15 22:48:31]

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

[mmenke, 2016-08-15 22:48:32]

Driveby response to a comment, don't plan to review the code, just want to make
sure everything gets considered.

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

https://codereview.chromium.org/2239273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1130: if (params.url !=
new_entry->GetURL()) {
On 2016/08/15 19:23:58, jam wrote:
> On 2016/08/15 08:14:08, estark wrote:
> > Is this check just an optimization? 
> 
> It wasn't; looking at it again it was poorly written. I've reworked it a bit
to
> make it clearer.
> 
> When the SSLState is saved on a pending NavigationEntry from ResourceLoader,
we
> want to use it. That's what the url check was effectively doing.
> 
> 
> > I'm concerned that two different navigations
> > to the same URL could have different SSL properties.
> 
> You mean that the server would return different certificates for the same url?

It could be two different servers with the same hostname.  Server could have
updated its cert.  Or one could be a captive portal that suddenly appeared
(Along with a cert error click through).

One response could have been served out of cache (With a cached cert), while the
next could require us to go to the server (Again, may have to click through cert
error due to captive portal for the second URL, too), etc.

[jam, 2016-08-15 22:57:38]

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2653:
pending_navigate_ssl_status_url_ = GURL();
On 2016/08/15 22:18:22, nasko (slow) wrote:
> Why would we not match the URL? I'd rather not return empty information from
> here.

it's the same reason for resource_loader.cc's SetSSLStatus checking the URL.
i.e. there could be race conditions that lead to new pending navigation entries

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:1065: SSLStatus
pending_navigate_ssl_status_;
On 2016/08/15 22:18:22, nasko (slow) wrote:
> If the SSLStatus is used only for the main frame, does it make sense to store
it
> in the RFH itself?

Are you thinking of storing this temporary object in the pending
NavigationEntryImpl?

[mmenke, 2016-08-16 16:56:43]

Description was changed from

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

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

to

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

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

[mmenke, 2016-08-16 16:56:43]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[jam, 2016-08-22 15:24:17]

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

[commit-bot: I haz the power, 2016-08-22 15:24:49]

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-08-22 16:30:41]

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

[commit-bot: I haz the power, 2016-08-22 16:30:42]

Dry run: This issue passed the CQ dry run.

[jam, 2016-08-22 20:56:37]

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

[commit-bot: I haz the power, 2016-08-22 20:57:26]

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-08-22 21:26:52]

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

[commit-bot: I haz the power, 2016-08-22 21:26:53]

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

[jam, 2016-08-22 21:36:06]

Patchset #7 (id:140001) has been deleted

[jam, 2016-08-22 21:36:12]

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

[commit-bot: I haz the power, 2016-08-22 21:37:30]

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

[jam, 2016-08-22 21:51:41]

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

[commit-bot: I haz the power, 2016-08-22 21:52:39]

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

[jam, 2016-08-22 22:01:30]

ptal, now the SSLStatus is always stored on the pending NavigationEntry, instead
of sometimes on it and sometimes on the RenderFrameHost. To handle redirects,
there's a map on the NavigationEntry. This should be easier to read than the the
earlier patchset.

I have tried to go through the different codepaths that result from new
navigation entries after the DidCommit IPC to make sure that SSLStatus is always
updated. But per Nasko's earlier comment I may be missing something.

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1135:
new_entry->GetSSL() = ssl_status;
On 2016/08/15 22:18:22, nasko (out until 08-30) wrote:
> I think if we go that route, we need to also put the same update code in other
> RendererDidNavigate* methods. SamePage, ExistingPage an do a redirect on
hitting
> the same URL, so they can change the URL and SSL status.

Can you explain some more? How would SamePage or ExistingPage change the origin?
My naive understanding from reading this code is that the origin for the
committed entry should match the origin for the previous entry.

[jam, 2016-08-22 22:02:37]

Description was changed from

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

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

to

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

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

[jam, 2016-08-22 22:03:37]

Description was changed from

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

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

to

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
cursor for removing cert_ids which depend on having a renderer process during
navigate). After this change, there's only one use left in the renderer that
(devtools).

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

[commit-bot: I haz the power, 2016-08-22 23:19:01]

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

[commit-bot: I haz the power, 2016-08-22 23:19:02]

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

[jam, 2016-08-22 23:27:13]

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

[commit-bot: I haz the power, 2016-08-22 23:27:52]

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-08-23 01:58:57]

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

[commit-bot: I haz the power, 2016-08-23 01:58:58]

Dry run: This issue passed the CQ dry run.

[jam, 2016-08-25 01:12:04]

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

[commit-bot: I haz the power, 2016-08-25 01:12:48]

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-08-25 03:23:02]

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

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

Dry run: This issue passed the CQ dry run.

[jam, 2016-08-25 03:35:17]

ptal, the SSLStatus is now stored on the NavigationHandle per your suggestion

[nasko, 2016-08-25 18:09:48]

Overall it looks good and I think it is the better approach. Thanks for doing
it!

I still think we need to pass through the URL though in non-PlzNavigate modes,
as there are still possible race conditions and we want to avoid them.

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:439:
navigation_handle->UpdateSSLCertId(new_cert_id);
I think it is still possible to have race conditions between creation of
NavigationHandle and activity on the IO thread with non-PlzNavigate. So I think
your old approach of passing along the URL as well I think is still a good idea.

If we've made a network request for foo.com and in the meantime the user clicks
a link to bar.com, we will emit DidStartProvisionalLoad, which will delete the
old NavigationHandle and create a new one. The IO thread might not have
cancelled the request by that time and the task makes it to the UI thread and we
will put the foo.com SSLStatus on bar.com NavigationHandle.

[nasko, 2016-08-25 18:10:17]

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

[nasko, 2016-08-25 18:10:18]

Also adding clamy@, as she knows the NavigationHandle code better than I do.

[jam, 2016-08-25 21:56:46]

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

[jam, 2016-08-25 21:56:46]

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:439:
navigation_handle->UpdateSSLCertId(new_cert_id);
On 2016/08/25 18:09:48, nasko (out until 08-30) wrote:
> I think it is still possible to have race conditions between creation of
> NavigationHandle and activity on the IO thread with non-PlzNavigate. So I
think
> your old approach of passing along the URL as well I think is still a good
idea.
> 
> If we've made a network request for http://foo.com and in the meantime the
user clicks
> a link to http://bar.com, we will emit DidStartProvisionalLoad, which will
delete the
> old NavigationHandle and create a new one. The IO thread might not have
> cancelled the request by that time and the task makes it to the UI thread and
we
> will put the http://foo.com SSLStatus on http://bar.com NavigationHandle.

this method is just for one case (cross-site transfer). I added a URL check.

However for the other non-plz navigate case in NavigationResourceThrottle, does
anything need to be done there? It looks like it already calls
NavigationHandleImpl::WillProcessResponse without any URL checks, so I think the
race condition you mention doesn't apply there?

[commit-bot: I haz the power, 2016-08-25 21:57:52]

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

[clamy, 2016-08-25 23:25:15]

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:439:
navigation_handle->UpdateSSLCertId(new_cert_id);
On 2016/08/25 21:56:46, jam wrote:
> On 2016/08/25 18:09:48, nasko (out until 08-30) wrote:
> > I think it is still possible to have race conditions between creation of
> > NavigationHandle and activity on the IO thread with non-PlzNavigate. So I
> think
> > your old approach of passing along the URL as well I think is still a good
> idea.
> > 
> > If we've made a network request for http://foo.com and in the meantime the
> user clicks
> > a link to http://bar.com, we will emit DidStartProvisionalLoad, which will
> delete the
> > old NavigationHandle and create a new one. The IO thread might not have
> > cancelled the request by that time and the task makes it to the UI thread
and
> we
> > will put the http://foo.com SSLStatus on http://bar.com NavigationHandle.
> 
> this method is just for one case (cross-site transfer). I added a URL check.
> 
> However for the other non-plz navigate case in NavigationResourceThrottle,
does
> anything need to be done there? It looks like it already calls
> NavigationHandleImpl::WillProcessResponse without any URL checks, so I think
the
> race condition you mention doesn't apply there?

Regarding the possible race condition, the issue came up when I was writing the
code. Normally there shouldn't be a race condition. When we click on a link to
bar.com, we will delete the the provisional DocumentLoader for foo.com before we
send the DidStartProvisionalLoad IPC. Deleting the provisional DocumentLoader
will also cancel any request it had made, issuing an IPC to cancel the request
on the IO thread.

Since all browser IPCs are synchronized on the IO thread, this means that the
IPC to abort the current request will be received before the
DidStartProvisionalLoad IPC on the Io thread, and in particular before the
DidStartProvisionalLoad IPC is passed to the UI thread where it will create the
new NavigationHandle. Similarly, the IPC to request the main resource for the
new navigation is sent after DidStartProvisionalLoad, so by the time the new
request wants to reach the NavigationHandle on the UI thread the
NavigationHandle has already been created.

[Avi (use Gerrit), 2016-08-25 23:36:00]

Random drive-by

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:442: 
extra blank line

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

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

[commit-bot: I haz the power, 2016-08-25 23:52:12]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-08-26 00:06:57]

Thanks! The code passing the SSLStatus to the NavigationHandle looks good. A few
questions below.

https://codereview.chromium.org/2239273002/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (left):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:929: if
(!DeserializeSecurityInfo(params.security_info, &details->ssl_status)) {
Considering that we are no longer enforcing proper deserialization of the
security info, should it be removed from
FrameHostMsg_DidCommitProvisionalLoad_Params?

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.h (right):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/navigation_resource_handler.h:60: CertStore* cert_store_;
Why can't we use CertStore::GetInstance()?

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:46: CertStore*
cert_store_;
Same question as in NavigationResourceHandler about using
CertStore::GetInstance().

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:439:
navigation_handle->UpdateSSLCertId(new_cert_id);
On 2016/08/25 23:25:14, clamy wrote:
> On 2016/08/25 21:56:46, jam wrote:
> > On 2016/08/25 18:09:48, nasko (out until 08-30) wrote:
> > > I think it is still possible to have race conditions between creation of
> > > NavigationHandle and activity on the IO thread with non-PlzNavigate. So I
> > think
> > > your old approach of passing along the URL as well I think is still a good
> > idea.
> > > 
> > > If we've made a network request for http://foo.com and in the meantime the
> > user clicks
> > > a link to http://bar.com, we will emit DidStartProvisionalLoad, which will
> > delete the
> > > old NavigationHandle and create a new one. The IO thread might not have
> > > cancelled the request by that time and the task makes it to the UI thread
> and
> > we
> > > will put the http://foo.com SSLStatus on http://bar.com NavigationHandle.
> > 
> > this method is just for one case (cross-site transfer). I added a URL check.
> > 
> > However for the other non-plz navigate case in NavigationResourceThrottle,
> does
> > anything need to be done there? It looks like it already calls
> > NavigationHandleImpl::WillProcessResponse without any URL checks, so I think
> the
> > race condition you mention doesn't apply there?
> 
> Regarding the possible race condition, the issue came up when I was writing
the
> code. Normally there shouldn't be a race condition. When we click on a link to
> http://bar.com, we will delete the the provisional DocumentLoader for
http://foo.com before we
> send the DidStartProvisionalLoad IPC. Deleting the provisional DocumentLoader
> will also cancel any request it had made, issuing an IPC to cancel the request
> on the IO thread.
> 
> Since all browser IPCs are synchronized on the IO thread, this means that the
> IPC to abort the current request will be received before the
> DidStartProvisionalLoad IPC on the Io thread, and in particular before the
> DidStartProvisionalLoad IPC is passed to the UI thread where it will create
the
> new NavigationHandle. Similarly, the IPC to request the main resource for the
> new navigation is sent after DidStartProvisionalLoad, so by the time the new
> request wants to reach the NavigationHandle on the UI thread the
> NavigationHandle has already been created.

Looking at the code some more, I'm wondering if it would be possible to update
the certificate  on the UI thread first before doing it on the IO thread. We
have access to the NavigationHandle at the moment we transfer the navigation, so
we can update the certificate in the cert store at that moment (the CertStore
can be accessed from the UI thread as well). Then when we inform the IO thread
that the navigation is transferring, passing along the new cert id. This would
avoid some potential trickiness of the NavigationHandle having been transferred
to a new RFH but the current request still having ids for the old RFH.

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1101:
new_entry->GetSSL() = handle->ssl_status();
nit: wouldn't using a setter for the ssl status be more readable?

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:226: void
UpdateSSLCertId(int new_cert_id);
@nasko:
1) Is it still necessary to update the cert id when transferring to a new
process if we no longer rely on what that new process is ending us?
2) In PlzNavigate mode, when we end up behaving like a transfer navigation (ie
we have a renderer-initiated navigation that ends up committing in a different
process) should we also update the cert id?

[clamy, 2016-08-26 00:08:26]

Ah and i realized I made a mistake and my comments are spilled over two
patchset, sorry about that.

[nasko, 2016-08-26 01:10:57]

Thanks John for changing this CL a few times! LGTM

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:226: void
UpdateSSLCertId(int new_cert_id);
On 2016/08/26 00:06:57, clamy wrote:
> @nasko:
> 1) Is it still necessary to update the cert id when transferring to a new
> process if we no longer rely on what that new process is ending us?

Yes, since we need to look up the same cert in a different CertStore.

> 2) In PlzNavigate mode, when we end up behaving like a transfer navigation (ie
> we have a renderer-initiated navigation that ends up committing in a different
> process) should we also update the cert id?

As we discussed in person with estark@, we will likely get rid of cert id and
potentially CertStore, so we can make it work well with PlzNavigate.

[jam, 2016-08-26 03:36:43]

https://codereview.chromium.org/2239273002/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (left):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:929: if
(!DeserializeSecurityInfo(params.security_info, &details->ssl_status)) {
On 2016/08/26 00:06:57, clamy wrote:
> Considering that we are no longer enforcing proper deserialization of the
> security info, should it be removed from
> FrameHostMsg_DidCommitProvisionalLoad_Params?

ah yes, thanks done

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.h (right):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/navigation_resource_handler.h:60: CertStore* cert_store_;
On 2016/08/26 00:06:57, clamy wrote:
> Why can't we use CertStore::GetInstance()?

Like RDH and ResourceLoader, both support getting a mock cert store for tests
which is retrieved in RDHImpl::GetCertStore.

CertStore will be removed in followup patches.

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:439:
navigation_handle->UpdateSSLCertId(new_cert_id);
On 2016/08/25 23:25:14, clamy wrote:
> On 2016/08/25 21:56:46, jam wrote:
> > On 2016/08/25 18:09:48, nasko (out until 08-30) wrote:
> > > I think it is still possible to have race conditions between creation of
> > > NavigationHandle and activity on the IO thread with non-PlzNavigate. So I
> > think
> > > your old approach of passing along the URL as well I think is still a good
> > idea.
> > > 
> > > If we've made a network request for http://foo.com and in the meantime the
> > user clicks
> > > a link to http://bar.com, we will emit DidStartProvisionalLoad, which will
> > delete the
> > > old NavigationHandle and create a new one. The IO thread might not have
> > > cancelled the request by that time and the task makes it to the UI thread
> and
> > we
> > > will put the http://foo.com SSLStatus on http://bar.com NavigationHandle.
> > 
> > this method is just for one case (cross-site transfer). I added a URL check.
> > 
> > However for the other non-plz navigate case in NavigationResourceThrottle,
> does
> > anything need to be done there? It looks like it already calls
> > NavigationHandleImpl::WillProcessResponse without any URL checks, so I think
> the
> > race condition you mention doesn't apply there?
> 
> Regarding the possible race condition, the issue came up when I was writing
the
> code. Normally there shouldn't be a race condition. When we click on a link to
> http://bar.com, we will delete the the provisional DocumentLoader for
http://foo.com before we
> send the DidStartProvisionalLoad IPC. Deleting the provisional DocumentLoader
> will also cancel any request it had made, issuing an IPC to cancel the request
> on the IO thread.
> 
> Since all browser IPCs are synchronized on the IO thread, this means that the
> IPC to abort the current request will be received before the
> DidStartProvisionalLoad IPC on the Io thread, and in particular before the
> DidStartProvisionalLoad IPC is passed to the UI thread where it will create
the
> new NavigationHandle. Similarly, the IPC to request the main resource for the
> new navigation is sent after DidStartProvisionalLoad, so by the time the new
> request wants to reach the NavigationHandle on the UI thread the
> NavigationHandle has already been created.

The first paragraph makes it sound like there's a race condition but the second
paragraph makes it sound like there isn't, am I reading this correctly?

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:439:
navigation_handle->UpdateSSLCertId(new_cert_id);
On 2016/08/26 00:06:57, clamy wrote:
> On 2016/08/25 23:25:14, clamy wrote:
> > On 2016/08/25 21:56:46, jam wrote:
> > > On 2016/08/25 18:09:48, nasko (out until 08-30) wrote:
> > > > I think it is still possible to have race conditions between creation of
> > > > NavigationHandle and activity on the IO thread with non-PlzNavigate. So
I
> > > think
> > > > your old approach of passing along the URL as well I think is still a
good
> > > idea.
> > > > 
> > > > If we've made a network request for http://foo.com and in the meantime
the
> > > user clicks
> > > > a link to http://bar.com, we will emit DidStartProvisionalLoad, which
will
> > > delete the
> > > > old NavigationHandle and create a new one. The IO thread might not have
> > > > cancelled the request by that time and the task makes it to the UI
thread
> > and
> > > we
> > > > will put the http://foo.com SSLStatus on http://bar.com
NavigationHandle.
> > > 
> > > this method is just for one case (cross-site transfer). I added a URL
check.
> > > 
> > > However for the other non-plz navigate case in NavigationResourceThrottle,
> > does
> > > anything need to be done there? It looks like it already calls
> > > NavigationHandleImpl::WillProcessResponse without any URL checks, so I
think
> > the
> > > race condition you mention doesn't apply there?
> > 
> > Regarding the possible race condition, the issue came up when I was writing
> the
> > code. Normally there shouldn't be a race condition. When we click on a link
to
> > http://bar.com, we will delete the the provisional DocumentLoader for
> http://foo.com before we
> > send the DidStartProvisionalLoad IPC. Deleting the provisional
DocumentLoader
> > will also cancel any request it had made, issuing an IPC to cancel the
request
> > on the IO thread.
> > 
> > Since all browser IPCs are synchronized on the IO thread, this means that
the
> > IPC to abort the current request will be received before the
> > DidStartProvisionalLoad IPC on the Io thread, and in particular before the
> > DidStartProvisionalLoad IPC is passed to the UI thread where it will create
> the
> > new NavigationHandle. Similarly, the IPC to request the main resource for
the
> > new navigation is sent after DidStartProvisionalLoad, so by the time the new
> > request wants to reach the NavigationHandle on the UI thread the
> > NavigationHandle has already been created.
> 
> Looking at the code some more, I'm wondering if it would be possible to update
> the certificate  on the UI thread first before doing it on the IO thread.

which method are you referring to?

> We
> have access to the NavigationHandle at the moment we transfer the navigation,
so
> we can update the certificate in the cert store at that moment (the CertStore
> can be accessed from the UI thread as well). Then when we inform the IO thread
> that the navigation is transferring, passing along the new cert id. This would
> avoid some potential trickiness of the NavigationHandle having been
transferred
> to a new RFH but the current request still having ids for the old RFH.

by "ids" do you mean the cert id? I'm not sure what the issue is, since they
won't be used until there's another IO to UI thread hop when a commit IPC is
received from the new renderer

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1101:
new_entry->GetSSL() = handle->ssl_status();
On 2016/08/26 00:06:57, clamy wrote:
> nit: wouldn't using a setter for the ssl status be more readable?

yes most definitely (it's against the google style guide afaik); this is just
how the method was written many years ago.

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:442: 
On 2016/08/25 23:36:00, Avi wrote:
> extra blank line

Done.

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2239273002/diff/240001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:226: void
UpdateSSLCertId(int new_cert_id);
On 2016/08/26 01:10:57, nasko (out until 08-30) wrote:
> On 2016/08/26 00:06:57, clamy wrote:
> > @nasko:
> > 1) Is it still necessary to update the cert id when transferring to a new
> > process if we no longer rely on what that new process is ending us?
> 
> Yes, since we need to look up the same cert in a different CertStore.
> 
> > 2) In PlzNavigate mode, when we end up behaving like a transfer navigation
(ie
> > we have a renderer-initiated navigation that ends up committing in a
different
> > process) should we also update the cert id?

It was my understanding that this transfer scenario doesn't exist for
plznavigate, because when the process is decided in the browser process it won't
pick a temporary incorrect one as we do now for the non-plznavigate case?

> 
> As we discussed in person with estark@, we will likely get rid of cert id and
> potentially CertStore, so we can make it work well with PlzNavigate.

yep this is what i'm working towards (removal of cert store), since it's not
compatible with plznavigate.

[jam, 2016-08-26 03:42:13]

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

[commit-bot: I haz the power, 2016-08-26 03:42:33]

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-08-26 06:00:08]

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

[commit-bot: I haz the power, 2016-08-26 06:00:09]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-08-26 18:14:30]

Thanks! Lgtm with one nit.

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:439:
navigation_handle->UpdateSSLCertId(new_cert_id);
On 2016/08/26 03:36:43, jam wrote:
> On 2016/08/25 23:25:14, clamy wrote:
> > On 2016/08/25 21:56:46, jam wrote:
> > > On 2016/08/25 18:09:48, nasko (out until 08-30) wrote:
> > > > I think it is still possible to have race conditions between creation of
> > > > NavigationHandle and activity on the IO thread with non-PlzNavigate. So
I
> > > think
> > > > your old approach of passing along the URL as well I think is still a
good
> > > idea.
> > > > 
> > > > If we've made a network request for http://foo.com and in the meantime
the
> > > user clicks
> > > > a link to http://bar.com, we will emit DidStartProvisionalLoad, which
will
> > > delete the
> > > > old NavigationHandle and create a new one. The IO thread might not have
> > > > cancelled the request by that time and the task makes it to the UI
thread
> > and
> > > we
> > > > will put the http://foo.com SSLStatus on http://bar.com
NavigationHandle.
> > > 
> > > this method is just for one case (cross-site transfer). I added a URL
check.
> > > 
> > > However for the other non-plz navigate case in NavigationResourceThrottle,
> > does
> > > anything need to be done there? It looks like it already calls
> > > NavigationHandleImpl::WillProcessResponse without any URL checks, so I
think
> > the
> > > race condition you mention doesn't apply there?
> > 
> > Regarding the possible race condition, the issue came up when I was writing
> the
> > code. Normally there shouldn't be a race condition. When we click on a link
to
> > http://bar.com, we will delete the the provisional DocumentLoader for
> http://foo.com before we
> > send the DidStartProvisionalLoad IPC. Deleting the provisional
DocumentLoader
> > will also cancel any request it had made, issuing an IPC to cancel the
request
> > on the IO thread.
> > 
> > Since all browser IPCs are synchronized on the IO thread, this means that
the
> > IPC to abort the current request will be received before the
> > DidStartProvisionalLoad IPC on the Io thread, and in particular before the
> > DidStartProvisionalLoad IPC is passed to the UI thread where it will create
> the
> > new NavigationHandle. Similarly, the IPC to request the main resource for
the
> > new navigation is sent after DidStartProvisionalLoad, so by the time the new
> > request wants to reach the NavigationHandle on the UI thread the
> > NavigationHandle has already been created.
> 
> The first paragraph makes it sound like there's a race condition but the
second
> paragraph makes it sound like there isn't, am I reading this correctly?

Hmm my first paragraph is not very clear. What I meant is that there is no race
condition due to to the ordering of IPCs on the IO thread that I explain in the
second paragraph. As long as you still have a URLRequest on the IO thread (and
we do), then the NavigationHandle on the UI thread matches the request on the IO
thread.

https://codereview.chromium.org/2239273002/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:439:
navigation_handle->UpdateSSLCertId(new_cert_id);
On 2016/08/26 03:36:43, jam wrote:
> On 2016/08/26 00:06:57, clamy wrote:
> > On 2016/08/25 23:25:14, clamy wrote:
> > > On 2016/08/25 21:56:46, jam wrote:
> > > > On 2016/08/25 18:09:48, nasko (out until 08-30) wrote:
> > > > > I think it is still possible to have race conditions between creation
of
> > > > > NavigationHandle and activity on the IO thread with non-PlzNavigate.
So
> I
> > > > think
> > > > > your old approach of passing along the URL as well I think is still a
> good
> > > > idea.
> > > > > 
> > > > > If we've made a network request for http://foo.com and in the meantime
> the
> > > > user clicks
> > > > > a link to http://bar.com, we will emit DidStartProvisionalLoad, which
> will
> > > > delete the
> > > > > old NavigationHandle and create a new one. The IO thread might not
have
> > > > > cancelled the request by that time and the task makes it to the UI
> thread
> > > and
> > > > we
> > > > > will put the http://foo.com SSLStatus on http://bar.com
> NavigationHandle.
> > > > 
> > > > this method is just for one case (cross-site transfer). I added a URL
> check.
> > > > 
> > > > However for the other non-plz navigate case in
NavigationResourceThrottle,
> > > does
> > > > anything need to be done there? It looks like it already calls
> > > > NavigationHandleImpl::WillProcessResponse without any URL checks, so I
> think
> > > the
> > > > race condition you mention doesn't apply there?
> > > 
> > > Regarding the possible race condition, the issue came up when I was
writing
> > the
> > > code. Normally there shouldn't be a race condition. When we click on a
link
> to
> > > http://bar.com, we will delete the the provisional DocumentLoader for
> > http://foo.com before we
> > > send the DidStartProvisionalLoad IPC. Deleting the provisional
> DocumentLoader
> > > will also cancel any request it had made, issuing an IPC to cancel the
> request
> > > on the IO thread.
> > > 
> > > Since all browser IPCs are synchronized on the IO thread, this means that
> the
> > > IPC to abort the current request will be received before the
> > > DidStartProvisionalLoad IPC on the Io thread, and in particular before the
> > > DidStartProvisionalLoad IPC is passed to the UI thread where it will
create
> > the
> > > new NavigationHandle. Similarly, the IPC to request the main resource for
> the
> > > new navigation is sent after DidStartProvisionalLoad, so by the time the
new
> > > request wants to reach the NavigationHandle on the UI thread the
> > > NavigationHandle has already been created.
> > 
> > Looking at the code some more, I'm wondering if it would be possible to
update
> > the certificate  on the UI thread first before doing it on the IO thread.
> 
> which method are you referring to?
> 
> > We
> > have access to the NavigationHandle at the moment we transfer the
navigation,
> so
> > we can update the certificate in the cert store at that moment (the
CertStore
> > can be accessed from the UI thread as well). Then when we inform the IO
thread
> > that the navigation is transferring, passing along the new cert id. This
would
> > avoid some potential trickiness of the NavigationHandle having been
> transferred
> > to a new RFH but the current request still having ids for the old RFH.
> 
> by "ids" do you mean the cert id? I'm not sure what the issue is, since they
> won't be used until there's another IO to UI thread hop when a commit IPC is
> received from the new renderer

It was just an optimization to avoid thread hops,. Since the issue with process
id that requires updating the cert should go away, I don't think it's worth it.

https://codereview.chromium.org/2239273002/diff/260001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2239273002/diff/260001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2685: int
render_process_id, render_frame_id;
nit: Maybe we should mention somewhere that the process & route id are those
corresponding to the RFH the navigation was transferred to, which now holds the
NavigationHandle corresponding to this navgation. I was wondering about it
myself, but went to check that we do update the ResourceRequestInfoImpl with the
right values before calling this function.

[estark, 2016-08-29 21:49:35]

lgtm. One step closer to killing CertStore, yay!!!

[jam, 2016-08-30 03:14:11]

The CQ bit was checked by jam@chromium.org

[jam, 2016-08-30 03:14:12]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2239273002/#ps260001
(title: "review comments")

[commit-bot: I haz the power, 2016-08-30 03:45: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-08-30 05:55:18]

Description was changed from

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
cursor for removing cert_ids which depend on having a renderer process during
navigate). After this change, there's only one use left in the renderer that
(devtools).

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

to

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
cursor for removing cert_ids which depend on having a renderer process during
navigate). After this change, there's only one use left in the renderer that
(devtools).

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

[commit-bot: I haz the power, 2016-08-30 05:55:20]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2016-08-30 05:56:52]

Description was changed from

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
cursor for removing cert_ids which depend on having a renderer process during
navigate). After this change, there's only one use left in the renderer that
(devtools).

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

to

==========
Don't use SSLStatus from FrameHostMsg_DidCommitProvisionalLoad and instead cache
it on the browser side.

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
cursor for removing cert_ids which depend on having a renderer process during
navigate). After this change, there's only one use left in the renderer that
(devtools).

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

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

[commit-bot: I haz the power, 2016-08-30 05:56:53]

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