Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1080593006:
Fix flaky browser test SSLUITest.BadCertFollowedByGoodCert (Closed)
DescriptionFix flaky browser test SSLUITest.BadCertFollowedByGoodCert
A flaky test was added in https://codereview.chromium.org/1058003004/
that involved modifying the root cert cache, which did not happen
reliably. This changes and simplifies the test to use two different
servers on the same host, so the test passes since it relies on
same-host not same-origin, but avoids modifying any caches.
R=rsleevi@chromium.org
BUG=480667
Committed: https://crrev.com/7299d59e226e8a5c8b46d36d7b9f9db7767f367f
Cr-Commit-Position: refs/heads/master@{#326840}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Removed extra includes #Messages
Total messages: 20 (7 generated)
jww@chromium.org changed reviewers: + rsleevi@chromium.org - davidben@chromium.org
Ryan, can you check out this change to the test? I actually think it's overall a simplification, so, you know, we've got that going for us.
rsleevi@chromium.org changed reviewers: + davidben@chromium.org
rsleevi@chromium.org changed required reviewers: + davidben@chromium.org
Deferring this one to David; he's the better reviewer, since I was the one who argued "Surely there won't be problems here" (or more aptly, "surely there won't be more problems then the problems we have")
https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:2335: ASSERT_EQ(https_server_expired_host, https_server_host); This isn't part of the contract of net::TestServer, so I'm uneasy about depending on it. I mean, it'll work, but it just adds to the 'hidden API' cost. David probably has thouhts here.
That's true, but there are also several years that depend on it. I can set the hosts explicitly, if that would be better. On Fri, Apr 24, 2015, 4:03 AM <rsleevi@chromium.org> wrote: > > > https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > > https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... > chrome/browser/ssl/ssl_browser_tests.cc:2335 > <https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow...> > : > ASSERT_EQ(https_server_expired_host, https_server_host); > This isn't part of the contract of net::TestServer, so I'm uneasy about > depending on it. > > I mean, it'll work, but it just adds to the 'hidden API' cost. David > probably has thouhts here. > > https://codereview.chromium.org/1080593006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/24 14:00:32, jww wrote: > That's true, but there are also several years that depend on it. I can set > the hosts explicitly, if that would be better. Well, no, that's just trading one wrong for the other wrong, since the point is that the host come from the test, and we can and do change it (Emily just ran into issues where tests hardcoded the host) I'm hoping David has a cleverer answer. My gut is that we could add to the RuleBasedHostResolver the added information about the hostname, but I'm not sure if the interface is setup to map hostname (what you'd hardcode for tests) -> hostname (what the test server responds on), or if it's only possible to do hostname -> ip address. But it's "an" option. To be clear, I don't think I'd block the CL on that, because I think the alternatives (such as hardcoding) is explicitly worse, but it was a "Huh, I wonder if there's a better way to do what you want to do, because what you're doing isn't ideal"
lgtm. I'm not too worried about the hostname thing. It means we'll need to change the two hostnames in tandem if we ever do, but that's doesn't seem too big a deal to me. https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:45: #include "content/public/browser/render_process_host.h" No longer needed. https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:59: #include "net/cert/test_root_certs.h" No longer needed. https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:62: #include "net/dns/mock_host_resolver.h" Not sure the two host resolver includes were ever needed. https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:63: #include "net/http/http_transaction_factory.h" No longer needed. https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:67: #include "net/url_request/url_request_context_getter.h" No longer needed. https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:2335: ASSERT_EQ(https_server_expired_host, https_server_host); On 2015/04/24 11:03:40, Ryan Sleevi wrote: > This isn't part of the contract of net::TestServer, so I'm uneasy about > depending on it. > > I mean, it'll work, but it just adds to the 'hidden API' cost. David probably > has thouhts here. I think this is fine. We'll notice if it breaks and "CERT_EXPIRED and CERT_OK differ only the notAfter field" is a fine assumption to make, IMO.
Thanks, and sorry about forgetting to remove those includes! https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:45: #include "content/public/browser/render_process_host.h" On 2015/04/24 14:53:00, David Benjamin (OOO sick) wrote: > No longer needed. Done. https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:59: #include "net/cert/test_root_certs.h" On 2015/04/24 14:53:00, David Benjamin (OOO sick) wrote: > No longer needed. Done. https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:62: #include "net/dns/mock_host_resolver.h" On 2015/04/24 14:53:00, David Benjamin (OOO sick) wrote: > Not sure the two host resolver includes were ever needed. Yeah, I added those by accident (or rather, forgot to remove them after I was messing around with ideas). Removed. https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:63: #include "net/http/http_transaction_factory.h" On 2015/04/24 14:53:00, David Benjamin (OOO sick) wrote: > No longer needed. Done. https://codereview.chromium.org/1080593006/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:67: #include "net/url_request/url_request_context_getter.h" On 2015/04/24 14:53:00, David Benjamin (OOO sick) wrote: > No longer needed. Done.
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1080593006/#ps20001 (title: "Removed extra includes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1080593006/20001
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...)
lgtm
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1080593006/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7299d59e226e8a5c8b46d36d7b9f9db7767f367f Cr-Commit-Position: refs/heads/master@{#326840} |
