Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
887223005:
Skip interstitials and don't block requests for localhost SSL errors (Closed)
DescriptionSkip interstitials and don't block requests for localhost SSL errors
For now this behavior is behind a flag, because there are some weird
cases where localhost certificate errors could indicate a real security
problem (see https://code.google.com/p/chromium/issues/detail?id=282927
for more detail).
BUG=282927
TEST=Set up HTTPS server on localhost with a bad certificate, visit
https://localhost and observe no interstitial
Committed: https://crrev.com/4b003b337d81dc88d4c16f5d5751cfff8064c9bf
Cr-Commit-Position: refs/heads/master@{#316347}
Patch Set 1 #Patch Set 2 : Tweaked flag placement and added histograms.xml entry #
Total comments: 2
Patch Set 3 : addressed Adrienne's comments #Patch Set 4 : Added warning in console on localhost SSL errors #
Total comments: 10
Patch Set 5 : fixes from previous round of feedback #
Total comments: 16
Patch Set 6 : latest round from jww and sleevi #
Total comments: 3
Patch Set 7 : Move console warning to chrome/ from content/ #Patch Set 8 : oops, this one includes the whole diff, not just part of it #
Total comments: 2
Patch Set 9 : Move kAllowInsecureLocalhost switch into chrome/ #Patch Set 10 : Remove unnecessary #include #
Total comments: 2
Patch Set 11 : relocated DidFinishDocumentLoad override #Patch Set 12 : rebase #Patch Set 13 : add license to test script #Patch Set 14 : rebase again #Messages
Total messages: 62 (17 generated)
estark@chromium.org changed reviewers: + felt@chromium.org, jww@chromium.org
A few questions/notes I have: * I'm not sure if what I did was sufficient to add the flag. On https://codereview.chromium.org/369703002/, for example, I see a lot of changes to content_settings_* files; I'm not sure what a content setting is and if I am supposed to be adding one. (I followed the instructions here: http://www.chromium.org/developers/design-documents/experiments) * As far as the actual special-casing for localhost, I put it in ChromeSSLHostStateDelegate::QueryPolicy -- but I wonder if that's wrong and it should go a level up at SSLPolicy::OnCertError instead. * If the check for localhost is in fact in the right place, then I think I should probably move the test into chrome_ssl_host_state_delegate_test.cc. * Any suggestions for other people who should review?
Oh, by the way! This is obviously just the first part of the change; the next step will be to add a nag statement about configuring SSL properly.
On 2015/02/02 22:19:14, emily wrote: > A few questions/notes I have: > * I'm not sure if what I did was sufficient to add the flag. On > https://codereview.chromium.org/369703002/, for example, I see a lot of changes > to content_settings_* files; I'm not sure what a content setting is and if I am > supposed to be adding one. (I followed the instructions here: > http://www.chromium.org/developers/design-documents/experiments) You don't need to do any of the content settings changes. The way you did it here looks correct. > * As far as the actual special-casing for localhost, I put it in > ChromeSSLHostStateDelegate::QueryPolicy -- but I wonder if that's wrong and it > should go a level up at SSLPolicy::OnCertError instead. QueryPolicy seems right to me; this is a form of a policy, and OnCertError should just obliviously handle whatever the policy determination is. > * If the check for localhost is in fact in the right place, then I think I > should probably move the test into chrome_ssl_host_state_delegate_test.cc. I like the UI test you have here, because it is testing the end outcome that we want in terms of UI state. With that being said I agree you should also add a test to chrome_ssl_host_state_delegate_test, in the style of those tests. > * Any suggestions for other people who should review? For this particular CL, I suggest adding Ryan Sleevi. He's a good reviewer and may have an opinion about QueryPolicy vs OnCertError. (If he does, take his opinion over mine.)
https://codereview.chromium.org/887223005/diff/20001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/887223005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:330: // We always let certificate errors on localhost through; they are nit: Given that "we" can sometimes be ambiguous, a standard Chrome recommendation is to rephrase to avoid it. another nit: Since this is behind a flag, we aren't *always* letting certificate errors on localhost through.
estark@chromium.org changed reviewers: + rsleevi@chromium.org
addressed Adrienne's comments and adding rsleevi as a reviewer
On 2015/02/03 18:31:12, emily wrote: > addressed Adrienne's comments and adding rsleevi as a reviewer FYI the convention is to mark each individual comment "Done", I think there is a button for it on each comment.
aha, thanks Adrienne https://codereview.chromium.org/887223005/diff/20001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/887223005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:330: // We always let certificate errors on localhost through; they are On 2015/02/03 06:23:17, felt wrote: > nit: Given that "we" can sometimes be ambiguous, a standard Chrome > recommendation is to rephrase to avoid it. > > another nit: Since this is behind a flag, we aren't *always* letting certificate > errors on localhost through. Done.
https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:335: if (allowLocalhost && url.DomainIs("localhost")) { will it always be normalized to "localhost"? what about 127.0.0.1? are there other addresses in the RFC that should count too?
On 2015/02/04 08:06:54, felt wrote: > will it always be normalized to "localhost"? what about 127.0.0.1? are there > other addresses in the RFC that should count too? Hmm, so I looked through RFCs 1918 and 2606, and also found a relevant pre-existing utility function (net::IsLocalhost, https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util....), and now I'm completely confused and don't know what to do! Here are my thoughts: - I don't think we should include anything from RFC 1918 in this change, i.e. I think we should stick to loopback only and err on the side of being too cautious instead of accidentally allowing requests that should be blocked. - From RFC 2606, I do think we should include .localhost, but not any of the others (it doesn't seem to me that there's anything prohibiting .test from resolving to a site on the public internet, for example). - 127.0.0.0/8 and ::1 seem like reasonable things to include. - I would like to use the existing utility function net::IsLocalhost rather than repeating the code, but net::IsLocalhost doesn't check for the .localhost TLD (not sure why). What do you think of modifying net::isLocalhost to check for .localhost? It feels like kind of a scary change to me, since so much other code uses that function. (Maybe I'll ask whoever touched that function last and see if there's a reason it's not included?)
On 2015/02/04 16:17:48, emily wrote: > What do you think of modifying net::isLocalhost to check for .localhost? It > feels like kind of a scary change to me, since so much other code uses that > function. (Maybe I'll ask whoever touched that function last and see if there's > a reason it's not included?) RFC 6761 is the new 2606. I think we just overlooked localhost, although to be fair, I put little trust in DNS libraries to get the provisos of 6.3 correct. I'm also not sure what the broader implications would be for the rest of the net-stack - I don't think anything treats .localhost special today. I would say let's leave .localhost treatment as a follow-up (and may be a good way to dig into the broader Chromium networking/security flow), and just use the net::IsLocalhost function as-is for now.
Mostly LG, but I need to dig through source archaeology on the localhost issue for UI tests. https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:333: bool allowLocalhost = base::CommandLine::ForCurrentProcess()-> nit: naming wise, this should be allow_localhost (see the naming rules for http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names ) This differs from our JS naming rules (which this would be correct in - see https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Naming ) https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:638: scoped_refptr<net::X509Certificate> google_cert = GetGoogleCert(); Not your fault, other than I never noticed until this CL, but we (ideally) should *not* be using a Google cert here. Those aren't automatically generated and thus can be a pain to update. If you're wanting bonus kudos, a (hopefully trivial) task would be to replace the google cert w/ one of our "automatically" (well, script-maintained) certs, such as ok_cert.pem or something. You can see which certs are which by looking at https://chromium.googlesource.com/chromium/src/+/master/net/data/ssl/certific... https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_browser_tests.cc:561: ui_test_utils::NavigateToURL(browser(), url); Unfortunately, I think this test is going to be on the flaky / hard to maintain side. We've run into issues with "localhost" in the past. It's going to take me a while to dig up the source archaeology on this (and it may have only been relevant to Chrome Frame). The TLDR is Windows is awful. https://codereview.chromium.org/887223005/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/887223005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:2752: // kAllowInsecureLocalhost flag is set? That's my gut feeling too. Note, our use of TODO at Google is a little... weirdish. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments
On 2015/02/04 19:15:52, Ryan Sleevi wrote: > I would say let's leave .localhost treatment as a follow-up (and may be a good > way to dig into the broader Chromium networking/security flow), and just use the > net::IsLocalhost function as-is for now. Okay, that sounds good to me (and done in the latest patch set). Would filing a bug for the .localhost change be the right thing for me to do?
On 2015/02/05 02:54:41, emily wrote: > Okay, that sounds good to me (and done in the latest patch set). Would filing a > bug for the .localhost change be the right thing for me to do? Yup!
https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:333: bool allowLocalhost = base::CommandLine::ForCurrentProcess()-> On 2015/02/04 19:34:41, Ryan Sleevi wrote: > nit: naming wise, this should be allow_localhost (see the naming rules for > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names ) > > This differs from our JS naming rules (which this would be correct in - see > https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Naming ) Done. https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:335: if (allowLocalhost && url.DomainIs("localhost")) { On 2015/02/04 08:06:54, felt wrote: > will it always be normalized to "localhost"? what about 127.0.0.1? are there > other addresses in the RFC that should count too? Done -- changed to use net::IsLocalhost, which is "localhost" and 127.0.0.0/8, and maybe .localhost sometime in the future. https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:638: scoped_refptr<net::X509Certificate> google_cert = GetGoogleCert(); On 2015/02/04 19:34:41, Ryan Sleevi wrote: > Not your fault, other than I never noticed until this CL, but we (ideally) > should *not* be using a Google cert here. Those aren't automatically generated > and thus can be a pain to update. > > If you're wanting bonus kudos, a (hopefully trivial) task would be to replace > the google cert w/ one of our "automatically" (well, script-maintained) certs, > such as ok_cert.pem or something. You can see which certs are which by looking > at > https://chromium.googlesource.com/chromium/src/+/master/net/data/ssl/certific... Done. Just to make sure I understand: * The pain of using the manually updated cert is that if it expires (for example), the tests could break until someone goes in and manually replaces it? * I replaced the Google cert in these tests with ok_cert.pem, but I didn't change the hostnames that the tests uses (google.com, www.google.com, example.com). IIUC, that's okay because the code being tested doesn't actually look at the names in the cert, so I think the hostnames used by the tests don't actually have to have any relationship to the cert. But I just want to make sure that the change didn't break the tests in such a way that they pass but are no longer testing the thing they're supposed to be testing. https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/887223005/diff/60001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_browser_tests.cc:561: ui_test_utils::NavigateToURL(browser(), url); On 2015/02/04 19:34:41, Ryan Sleevi wrote: > Unfortunately, I think this test is going to be on the flaky / hard to maintain > side. > > We've run into issues with "localhost" in the past. It's going to take me a > while to dig up the source archaeology on this (and it may have only been > relevant to Chrome Frame). The TLDR is Windows is awful. Is there any chance that using 127.0.0.1 instead of "localhost" would make it more stable? https://codereview.chromium.org/887223005/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/887223005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:2752: // kAllowInsecureLocalhost flag is set? On 2015/02/04 19:34:42, Ryan Sleevi wrote: > That's my gut feeling too. > > Note, our use of TODO at Google is a little... weirdish. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments Done.
I'm not sure what's up with these trybot failures. Maybe it's wishful thinking but they seem kind of unrelated to this change (e.g. "FlashFullscreenInteractiveBrowserTest.FullscreenWithinTab_EscapeKeyExitsFullscreen").
On 2015/02/05 22:51:33, emily wrote: > I'm not sure what's up with these trybot failures. Maybe it's wishful thinking > but they seem kind of unrelated to this change (e.g. > "FlashFullscreenInteractiveBrowserTest.FullscreenWithinTab_EscapeKeyExitsFullscreen"). It's normal to see some unrelated trybot failures; maybe try again a few hours later and see whether it's still failing?
On 2015/02/06 08:20:30, felt wrote: > On 2015/02/05 22:51:33, emily wrote: > > I'm not sure what's up with these trybot failures. Maybe it's wishful thinking > > but they seem kind of unrelated to this change (e.g. > > > "FlashFullscreenInteractiveBrowserTest.FullscreenWithinTab_EscapeKeyExitsFullscreen"). > > It's normal to see some unrelated trybot failures; maybe try again a few hours > later and see whether it's still failing? Ok, cool, you're right -- they all passed when I re-ran. Also on the possibly-flaky test issue that rsleevi brought up, in IRC he mentioned that he found the old tests that he was thinking of (crbug.com/114369) and the issue was a Chrome Frame one, so I think the browser tests in this CL should be okay.
lgtm now, but wait for ryan's approval too :)
lgtm, too, modulo my two comments. https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:335: bool allow_localhost = base::CommandLine::ForCurrentProcess()-> The "*expired_previous_decision = false;" line should go before this code, otherwise the caller will have a mystery return value. https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:630: // Tests to make sure that localhost certificate errors are ignored or This is really nit picky, but you might want to reverse "...are ignored..." and "...treated as normal errors..." since the actual tests are in the reverse order (this confused me when I read your comment).
LGTM % a few nits https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:339: } Note: Consistent with the style in the rest of this file, you would omit the {} for the single-line if. ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals lists them as optional, so defer to local code patterns) https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:43: return net::ImportCertFromFile(net::GetTestCertsDirectory(), kOkCertFile); note: More for future reference, as this is OK here, but you may get reviews (*cough* I'm guilty of it especially) being like "Oh hey, there's this other thing in the code, you should fix it too" In general, you should do that in a separate CL. Depending on whether the reviewer flagged it as a "blocks this CL" or "nice follow-up cleanup" depends on which order it is. You can use git to have multiple reviews in progress (although sometimes it can get a little tricky with the rebase-y stuff), just one CL per branch So let's say I was a jerk and said "Fixing this blocks your localhost CL", and your original setup was master -> localhostBranch Fix it all up in a solo commit, so now you have master -> localhostBranch -> [extra commit to cleanup] `git rebase -i` to reorder master -> [extra commit to cleanup] -> localhostBranch `git checkout -b sleeviIsAJerk [extra commit hash]` `git cl upload` `git checkout localhostBranch` `git branch --set-upstream-to sleeviIsAJerk localhostBranch` `git cl upload` <-- will now diff sleeviIsAJerk...localhostBranch when uploading If you didn't set the upstream (since I always botch that command), you can also do `git cl upload sleeviIsAJerk` to do the appropriate inter-branch diff, you just have to remember it when uploading. I just mention all of it because it was my #1 switching to Chromium-git (when we still had Chromium-svn), and it's fairly common to find nits in existing code and be asked "While you're here, can you clean up [slightly-related-but-unrelated thing]" That was the context for my ok_cert.pem grump. But for this CL, it's small enough that I can deal :) https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:62: browser()->tab_strip_model()->GetActiveWebContents(); STYLE: This should be four spaces. You can run 'git cl format' and it should reformat per style guide. https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:71: *cert.get(), Is it necessary to call .get() on this, given that |cert| is a scoped_refptr? I would expect that operator* from scoped_refptr "Just Works", hence "*cert" should Just Work. https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_browser_tests.cc:571: // wasn't blocked by the certificate error). Prevailing code pattern wins here (meaning no change required), but I never let a "we" go by without parroting https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... https://codereview.chromium.org/887223005/diff/80001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/887223005/diff/80001/content/renderer/render_... content/renderer/render_frame_impl.cc:2758: bool is_cert_error = net::IsCertStatusError(ssl_status.cert_status); Note: We only block for IsCertStatusError && !IsCertStatusMinorError
New patchsets have been uploaded after l-g-t-m from felt@chromium.org,jww@chromium.org,rsleevi@chromium.org
https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:335: bool allow_localhost = base::CommandLine::ForCurrentProcess()-> On 2015/02/09 19:29:38, jww wrote: > The "*expired_previous_decision = false;" line should go before this code, > otherwise the caller will have a mystery return value. Done. https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:339: } On 2015/02/09 19:31:34, Ryan Sleevi wrote: > Note: Consistent with the style in the rest of this file, you would omit the {} > for the single-line if. > > ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals > lists them as optional, so defer to local code patterns) Done. https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:43: return net::ImportCertFromFile(net::GetTestCertsDirectory(), kOkCertFile); On 2015/02/09 19:31:34, Ryan Sleevi wrote: > note: More for future reference, as this is OK here, but you may get reviews > (*cough* I'm guilty of it especially) being like "Oh hey, there's this other > thing in the code, you should fix it too" > > In general, you should do that in a separate CL. Depending on whether the > reviewer flagged it as a "blocks this CL" or "nice follow-up cleanup" depends on > which order it is. > > You can use git to have multiple reviews in progress (although sometimes it can > get a little tricky with the rebase-y stuff), just one CL per branch > > So let's say I was a jerk and said "Fixing this blocks your localhost CL", and > your original setup was > > master -> localhostBranch > Fix it all up in a solo commit, so now you have > master -> localhostBranch -> [extra commit to cleanup] > `git rebase -i` to reorder > master -> [extra commit to cleanup] -> localhostBranch > `git checkout -b sleeviIsAJerk [extra commit hash]` > `git cl upload` > `git checkout localhostBranch` > `git branch --set-upstream-to sleeviIsAJerk localhostBranch` > `git cl upload` <-- will now diff sleeviIsAJerk...localhostBranch when uploading > > If you didn't set the upstream (since I always botch that command), you can also > do > `git cl upload sleeviIsAJerk` to do the appropriate inter-branch diff, you just > have to remember it when uploading. > > > I just mention all of it because it was my #1 switching to Chromium-git (when we > still had Chromium-svn), and it's fairly common to find nits in existing code > and be asked "While you're here, can you clean up > [slightly-related-but-unrelated thing]" > > That was the context for my ok_cert.pem grump. > > But for this CL, it's small enough that I can deal :) makes sense, will do next time! https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:62: browser()->tab_strip_model()->GetActiveWebContents(); On 2015/02/09 19:31:34, Ryan Sleevi wrote: > STYLE: This should be four spaces. > > You can run 'git cl format' and it should reformat per style guide. Done. That's very handy! https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:71: *cert.get(), On 2015/02/09 19:31:34, Ryan Sleevi wrote: > Is it necessary to call .get() on this, given that |cert| is a scoped_refptr? > > I would expect that operator* from scoped_refptr "Just Works", hence "*cert" > should Just Work. Looks like *cert does work. I'll submit that it in a separate CL, now that I know how to do that. :) https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:630: // Tests to make sure that localhost certificate errors are ignored or On 2015/02/09 19:29:38, jww wrote: > This is really nit picky, but you might want to reverse "...are ignored..." and > "...treated as normal errors..." since the actual tests are in the reverse order > (this confused me when I read your comment). Done. (I don't think it's that nit-picky, FWIW :) ) https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/887223005/diff/80001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_browser_tests.cc:571: // wasn't blocked by the certificate error). On 2015/02/09 19:31:34, Ryan Sleevi wrote: > Prevailing code pattern wins here (meaning no change required), but I never let > a "we" go by without parroting > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... Interesting thread! This is going to be a tough habit for me to break... but I can definitely see why it's preferable to avoid it. https://codereview.chromium.org/887223005/diff/80001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/887223005/diff/80001/content/renderer/render_... content/renderer/render_frame_impl.cc:2758: bool is_cert_error = net::IsCertStatusError(ssl_status.cert_status); On 2015/02/09 19:31:34, Ryan Sleevi wrote: > Note: We only block for IsCertStatusError && !IsCertStatusMinorError Done.
Is the next step here to make sure there's a LGTM from an OWNER of each file? Should I just pick somewhat randomly from each relevant OWNERS file, or do any of you all have recommendations?
On 2015/02/10 23:39:31, emily wrote: > Is the next step here to make sure there's a LGTM from an OWNER of each file? > Should I just pick somewhat randomly from each relevant OWNERS file, or do any > of you all have recommendations? 'git cl owners' will suggest owners for you to ensure coverage. There is also the butler on http://www.chromium.org/developers/useful-extensions that can help. Failing that, grab OWNERS where you need them.
estark@chromium.org changed reviewers: + davidben@chromium.org, mpearson@chromium.org
davidben@chromium.org: Hi! Could you please review changes in content/? mpearson@chromium.org: Could you please review the change to histograms.xml?
https://codereview.chromium.org/887223005/diff/100001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/887223005/diff/100001/content/renderer/render... content/renderer/render_frame_impl.cc:2745: // before going to production. This only pays attention to the navigation request and not subresources, right? (Since it's just looking at ds->request().url().) It might be better to key this on the commit point (where we switch documents), so didCommitProvisionalLoad. didFinishLoad is when the load event fires which depends on subresources and recursively on the load events of any child frames. I'd also suggest putting the logic in //chrome in a RenderFrameObserver::DidCommitProvisionalLoad rather than //content. The logic to let the certificate error through happens in //chrome, so it makes sense to put the corresponding warning there too. (ChromeRenderFrameObserver? Probably no need to attach a whole new one for a one-off thing like this.)
On 2015/02/11 01:14:43, emily wrote: > mailto:mpearson@chromium.org: Could you please review the change to histograms.xml? There are no changes to histograms.xml in this changelist.
On 2015/02/11 18:48:46, Mark P wrote: > On 2015/02/11 01:14:43, emily wrote: > > mailto:mpearson@chromium.org: Could you please review the change to > histograms.xml? > > There are no changes to histograms.xml in this changelist. I'm sorry about that -- I somehow got my branch in a bad state and patchset #7 only included changes from the latest commit, not the whole branch. Can you please look at Patch Set 8?
https://codereview.chromium.org/887223005/diff/100001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/887223005/diff/100001/content/renderer/render... content/renderer/render_frame_impl.cc:2745: // before going to production. On 2015/02/11 04:13:14, David Benjamin wrote: > This only pays attention to the navigation request and not subresources, right? > (Since it's just looking at ds->request().url().) It might be better to key this > on the commit point (where we switch documents), so didCommitProvisionalLoad. > didFinishLoad is when the load event fires which depends on subresources and > recursively on the load events of any child frames. > > I'd also suggest putting the logic in //chrome in a > RenderFrameObserver::DidCommitProvisionalLoad rather than //content. The logic > to let the certificate error through happens in //chrome, so it makes sense to > put the corresponding warning there too. (ChromeRenderFrameObserver? Probably no > need to attach a whole new one for a one-off thing like this.) Done -- mostly. DidCommitProvisionalLoad doesn't seem to work so well because the console gets cleared after that, so you only see the message if you have "Preserve log" checked. So I put it in DidFinishDocumentLoad, which seems to not have to wait for all subresources at least. What do you think?
histograms.xml lgtm
LGTM with one comment on command-line switch forwarding. https://codereview.chromium.org/887223005/diff/100001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/887223005/diff/100001/content/renderer/render... content/renderer/render_frame_impl.cc:2745: // before going to production. On 2015/02/11 18:54:42, emily wrote: > On 2015/02/11 04:13:14, David Benjamin wrote: > > This only pays attention to the navigation request and not subresources, > right? > > (Since it's just looking at ds->request().url().) It might be better to key > this > > on the commit point (where we switch documents), so didCommitProvisionalLoad. > > didFinishLoad is when the load event fires which depends on subresources and > > recursively on the load events of any child frames. > > > > I'd also suggest putting the logic in //chrome in a > > RenderFrameObserver::DidCommitProvisionalLoad rather than //content. The logic > > to let the certificate error through happens in //chrome, so it makes sense to > > put the corresponding warning there too. (ChromeRenderFrameObserver? Probably > no > > need to attach a whole new one for a one-off thing like this.) > > Done -- mostly. DidCommitProvisionalLoad doesn't seem to work so well because > the console gets cleared after that, so you only see the message if you have > "Preserve log" checked. So I put it in DidFinishDocumentLoad, which seems to not > have to wait for all subresources at least. What do you think? Huh. I guess some things are ordered funny. I wonder if we have other places where console messages key on the document's response info (some header or text encoding maybe), maybe within Blink itself, and when those trigger. *shrug* This seems fine too. https://codereview.chromium.org/887223005/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/887223005/diff/140001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1190: switches::kAllowInsecureLocalhost, We seem to be woefully inconsistent about whether we use this list or query something else, but it looks like //chrome forwards command-line flags using ChromeContentBrowserClient::AppendExtraCommandLineSwitches. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... Which... actually lets you do this without touching //content at all. :-)
New patchsets have been uploaded after l-g-t-m from mpearson@chromium.org,davidben@chromium.org
https://codereview.chromium.org/887223005/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/887223005/diff/140001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1190: switches::kAllowInsecureLocalhost, On 2015/02/11 19:41:35, David Benjamin wrote: > We seem to be woefully inconsistent about whether we use this list or query > something else, but it looks like //chrome forwards command-line flags using > ChromeContentBrowserClient::AppendExtraCommandLineSwitches. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > Which... actually lets you do this without touching //content at all. :-) Done. You've rendered yourself unnecessary as an OWNER here. :)
estark@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Could you please review changes in chrome/renderer/? Thank you!
LGTM https://codereview.chromium.org/887223005/diff/180001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_frame_observer.h (right): https://codereview.chromium.org/887223005/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_render_frame_observer.h:33: void DidFinishDocumentLoad() override; overrides should be grouped with where they are overriding the function from.
New patchsets have been uploaded after l-g-t-m from sky@chromium.org
https://codereview.chromium.org/887223005/diff/180001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_frame_observer.h (right): https://codereview.chromium.org/887223005/diff/180001/chrome/renderer/chrome_... chrome/renderer/chrome_render_frame_observer.h:33: void DidFinishDocumentLoad() override; On 2015/02/12 00:15:47, sky wrote: > overrides should be grouped with where they are overriding the function from. Done.
The CQ bit was checked by estark@chromium.org
The CQ bit was unchecked by estark@chromium.org
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/887223005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/887223005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/887223005/240001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/about_flags.cc:
While running git apply --index -3 -p1;
error: patch failed: chrome/browser/about_flags.cc:2173
Falling back to three-way merge...
Applied patch to 'chrome/browser/about_flags.cc' with conflicts.
U chrome/browser/about_flags.cc
Patch: chrome/browser/about_flags.cc
Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index
9010c1c1342a7c7448be67524b5c990c6c9c65f7..09f96387823d1a01a36e7bfb131e58665fb90817
100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -2173,6 +2173,13 @@ const Experiment kExperiments[] = {
SINGLE_VALUE_TYPE(data_reduction_proxy::switches::
kEnableDataReductionProxyLoFi)
},
+ {
+ "allow-insecure-localhost",
+ IDS_ALLOW_INSECURE_LOCALHOST,
+ IDS_ALLOW_INSECURE_LOCALHOST_DESCRIPTION,
+ kOsAll,
+ SINGLE_VALUE_TYPE(switches::kAllowInsecureLocalhost)
+ },
// NOTE: Adding new command-line switches requires adding corresponding
// entries to enum "LoginCustomFlags" in histograms.xml. See note in
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/887223005/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/4b003b337d81dc88d4c16f5d5751cfff8064c9bf Cr-Commit-Position: refs/heads/master@{#316347} |
