content/browser/ssl/ssl_policy_backend.cc - Issue 2225213004: Teach SSLHostStateDelegate about subresources with cert errors

jww 2016/08/11 18:50:15 Okay, this has me very confused. I think maybe tha

jam 2016/08/11 19:53:12 jww: since i got the next patch to review in this

estark 2016/08/11 20:58:38 Urgh, yeah, I tried to split up a giant CL into sm

Urgh, yeah, I tried to split up a giant CL into smallers CLs to make it easier to review and might have gone too far to the other side of the line. :) Sorry for the confusion, Joel. To explain a little more, this CL just gives SSLHostStateDelegate the ability to track subresources with cert errors, but doesn't actually tell SSLHostStateDelegate when a site has subresources with cert errors. The follow-up CL that jam linked to hooks everything up, so that when the renderer tells the browser that it loaded a subresource with cert errors, WebContents tells SSLManager who tells SSLPolicy who tells SSLPolicyBackend who tells SSLHostStateDelegate that there's CONTENT_WITH_CERT_ERRORS. Does that make any more sense? I'll add a TODO in SSLHostStateDelegate to explain this as well. (The TODO will be done in the follow-up CL, but I'll add it in case any people from the future are looking at this commit in isolation and become similarly confused.) (Also, next on my list of content/browser/ssl clean-up is to see if we can collapse SSLManager, SSLPolicy, and SSLPolicyBackend, since AFAICT there's no real reason for them to be split up and they all just kind of shuttle method calls from one to the next.) Finally, you raise a good point that these SSLManager/SSLPolicy/SSLPolicyBackend methods should be renamed. Once this CL and the follow-up CL land, HostRanInsecureContent() will handle only mixed content, not content with cert errors, so it should be renamed to HostRanMixedContent(), and similarly for DidHostRunInsecureContent(). I will file a bug for that but would like to do it in a follow-up CL, if it's okay with you, since there's already so many moving parts here.

jww 2016/08/11 21:13:24 Great, thanks for the clarifications!

On 2016/08/11 20:58:38, estark wrote: > On 2016/08/11 19:53:12, jam wrote: > > On 2016/08/11 18:50:15, jww wrote: > > > Okay, this has me very confused. I think maybe that you're planning a later > > > patch or something, but can you explain to me why this only has the > > possibility > > > of marking mixed content, and not cert errors? > > > > > > If this is just to simulate current behavior, then that's fine, but please > add > > a > > > comment somewhere to clarify it, and add a TODO saying that you're going to > > fix > > > it. > > > > > > If this code is only reachable by mixed content (although I don't think > that's > > > the case), that's also fine, but then I'd like to see the methods renamed to > > > reflect that, since otherwise it gives the impression that this magically > > > handles all cases. > > > > jww: since i got the next patch to review in this series, > > > https://codereview.chromium.org/2226363002/diff/1/content/browser/ssl/ssl_pol... > > does what you suspect of adding new methods which when it lands will make this > > easy to read i think > > Urgh, yeah, I tried to split up a giant CL into smallers CLs to make it easier > to review and might have gone too far to the other side of the line. :) Sorry > for the confusion, Joel. To explain a little more, this CL just gives > SSLHostStateDelegate the ability to track subresources with cert errors, but > doesn't actually tell SSLHostStateDelegate when a site has subresources with > cert errors. > > The follow-up CL that jam linked to hooks everything up, so that when the > renderer tells the browser that it loaded a subresource with cert errors, > WebContents tells SSLManager who tells SSLPolicy who tells SSLPolicyBackend who > tells SSLHostStateDelegate that there's CONTENT_WITH_CERT_ERRORS. > > Does that make any more sense? I'll add a TODO in SSLHostStateDelegate to > explain this as well. (The TODO will be done in the follow-up CL, but I'll add > it in case any people from the future are looking at this commit in isolation > and become similarly confused.) > > (Also, next on my list of content/browser/ssl clean-up is to see if we can > collapse SSLManager, SSLPolicy, and SSLPolicyBackend, since AFAICT there's no > real reason for them to be split up and they all just kind of shuttle method > calls from one to the next.) > > Finally, you raise a good point that these SSLManager/SSLPolicy/SSLPolicyBackend > methods should be renamed. Once this CL and the follow-up CL land, > HostRanInsecureContent() will handle only mixed content, not content with cert > errors, so it should be renamed to HostRanMixedContent(), and similarly for > DidHostRunInsecureContent(). I will file a bug for that but would like to do it > in a follow-up CL, if it's okay with you, since there's already so many moving > parts here.

Great, thanks for the clarifications!

jam 2016/08/11 21:14:04 That would be really great for readability. I've b

On 2016/08/11 20:58:38, estark wrote: > On 2016/08/11 19:53:12, jam wrote: > > On 2016/08/11 18:50:15, jww wrote: > > > Okay, this has me very confused. I think maybe that you're planning a later > > > patch or something, but can you explain to me why this only has the > > possibility > > > of marking mixed content, and not cert errors? > > > > > > If this is just to simulate current behavior, then that's fine, but please > add > > a > > > comment somewhere to clarify it, and add a TODO saying that you're going to > > fix > > > it. > > > > > > If this code is only reachable by mixed content (although I don't think > that's > > > the case), that's also fine, but then I'd like to see the methods renamed to > > > reflect that, since otherwise it gives the impression that this magically > > > handles all cases. > > > > jww: since i got the next patch to review in this series, > > > https://codereview.chromium.org/2226363002/diff/1/content/browser/ssl/ssl_pol... > > does what you suspect of adding new methods which when it lands will make this > > easy to read i think > > Urgh, yeah, I tried to split up a giant CL into smallers CLs to make it easier > to review and might have gone too far to the other side of the line. :) Sorry > for the confusion, Joel. To explain a little more, this CL just gives > SSLHostStateDelegate the ability to track subresources with cert errors, but > doesn't actually tell SSLHostStateDelegate when a site has subresources with > cert errors. > > The follow-up CL that jam linked to hooks everything up, so that when the > renderer tells the browser that it loaded a subresource with cert errors, > WebContents tells SSLManager who tells SSLPolicy who tells SSLPolicyBackend who > tells SSLHostStateDelegate that there's CONTENT_WITH_CERT_ERRORS. > > Does that make any more sense? I'll add a TODO in SSLHostStateDelegate to > explain this as well. (The TODO will be done in the follow-up CL, but I'll add > it in case any people from the future are looking at this commit in isolation > and become similarly confused.) > > (Also, next on my list of content/browser/ssl clean-up is to see if we can > collapse SSLManager, SSLPolicy, and SSLPolicyBackend, since AFAICT there's no > real reason for them to be split up and they all just kind of shuttle method > calls from one to the next.)

That would be really great for readability. I've been confused about the differences between them.

jww 2016/08/11 18:50:15 This is especially confusing. I would expect this

estark 2016/08/11 20:58:38 Same explanation as above, I think. In the follow-

Unified Diff: content/browser/ssl/ssl_policy_backend.cc