Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1818603002:
Keep track of ChannelIDService in CookieStore (Closed)
DescriptionKeep track of ChannelIDService in CookieStore
This is what I meant to do in https://codereview.chromium.org/1770983002/
BUG=548423
Committed: https://crrev.com/3876dd56009d12b12d1ac62acb6397598a893dca
Cr-Commit-Position: refs/heads/master@{#383853}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix check #
Total comments: 10
Patch Set 3 : Add DCHECK #
Total comments: 8
Patch Set 4 : rebase #Patch Set 5 : Add DCHECKs, add comments to LogChannelIDAndCookieStores #
Total comments: 1
Patch Set 6 : s/DCHECK(false)/NOTREACHED()/ #
Messages
Total messages: 24 (6 generated)
Description was changed from ========== Keep track of ChannelIDService in CookieStore BUG=548423 ========== to ========== Keep track of ChannelIDService in CookieStore This is what I meant to do in https://codereview.chromium.org/1770983002/ BUG=548423 ==========
nharper@chromium.org changed reviewers: + mattm@chromium.org, mmenke@chromium.org
https://codereview.chromium.org/1818603002/diff/1/net/url_request/url_request... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1818603002/diff/1/net/url_request/url_request... net/url_request/url_request_http_job.cc:102: context->channel_id_service()->GetUniqueID()) { While I'm not familiar with just how channel ID is hooked up, this whole thing seems very prone to bugs. Safebrowsing, for instance, does wire up its own ChannelIDService and CookieStore. But since it shares an HttpNetworkSession with the main profile, it's still presumably borked, and this code wouldn't catch it, since the URLRequestContext's CookieStore and ChannelIDService match, it's just the HttpNetworkSession's ChannelIDService that's wrong.
https://codereview.chromium.org/1818603002/diff/1/net/url_request/url_request... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1818603002/diff/1/net/url_request/url_request... net/url_request/url_request_http_job.cc:102: context->channel_id_service()->GetUniqueID()) { On 2016/03/18 21:50:45, mmenke wrote: > While I'm not familiar with just how channel ID is hooked up, this whole thing > seems very prone to bugs. Safebrowsing, for instance, does wire up its own > ChannelIDService and CookieStore. But since it shares an HttpNetworkSession > with the main profile, it's still presumably borked, and this code wouldn't > catch it, since the URLRequestContext's CookieStore and ChannelIDService match, > it's just the HttpNetworkSession's ChannelIDService that's wrong. Safe browsing wires up its own CookieStore, but doesn't touch the ChannelIDService (Ignoring my other CL). However, you correctly point out that I meant to replace the context->channel_id_service()->GetUniqueID() line, not the params->channel_id_service->GetUniqueID(). I.e. I should be checking that the id in the cookie store matches the one in the HttpNetworkSession::Params's channel_id_service, since I want to be comparing against the ChannelIDService actually in use.
On 2016/03/18 21:59:03, nharper wrote: > https://codereview.chromium.org/1818603002/diff/1/net/url_request/url_request... > File net/url_request/url_request_http_job.cc (right): > > https://codereview.chromium.org/1818603002/diff/1/net/url_request/url_request... > net/url_request/url_request_http_job.cc:102: > context->channel_id_service()->GetUniqueID()) { > On 2016/03/18 21:50:45, mmenke wrote: > > While I'm not familiar with just how channel ID is hooked up, this whole thing > > seems very prone to bugs. Safebrowsing, for instance, does wire up its own > > ChannelIDService and CookieStore. But since it shares an HttpNetworkSession > > with the main profile, it's still presumably borked, and this code wouldn't > > catch it, since the URLRequestContext's CookieStore and ChannelIDService > match, > > it's just the HttpNetworkSession's ChannelIDService that's wrong. > > Safe browsing wires up its own CookieStore, but doesn't touch the > ChannelIDService (Ignoring my other CL). However, you correctly point out that I > meant to replace the context->channel_id_service()->GetUniqueID() line, not the > params->channel_id_service->GetUniqueID(). > > I.e. I should be checking that the id in the cookie store matches the one in the > HttpNetworkSession::Params's channel_id_service, since I want to be comparing > against the ChannelIDService actually in use. Ahh, right...Just did a quick search for ChannelIDService in SafeBrowsing (Knew about the borkage already), and found it in a comment...didn't notice it was a comment, as opposed to setting the service on the context.
https://codereview.chromium.org/1818603002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1818603002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:518: main_cookie_store_->SetChannelIDServiceID(channel_id_service->GetUniqueID()); This seems weird. We're calling into one ChannelIDService to get unique IDs for other ChannelIDServices that are, otherwise, completely independent of the main ChannelIDService. Could we just have IOThread vend them directly or something? Open to other ideas. https://codereview.chromium.org/1818603002/diff/20001/net/cookies/cookie_stor... File net/cookies/cookie_store.cc (right): https://codereview.chromium.org/1818603002/diff/20001/net/cookies/cookie_stor... net/cookies/cookie_store.cc:63: void CookieStore::SetChannelIDServiceID(int id) { DCHECK_EQ(-1, channel_id_service_id_); ? https://codereview.chromium.org/1818603002/diff/20001/net/url_request/url_req... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1818603002/diff/20001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:351: storage->set_channel_id_service(std::move(channel_id_service)); Suggestion: Replace URLRequestContext::set_cookie_store and URLRequestContext::set_channel_id_service with a single method to set both. Have it dcheck that the channel store ID either matches that of the ChannelIDService, or is not yet set (And if not set, set it). Make a similar method in URLRequestContextStorage. Then have a separate set_cookie_store_borked method (Or whatever) that just takes a cookie store and sets has_known_mismatched_cookie_store_ to true.
https://codereview.chromium.org/1818603002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1818603002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:518: main_cookie_store_->SetChannelIDServiceID(channel_id_service->GetUniqueID()); On 2016/03/18 22:12:56, mmenke wrote: > This seems weird. We're calling into one ChannelIDService to get unique IDs for > other ChannelIDServices that are, otherwise, completely independent of the main > ChannelIDService. Could we just have IOThread vend them directly or something? > Open to other ideas. I don't follow what you're saying here. This is setting a field in the main cookie store to indicate that it should be used with the channel id service used in the main context, so that it can be checked later that the expected channel id service for the cookie store was indeed used. I don't see how this uses one ChannelIDService to get unique IDs for other ChannelIDServices. https://codereview.chromium.org/1818603002/diff/20001/net/cookies/cookie_stor... File net/cookies/cookie_store.cc (right): https://codereview.chromium.org/1818603002/diff/20001/net/cookies/cookie_stor... net/cookies/cookie_store.cc:63: void CookieStore::SetChannelIDServiceID(int id) { On 2016/03/18 22:12:56, mmenke wrote: > DCHECK_EQ(-1, channel_id_service_id_); ? Done. https://codereview.chromium.org/1818603002/diff/20001/net/url_request/url_req... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1818603002/diff/20001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:351: storage->set_channel_id_service(std::move(channel_id_service)); On 2016/03/18 22:12:56, mmenke wrote: > Suggestion: Replace URLRequestContext::set_cookie_store and > URLRequestContext::set_channel_id_service with a single method to set both. > > Have it dcheck that the channel store ID either matches that of the > ChannelIDService, or is not yet set (And if not set, set it). Make a similar > method in URLRequestContextStorage. > > Then have a separate set_cookie_store_borked method (Or whatever) that just > takes a cookie store and sets has_known_mismatched_cookie_store_ to true. The problem with this is that resetting the cookie store on an URLRequestContext will result in using the new cookie store, but resetting the channel id service will result in the URLRequestContext using whatever channel id service was in the HttpNetworkSession (assuming that's already been set). I tried calling set_channel_id_service in the safe browsing code, but because the HttpNetworkSession had already been constructed with the system channel id service, the call to URLRequestContext::set_channel_id_service didn't do anything useful.
https://codereview.chromium.org/1818603002/diff/20001/net/url_request/url_req... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1818603002/diff/20001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:351: storage->set_channel_id_service(std::move(channel_id_service)); On 2016/03/18 22:31:09, nharper wrote: > On 2016/03/18 22:12:56, mmenke wrote: > > Suggestion: Replace URLRequestContext::set_cookie_store and > > URLRequestContext::set_channel_id_service with a single method to set both. > > > > Have it dcheck that the channel store ID either matches that of the > > ChannelIDService, or is not yet set (And if not set, set it). Make a similar > > method in URLRequestContextStorage. > > > > Then have a separate set_cookie_store_borked method (Or whatever) that just > > takes a cookie store and sets has_known_mismatched_cookie_store_ to true. > > The problem with this is that resetting the cookie store on an URLRequestContext > will result in using the new cookie store, but resetting the channel id service > will result in the URLRequestContext using whatever channel id service was in > the HttpNetworkSession (assuming that's already been set). I tried calling > set_channel_id_service in the safe browsing code, but because the > HttpNetworkSession had already been constructed with the system channel id > service, the call to URLRequestContext::set_channel_id_service didn't do > anything useful. This is true...But this code has the exact same problem. The change I suggested makes sure that people who are setting just the cookie store at least run into the problem, though they may try and incorrect fix (You could also make the same DCHECK check the HttpNetworkSession, if we have one, and then you can catch everything, if you add a DCHECK to the method to set the HttpNetworkSession as well).
https://codereview.chromium.org/1818603002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1818603002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:518: main_cookie_store_->SetChannelIDServiceID(channel_id_service->GetUniqueID()); On 2016/03/18 22:31:09, nharper wrote: > On 2016/03/18 22:12:56, mmenke wrote: > > This seems weird. We're calling into one ChannelIDService to get unique IDs > for > > other ChannelIDServices that are, otherwise, completely independent of the > main > > ChannelIDService. Could we just have IOThread vend them directly or > something? > > Open to other ideas. > > I don't follow what you're saying here. This is setting a field in the main > cookie store to indicate that it should be used with the channel id service used > in the main context, so that it can be checked later that the expected channel > id service for the cookie store was indeed used. I don't see how this uses one > ChannelIDService to get unique IDs for other ChannelIDServices. Oh, "GetUniqueId()" was confusing me. Was thinking it was generating a unique ID, and you were assigning it to the channel ID service here.
https://codereview.chromium.org/1818603002/diff/20001/net/url_request/url_req... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1818603002/diff/20001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:351: storage->set_channel_id_service(std::move(channel_id_service)); Ok, I think I'm understanding how this could work. For the common case where a new URLRequestContext is being made, and set_cookie_store and set_channel_id_service are being called in sequence, change those to a new URLRequestContext::set_cookie_store_and_channel_id_service method, replacing the previous two. In the places where a cookie store is being changed, either the new set_cookie_store_and_channel_id_service method should be used (and the developer needs to figure out how to fix the HttpNetworkSession as well), or use an escape hatch URLRequestContext:set_cookie_store_borked. I don't think we can add DCHECKs to catch everything (i.e. checking that the CookieStore's channel id service matches the HttpNetworkTransaction's channel id service), because in the case where we're setting new objects, one of them has to be set first (or we need a variant of set_cookie_store_and_channel_id_service that also takes a HttpTransactionFactory).
https://codereview.chromium.org/1818603002/diff/20001/net/url_request/url_req... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1818603002/diff/20001/net/url_request/url_req... net/url_request/url_request_context_builder.cc:351: storage->set_channel_id_service(std::move(channel_id_service)); On 2016/03/18 23:09:01, nharper wrote: > Ok, I think I'm understanding how this could work. > > For the common case where a new URLRequestContext is being made, and > set_cookie_store and set_channel_id_service are being called in sequence, change > those to a new URLRequestContext::set_cookie_store_and_channel_id_service > method, replacing the previous two. > > In the places where a cookie store is being changed, either the new > set_cookie_store_and_channel_id_service method should be used (and the developer > needs to figure out how to fix the HttpNetworkSession as well), or use an escape > hatch URLRequestContext:set_cookie_store_borked. > > I don't think we can add DCHECKs to catch everything (i.e. checking that the > CookieStore's channel id service matches the HttpNetworkTransaction's channel id > service), because in the case where we're setting new objects, one of them has > to be set first (or we need a variant of set_cookie_store_and_channel_id_service > that also takes a HttpTransactionFactory). We could, if we required the HttpNetworkSession be cleared and then set...but yea, that's rather ugly.
I don't think an URLRequestContext::set_cookie_store_and_channel_id_service method is going to happen in this CL. It would be better as a piece of cleanup after this and https://codereview.chromium.org/1814543002/.
https://codereview.chromium.org/1818603002/diff/40001/net/cookies/cookie_store.h File net/cookies/cookie_store.h (right): https://codereview.chromium.org/1818603002/diff/40001/net/cookies/cookie_stor... net/cookies/cookie_store.h:210: int GetChannelIDServiceID(); Do you plan to keep this, long term? Why make the cookie store know about the ChannelIDService, and not the other way around? Not sure which is the right dependency order, just wondering if you thought about it. https://codereview.chromium.org/1818603002/diff/40001/net/url_request/url_req... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1818603002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job.cc:68: void LogChannelIDAndCookieStores(const net::URLRequestContext* context, This function really needs a comment., or the histogram fields do. https://codereview.chromium.org/1818603002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job.cc:91: net::CookieStore* cookie_store = context->cookie_store(); We allow NULL cookie stores. https://codereview.chromium.org/1818603002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job.cc:104: ephemerality = EPHEMERAL_MISMATCH; If this happens, it is a bug. Should we really be logging bugs in histograms instead of, say, DCHECKing when this happens? If we can't DCHECK on URLRequestContext initialization (Though I'd really prefer if we figure out a way to do that), maybe DCHECK in URLRequestContext::CreateRequest? I'm fine with doing both, but I really don't think we want to keep a histogram about whether a URLRequestContext was created correctly, long term it's just weird, and puts the burden of ensuring proper URLRequestContext creation on the wrong team.
And sorry for slowness last week - hope to be very responsive on these two CLs this week, so we can get these out the door.
https://codereview.chromium.org/1818603002/diff/40001/net/cookies/cookie_store.h File net/cookies/cookie_store.h (right): https://codereview.chromium.org/1818603002/diff/40001/net/cookies/cookie_stor... net/cookies/cookie_store.h:210: int GetChannelIDServiceID(); On 2016/03/28 17:54:00, mmenke wrote: > Do you plan to keep this, long term? Why make the cookie store know about the > ChannelIDService, and not the other way around? Not sure which is the right > dependency order, just wondering if you thought about it. I'd like all of this debugging stuff to go away eventually (although that would be problematic if we want to DCHECK when a mismatch happens). The reason for having the cookie store know about the ChannelIDService is because ChannelIDService already has an atomic sequence number for generating these IDs - I believe I'd need to add something similar to CookieStore to have this work the other way. I didn't think very hard about the dependency order when I first did this, but I think I'm convinced that this is the correct order. This set up allows for multiple CookieStores to belong to the same ChannelIDService, which could be fine (or could be problematic if deleting data from a CookieStore triggers deletions from the ChannelIDService). If this is reversed, then a single CookieStore could end up belonging to multiple ChannelIDServices, which would always be wrong. The most robust way to ensure a 1-to-1 mapping would be to have both know who the other is. https://codereview.chromium.org/1818603002/diff/40001/net/url_request/url_req... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1818603002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job.cc:68: void LogChannelIDAndCookieStores(const net::URLRequestContext* context, On 2016/03/28 17:54:00, mmenke wrote: > This function really needs a comment., or the histogram fields do. Done. Comments added for both. https://codereview.chromium.org/1818603002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job.cc:91: net::CookieStore* cookie_store = context->cookie_store(); On 2016/03/28 17:54:00, mmenke wrote: > We allow NULL cookie stores. I check for a null cookie store a few lines down. https://codereview.chromium.org/1818603002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job.cc:104: ephemerality = EPHEMERAL_MISMATCH; On 2016/03/28 17:54:00, mmenke wrote: > If this happens, it is a bug. Should we really be logging bugs in histograms > instead of, say, DCHECKing when this happens? If we can't DCHECK on > URLRequestContext initialization (Though I'd really prefer if we figure out a > way to do that), maybe DCHECK in URLRequestContext::CreateRequest? I don't see any reason why we can't DCHECK. > > I'm fine with doing both, but I really don't think we want to keep a histogram > about whether a URLRequestContext was created correctly, long term it's just > weird, and puts the burden of ensuring proper URLRequestContext creation on the > wrong team. I'm assuming that DCHECKs don't fire on canary/dev builds, so I've added them to the 4 cases that should never happen. I think doing both would be a good short-term step, but I think long-term this histogram should go away. (Ideally this whole method would go, but then there's no place to leave the DCHECKs.)
LGTM https://codereview.chromium.org/1818603002/diff/80001/net/url_request/url_req... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1818603002/diff/80001/net/url_request/url_req... net/url_request/url_request_http_job.cc:133: DCHECK(false); nit: These should all be NOTREACHED()
The CQ bit was checked by nharper@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1818603002/#ps100001 (title: "s/DCHECK(false)/NOTREACHED()/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818603002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818603002/100001
Message was sent while issue was closed.
Description was changed from ========== Keep track of ChannelIDService in CookieStore This is what I meant to do in https://codereview.chromium.org/1770983002/ BUG=548423 ========== to ========== Keep track of ChannelIDService in CookieStore This is what I meant to do in https://codereview.chromium.org/1770983002/ BUG=548423 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Keep track of ChannelIDService in CookieStore This is what I meant to do in https://codereview.chromium.org/1770983002/ BUG=548423 ========== to ========== Keep track of ChannelIDService in CookieStore This is what I meant to do in https://codereview.chromium.org/1770983002/ BUG=548423 Committed: https://crrev.com/3876dd56009d12b12d1ac62acb6397598a893dca Cr-Commit-Position: refs/heads/master@{#383853} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3876dd56009d12b12d1ac62acb6397598a893dca Cr-Commit-Position: refs/heads/master@{#383853} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
