Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
767163005:
Add AffiliationFetcher to fetch authoritative affiliation information regarding facets. (Closed)
DescriptionAdd AffiliationFetcher to fetch authoritative affiliation information regarding facets.
This uses the utilities introduced in https://codereview.chromium.org/771173002/.
BUG=437865
Committed: https://crrev.com/7411df512d4b3691b4753e65622fc46a50d50b07
Cr-Commit-Position: refs/heads/master@{#308354}
Patch Set 1 #
Total comments: 42
Patch Set 2 : Factored out utility methods into a separate CL. #Patch Set 3 : Addressed comments. #Patch Set 4 : Addressed comments. #Patch Set 5 : Minor improvements to phrasing. #
Total comments: 24
Patch Set 6 : Addressed comments from rsleevi@, rdsmith@, and gcasto@. #
Total comments: 12
Patch Set 7 : Addressed nits from gcasto@ and rdsmith@. #
Total comments: 6
Patch Set 8 : Address final set of comments from rsleevi@. #Patch Set 9 : Address final set of comments from rsleevi@. #Patch Set 10 : Minor tweaks to comments. #Patch Set 11 : Rebase on ToT, fix GN format. #Patch Set 12 : Fix GN build. #Messages
Total messages: 30 (8 generated)
engedy@chromium.org changed reviewers: + mkwst@chromium.org
Mike, please take a look. This is the first of a series of CLs. Apologies for not being able to separate this further into even smaller pieces.
https://codereview.chromium.org/767163005/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:36: const char kResponseEquivalenceClassMembersKey[] = "facet"; Hrm. You're only using each of these keys once. While I agree that the descriptive name is significantly better than a bare string, it's not clear that these need to be extracted into an anonymous namespace. It's also unclear why some string constants are extracted here, and others aren't. For instance, "key" in BuildQueryURL(). Do you think you'll use them elsewhere in the future? If so, perhaps they could be public constants that other classes could use? If not, it might make sense to bring them back down into local scopes. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:51: Nit: Whitespace. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:52: for (const auto& uri : requested_facet_uris_) { Consider dropping "auto" here; the actual type isn't terribly complex. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:54: } Nit: No {} for a one-line body. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:80: NOTREACHED(); Perhaps just `DCHECK(!fetcher_)`? https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:90: net::LOAD_DO_NOT_SEND_AUTH_DATA | Probably should add net::LOAD_DO_NOT_PROMPT_FOR_LOGIN. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:91: net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE); Why both bypass and disable? Disabling should be enough, shouldn't it? https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:104: for (const auto& uri : requested_facet_uris_) { Consider just using the type rather than auto. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:106: } Nit: No {}. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:113: if (!base::JSONWriter::Write(&payload_dictionary, &payload_json)) { Consider using OPTIONS_PRETTY_PRINT while in Debug mode (or having some other method to inject the preference). That might make things simpler for you to visually inspect while you're working through the implementation details. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:114: // This should never really happen. DCHECK? https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:126: (base::JSONReader::Read(response_json))); Nit: Extra (). https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:137: return false; You might find it helpful to have a bit more error reporting here. It's going to be tough to debug as-is. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliation_fetcher.h (right): https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.h:32: // However, it should always be accessed on the thread it was created on. "should" or "must"? It looks like you're only using the thread checker in 'StartRequest()'. If this is a "must", then perhaps you could consider asserting thread safety in the accessor methods as well? https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.h:58: // * Results are guaranteed to be always fresh and will never be cached. Is it easily possible to add tests for these assertions to lock them in place? https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.h:101: class AffiliationFetcherDelegate { Nit: Why bundle this into the same file as the fetcher? https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.h:123: class AffiliationFetcherFactory { Nit: Ditto.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/767163005/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:36: const char kResponseEquivalenceClassMembersKey[] = "facet"; On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Hrm. You're only using each of these keys once. While I agree that the > descriptive name is significantly better than a bare string, it's not clear that > these need to be extracted into an anonymous namespace. It's also unclear why > some string constants are extracted here, and others aren't. For instance, "key" > in BuildQueryURL(). > > Do you think you'll use them elsewhere in the future? If so, perhaps they could > be public constants that other classes could use? If not, it might make sense to > bring them back down into local scopes. They shouldn't any other classes that need this, so I just moved them to local scopes. In fact, I have removed the constant definitions altogether, and added an sample request/response instead. WDYT? Happy to bring back the constants if you think that is better. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:51: On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Nit: Whitespace. Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:52: for (const auto& uri : requested_facet_uris_) { On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Consider dropping "auto" here; the actual type isn't terribly complex. Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:54: } On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Nit: No {} for a one-line body. Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:80: NOTREACHED(); On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Perhaps just `DCHECK(!fetcher_)`? Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:90: net::LOAD_DO_NOT_SEND_AUTH_DATA | On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Probably should add net::LOAD_DO_NOT_PROMPT_FOR_LOGIN. Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:91: net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE); On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Why both bypass and disable? Disabling should be enough, shouldn't it? I think both are needed. DISABLE alone would not be enough as only BYPASS will add "Pragma: no-cache" and "Cache-Control: no-cache" headers to the request: otherwise a proxy may decide to send back a cached, and possibly out-of-date, response. See: https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... BYPASS alone would not be enough as only DISABLE will prevent writing the response to Chrome's cache (which would only serve to waste space). See: https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_cach... https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_cach... https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_cach... The documentation in load_flags_list.h is a bit vague, but seems to be in agreement with this, and there are also a few other call sites specifying both flags. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:104: for (const auto& uri : requested_facet_uris_) { On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Consider just using the type rather than auto. Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:106: } On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Nit: No {}. Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:113: if (!base::JSONWriter::Write(&payload_dictionary, &payload_json)) { On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Consider using OPTIONS_PRETTY_PRINT while in Debug mode (or having some other > method to inject the preference). That might make things simpler for you to > visually inspect while you're working through the implementation details. For now, I would not complicate the code with this, as we mostly build rather straightforward JSON outputs here. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:114: // This should never really happen. On 2014/12/02 14:32:42, Mike West (OOO) wrote: > DCHECK? Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:126: (base::JSONReader::Read(response_json))); On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Nit: Extra (). Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:137: return false; On 2014/12/02 14:32:42, Mike West (OOO) wrote: > You might find it helpful to have a bit more error reporting here. It's going to > be tough to debug as-is. Hmm, what kind of scenario do you have in mind? The server-side suddenly sending bad responses, or somebody updating this part of the code? That is, should we add VLOGs and/or UMA histograms? https://codereview.chromium.org/767163005/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliation_fetcher.h (right): https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.h:32: // However, it should always be accessed on the thread it was created on. On 2014/12/02 14:32:42, Mike West (OOO) wrote: > "should" or "must"? It looks like you're only using the thread checker in > 'StartRequest()'. If this is a "must", then perhaps you could consider asserting > thread safety in the accessor methods as well? Actually, I have just removed the ThreadChecker. Unless the caller is crazy, I do not foresee any use-cases where there would be a significant risk of misuse that such a check could prevent -- overchecking this might not have much merit. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.h:58: // * Results are guaranteed to be always fresh and will never be cached. On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Is it easily possible to add tests for these assertions to lock them in place? We could verify in the unit test that the load flags are set on the TestURLFetcher, but as far as I understand, that would be a change-detector test, which are considered harmful. Let me know if you disagree. (Another solution would be to add a browsertest in Chrome code to check the state of the cache/cookie jar before and after. That would be a huge amount of work, and it would mostly test URLFetcher, not our class.) https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.h:101: class AffiliationFetcherDelegate { On 2014/12/02 14:32:43, Mike West (OOO) wrote: > Nit: Why bundle this into the same file as the fetcher? I just didn't want to frighten potential code reviewers with a large number of files. :) Factored it out. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.h:123: class AffiliationFetcherFactory { On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Nit: Ditto. Actually, I will remove this for now, and will add it back with the testing helpers later.
LGTM % nits, thanks for going another round. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:137: return false; On 2014/12/02 18:33:55, engedy wrote: > On 2014/12/02 14:32:42, Mike West (OOO) wrote: > Hmm, what kind of scenario do you have in mind? The server-side suddenly sending > bad responses, or somebody updating this part of the code? That is, should we > add VLOGs and/or UMA histograms? I was thinking about VLOGs, as I imagine you'll be debugging the interactions manually for at least a little while. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:154: return false; Nit: You need {} here, as the conditional is multi-line. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:181: return false; Nit: {}. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:184: if (facet_uri_to_class_index[affiliated_uris[0]] == result->size()) Isn't this always true if the conditions on 179-180 are met?
https://codereview.chromium.org/767163005/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:137: return false; On 2014/12/03 10:44:43, Mike West wrote: > On 2014/12/02 18:33:55, engedy wrote: > > On 2014/12/02 14:32:42, Mike West (OOO) wrote: > > Hmm, what kind of scenario do you have in mind? The server-side suddenly > sending > > bad responses, or somebody updating this part of the code? That is, should we > > add VLOGs and/or UMA histograms? > > I was thinking about VLOGs, as I imagine you'll be debugging the interactions > manually for at least a little while. Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:154: return false; On 2014/12/03 10:44:43, Mike West wrote: > Nit: You need {} here, as the conditional is multi-line. Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:181: return false; On 2014/12/03 10:44:43, Mike West wrote: > Nit: {}. Done. https://codereview.chromium.org/767163005/diff/1/components/password_manager/... components/password_manager/core/browser/affiliation_fetcher.cc:184: if (facet_uri_to_class_index[affiliated_uris[0]] == result->size()) On 2014/12/03 10:44:43, Mike West wrote: > Isn't this always true if the conditions on 179-180 are met? I have added a comment to clarify.
Please change the CL description to point to the previous patch (and to drop the "utilities" bit)
engedy@chromium.org changed reviewers: + rsleevi@chromium.org
@Ryan, could you please sign off on our adding the following dependencies: net/http/http_status_code.h net/url_request
rsleevi@chromium.org changed reviewers: + rdsmith@chromium.org
I've left some comments. Overall, my understanding of your use of URLFetcher is safe, although I've left nits on some comments and highlighted one area where you're in a SECURITY DANGER ZONE. It can go either way, since you're talking explicitly to Google servers, but I'd rather we opted for the secure solution than making it possible that people follow the pattern. I've also added rdsmith, as my knowledge of URLFetcher is somewhat limited and I want to make sure I didn't miss any subtle, sharp edges. https://codereview.chromium.org/767163005/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:53: net::LOAD_DISABLE_CACHE | net::LOAD_DO_NOT_PROMPT_FOR_LOGIN); Why LOAD_DISABLE_CACHE? Seems emminently sensible that this could potentially be cached for some duration, at the HTTP layer, negating some of the application-level complexity. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:110: base::JSONReader::Read(response_json)); SECURITY: Parsing JSON inside the Browser process, received from remote endpoints, is DANGEROUS and NOT ENCOURAGED. We have a utility in //chrome to defer to the browser process. Alternatively, using protobufs is blessed for security. Put differently, our base::JSONReader has not at all been fuzzed / vetted to take arbitrary data. I realize this is data "from us", but it's in more of the grey area here, so it's easier to avoid the potential red flags. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:113: VLOG(1) << response_json; These are fairly chatty VLOGs. I want to make sure that the benefit is justified; it's easy to miss the hidden costs of logging and suffer from death by a thousand strings. Does a DVLOG make more sense here? Or alternatively omitting these until they're needed for debugging? I'm not a fan of printf style debugging in production code, unless there's demonstrated need. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:134: // as it may be missing classes artificially added by this function later. This is a pretty lengthy comment for what is implicit in the code. Further, the "costly reallocation" isn't backed up with data, and so it's a comment that can mislead. Costly where, for who, why, etc. It's easier to just let the idiom of "reserve what you need" apply, as a skilled C++ developer should instantly recognize the pattern. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:179: // response cannot be part of an equivalence relation. Please consider rewriting this without the pronoun (see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... for discussion) // However, if a partial overlapping is discovered, terminate early, as the response cannot be part of an equivalence relation. ^-- or whatever it means (as someone who has read your design doc, I'm still lost on your terminology, but I'll assume that's my own ignorance than the comment) https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:197: // anything, or facet URIs that it does not know about. However, this class This first comment reads weird, in particular, the "does not return at all facet URIs" Is it meant to be "does not return any facet URIs"? is the "at" a typo and it should be "does not return all" ? https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:199: // one for each each of these missing facets. // The API contract of this class is to return an equivalence class for all requested facets; however, the // server does not return any facets that are either not affiliated with anything or which it does not // know about. In this case, synthesize an equivalence class for all remaining facets. I'm still not sure what the distinction is between "facet URIs that are not affiliated with anything" and "facet URIs that it does not know about", but I assume the distinction is clear for you.
LGTM with below changes. https://codereview.chromium.org/767163005/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:215: if (fetcher_->GetResponseCode() == net::HTTP_OK) { I'm a little uncomfortable with testing only this, and not testing fetcher_->GetStatus(). It may well be that for all cases in which fetcher_->GetResponseCode() == HTTP_OK, GetStatus() will be { Success, OK}, but it seems unwise to count on it--for instance, if we receive http headers and partial data and then the connection gets reset, maybe URLFetcher will return the partial data and a failure in GetStatus(). My guess is it doesn't, but based on the header comments it might, so defensive programming seems the right strategy. https://codereview.chromium.org/767163005/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.h (right): https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.h:28: // An instance is good for exactly one fetch, and may be used from any thread, Strictly speaking, not any thread; the URLFetcher can only be used from non-worker pools because there needs to be a "real thread" to post notifications back to. See https://code.google.com/p/chromium/issues/detail?id=432307#c5.
gcasto@chromium.org changed reviewers: + gcasto@chromium.org
Drive by. https://codereview.chromium.org/767163005/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:110: base::JSONReader::Read(response_json)); On 2014/12/10 19:44:36, Ryan Sleevi wrote: > SECURITY: Parsing JSON inside the Browser process, received from remote > endpoints, is DANGEROUS and NOT ENCOURAGED. > > We have a utility in //chrome to defer to the browser process. > > Alternatively, using protobufs is blessed for security. > > Put differently, our base::JSONReader has not at all been fuzzed / vetted to > take arbitrary data. I realize this is data "from us", but it's in more of the > grey area here, so it's easier to avoid the potential red flags. Protocol buffers also look like they would save you about 30 lines worth of parsing code, which would be nice. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:113: VLOG(1) << response_json; On 2014/12/10 19:44:36, Ryan Sleevi wrote: > These are fairly chatty VLOGs. I want to make sure that the benefit is > justified; it's easy to miss the hidden costs of logging and suffer from death > by a thousand strings. > > Does a DVLOG make more sense here? Or alternatively omitting these until they're > needed for debugging? I'm not a fan of printf style debugging in production > code, unless there's demonstrated need. +1 to DVLOG. If you're actually worried about these error conditions, adding UMA stats are a better way to keep track of this. VLOG only really seems useful if you have an external reporter that is experiencing a problem that you can't reproduce. https://codereview.chromium.org/767163005/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.h (right): https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.h:37: static AffiliationFetcher* Create( Out of curiosity, any particular reason for the factory pattern here?
@Ryan, @Randy, @Garrett: Thank you all for the thorough review! Please take another look. https://codereview.chromium.org/767163005/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:53: net::LOAD_DISABLE_CACHE | net::LOAD_DO_NOT_PROMPT_FOR_LOGIN); On 2014/12/10 19:44:36, Ryan Sleevi wrote: > Why LOAD_DISABLE_CACHE? Seems emminently sensible that this could potentially be > cached for some duration, at the HTTP layer, negating some of the > application-level complexity. Most of the time the lookups will be performed in the reverse direction. For an example of what this means, assume that the user has a password stored for Android app A, where A is affiliated with website B, and so the password from A should be offered to be filled into B. Obviously, as the user visits websites W_1, W_2, ..., W_n, Chrome should not issue a request to look up affiliates for each website W_i, just because of the odd chance of encountering W_i = B at one point. Instead, it will query the affiliates of A and preload them into a "smarter" cache that can be later queried in the reverse direction, i.e., yielding A when queried for W_i = B. As this smart cache will be needed anyway, I do not see a need to do caching at the HTTP layer too. In fact, I think caching at the HTTP layer is somewhat harmful, as it makes the freshness level of the data less clear, and we want to offer relatively strict guarantees regarding how soon Chrome will forget that A and B are affiliated with each other once the relation is removed from the server side database. Please let me know if I have misunderstood something, and HTTP caching would be, in fact, useful in this case. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:110: base::JSONReader::Read(response_json)); On 2014/12/11 08:18:39, Garrett Casto wrote: > On 2014/12/10 19:44:36, Ryan Sleevi wrote: > > SECURITY: Parsing JSON inside the Browser process, received from remote > > endpoints, is DANGEROUS and NOT ENCOURAGED. > > > > We have a utility in //chrome to defer to the browser process. > > > > Alternatively, using protobufs is blessed for security. > > > > Put differently, our base::JSONReader has not at all been fuzzed / vetted to > > take arbitrary data. I realize this is data "from us", but it's in more of the > > grey area here, so it's easier to avoid the potential red flags. > > Protocol buffers also look like they would save you about 30 lines worth of > parsing code, which would be nice. Thanks for the heads-up! The server side support protobufs as well, so... Protobuf it is. Done. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:113: VLOG(1) << response_json; On 2014/12/11 08:18:38, Garrett Casto wrote: > On 2014/12/10 19:44:36, Ryan Sleevi wrote: > > These are fairly chatty VLOGs. I want to make sure that the benefit is > > justified; it's easy to miss the hidden costs of logging and suffer from death > > by a thousand strings. > > > > Does a DVLOG make more sense here? Or alternatively omitting these until > they're > > needed for debugging? I'm not a fan of printf style debugging in production > > code, unless there's demonstrated need. > > +1 to DVLOG. If you're actually worried about these error conditions, adding UMA > stats are a better way to keep track of this. VLOG only really seems useful if > you have an external reporter that is experiencing a problem that you can't > reproduce. Removed logging statements -- Given that the Fetcher is using protobufs now, there is no more need, really. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:134: // as it may be missing classes artificially added by this function later. On 2014/12/10 19:44:36, Ryan Sleevi wrote: > This is a pretty lengthy comment for what is implicit in the code. > > Further, the "costly reallocation" isn't backed up with data, and so it's a > comment that can mislead. Costly where, for who, why, etc. > > It's easier to just let the idiom of "reserve what you need" apply, as a skilled > C++ developer should instantly recognize the pattern. Done (removed comment). https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:179: // response cannot be part of an equivalence relation. On 2014/12/10 19:44:36, Ryan Sleevi wrote: > Please consider rewriting this without the pronoun (see > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > for discussion) > > // However, if a partial overlapping is discovered, terminate early, as the > response cannot be part of an equivalence relation. > > ^-- or whatever it means (as someone who has read your design doc, I'm still > lost on your terminology, but I'll assume that's my own ignorance than the > comment) I have rephrased it, hopefully, for the better. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:197: // anything, or facet URIs that it does not know about. However, this class On 2014/12/10 19:44:36, Ryan Sleevi wrote: > This first comment reads weird, in particular, the "does not return at all facet > URIs" > > Is it meant to be "does not return any facet URIs"? is the "at" a typo and it > should be "does not return all" ? That was a typo, sorry. I have used you comment suggestion below. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:199: // one for each each of these missing facets. On 2014/12/10 19:44:36, Ryan Sleevi wrote: > // The API contract of this class is to return an equivalence class for all > requested facets; however, the > // server does not return any facets that are either not affiliated with > anything or which it does not > // know about. In this case, synthesize an equivalence class for all remaining > facets. > > I'm still not sure what the distinction is between "facet URIs that are not > affiliated with anything" and "facet URIs that it does not know about", but I > assume the distinction is clear for you. Thanks for the suggestion! I have rephrased along these lines. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:215: if (fetcher_->GetResponseCode() == net::HTTP_OK) { On 2014/12/10 22:13:24, rdsmith wrote: > I'm a little uncomfortable with testing only this, and not testing > fetcher_->GetStatus(). It may well be that for all cases in which > fetcher_->GetResponseCode() == HTTP_OK, GetStatus() will be { Success, OK}, but > it seems unwise to count on it--for instance, if we receive http headers and > partial data and then the connection gets reset, maybe URLFetcher will return > the partial data and a failure in GetStatus(). My guess is it doesn't, but > based on the header comments it might, so defensive programming seems the right > strategy. Done. Thanks for pointing that out! https://codereview.chromium.org/767163005/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.h (right): https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.h:28: // An instance is good for exactly one fetch, and may be used from any thread, On 2014/12/10 22:13:24, rdsmith wrote: > Strictly speaking, not any thread; the URLFetcher can only be used from > non-worker pools because there needs to be a "real thread" to post notifications > back to. See https://code.google.com/p/chromium/issues/detail?id=432307#c5. Rephrased, but I did not want to be overly verbose. Let me know if I you'd rather I added more detail to the comment. https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.h:37: static AffiliationFetcher* Create( On 2014/12/11 08:18:39, Garrett Casto wrote: > Out of curiosity, any particular reason for the factory pattern here? I am going to add a way to insert a custom factory for testing in the next CL. I wanted to avoid the repetitive task of refactoring all the test just for the sake of being able to remove the factory method.
https://codereview.chromium.org/767163005/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.h (right): https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.h:28: // An instance is good for exactly one fetch, and may be used from any thread, On 2014/12/11 20:49:24, engedy wrote: > On 2014/12/10 22:13:24, rdsmith wrote: > > Strictly speaking, not any thread; the URLFetcher can only be used from > > non-worker pools because there needs to be a "real thread" to post > notifications > > back to. See https://code.google.com/p/chromium/issues/detail?id=432307#c5. > > Rephrased, but I did not want to be overly verbose. Let me know if I you'd > rather I added more detail to the comment. I'd prefer you just said "and may be used from any thread that runs a message loop (i.e. not a worker pool thread)." "Not thread-safe" doesn't really have a lot of information in it; very little chromium code is actually thread safe.
lgtm https://codereview.chromium.org/767163005/diff/120001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:72: bool success = lookup_request.SerializeToString(&serialized_request); nit: You can just DCHECK() this line directly. https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:132: result->back().push_back(uri); Nit: You should be able to do this all in one line. result->push_back(AffiliatedFacets(1, uri)); https://codereview.chromium.org/767163005/diff/120001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher_unittest.cc (right): https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher_unittest.cc:32: void OnFetchSucceeded(scoped_ptr<Result> result) override { Are you doing this just to save |result|? Is there a reason testing::SaveArg doesn't work? https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher_unittest.cc:276: ASSERT_TRUE(testing::Mock::VerifyAndClearExpectations(&mock_delegate)); Nit: Unnecessary. https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher_unittest.cc:301: ASSERT_TRUE(testing::Mock::VerifyAndClearExpectations(&mock_delegate)); Nit: Unnecessary.
https://codereview.chromium.org/767163005/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.h (right): https://codereview.chromium.org/767163005/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.h:28: // An instance is good for exactly one fetch, and may be used from any thread, On 2014/12/11 20:54:35, rdsmith wrote: > On 2014/12/11 20:49:24, engedy wrote: > > On 2014/12/10 22:13:24, rdsmith wrote: > > > Strictly speaking, not any thread; the URLFetcher can only be used from > > > non-worker pools because there needs to be a "real thread" to post > > notifications > > > back to. See https://code.google.com/p/chromium/issues/detail?id=432307#c5. > > > > > Rephrased, but I did not want to be overly verbose. Let me know if I you'd > > rather I added more detail to the comment. > > I'd prefer you just said "and may be used from any thread that runs a message > loop (i.e. not a worker pool thread)." "Not thread-safe" doesn't really have a > lot of information in it; very little chromium code is actually thread safe. Sounds good, thanks for the proposal. Done. https://codereview.chromium.org/767163005/diff/120001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:72: bool success = lookup_request.SerializeToString(&serialized_request); On 2014/12/12 07:53:02, Garrett Casto wrote: > nit: You can just DCHECK() this line directly. Well, I guess I could, but I would really like the serialization of the request data to be executed in release mode too. ;-) Would you rather I used NOTREACHED()? https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:132: result->back().push_back(uri); On 2014/12/12 07:53:02, Garrett Casto wrote: > Nit: You should be able to do this all in one line. > > result->push_back(AffiliatedFacets(1, uri)); Done. https://codereview.chromium.org/767163005/diff/120001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher_unittest.cc (right): https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher_unittest.cc:32: void OnFetchSucceeded(scoped_ptr<Result> result) override { On 2014/12/12 07:53:02, Garrett Casto wrote: > Are you doing this just to save |result|? Is there a reason testing::SaveArg > doesn't work? Unfortunately, testing::SaveArg will mindlessly try to copy out the argument, which will fail with moveable-only types; and fixing this (in the foreseeable future) in not planned by the maintainers. I believe this is the standard workaround. https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher_unittest.cc:276: ASSERT_TRUE(testing::Mock::VerifyAndClearExpectations(&mock_delegate)); On 2014/12/12 07:53:02, Garrett Casto wrote: > Nit: Unnecessary. Done. https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher_unittest.cc:301: ASSERT_TRUE(testing::Mock::VerifyAndClearExpectations(&mock_delegate)); On 2014/12/12 07:53:02, Garrett Casto wrote: > Nit: Unnecessary. Done.
lgtm https://codereview.chromium.org/767163005/diff/120001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:72: bool success = lookup_request.SerializeToString(&serialized_request); On 2014/12/12 14:06:13, engedy wrote: > On 2014/12/12 07:53:02, Garrett Casto wrote: > > nit: You can just DCHECK() this line directly. > > Well, I guess I could, but I would really like the serialization of the request > data to be executed in release mode too. ;-) > > Would you rather I used NOTREACHED()? Ahh, yeah. Never mind. https://codereview.chromium.org/767163005/diff/120001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher_unittest.cc (right): https://codereview.chromium.org/767163005/diff/120001/components/password_man... components/password_manager/core/browser/affiliation_fetcher_unittest.cc:32: void OnFetchSucceeded(scoped_ptr<Result> result) override { On 2014/12/12 14:06:13, engedy wrote: > On 2014/12/12 07:53:02, Garrett Casto wrote: > > Are you doing this just to save |result|? Is there a reason testing::SaveArg > > doesn't work? > > Unfortunately, testing::SaveArg will mindlessly try to copy out the argument, > which will fail with moveable-only types; and fixing this (in the foreseeable > future) in not planned by the maintainers. I believe this is the standard > workaround. Got it. I vaguely remember having to work around this as well, but I couldn't remember the specifics.
LGTM % nits https://codereview.chromium.org/767163005/diff/140001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:30: DCHECK(uri.is_valid()); BUG/DANGER: Whenever using debugging macros and conditionals, you should ensure the conditional has appropriate braces around it, in the event the macros is optimized out. While DCHECK is a "safe" macro for this construct, it encourages safe coding around potentially dangerous idioms, and isolates you from any future changes to DCHECK. https://codereview.chromium.org/767163005/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:77: bool AffiliationFetcher::ParseResponse( DOCUMENTATION: I think it'd be helpful for future readers/maintainers of this code to provide a bit of a design-level comment in this file about the transformation/sanitization that you're doing here (similar to what you mentioned in the code review). Just clarifying the facet cleanups in more detail (that is, your comments on 98, 104, and 110 are all good, but perhaps a high level view of inputs and outputs and why, and then leave the code to show & comment the how) https://codereview.chromium.org/767163005/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:96: const std::string uri_spec(equivalence_class.facet(j)); Should this be "const std::string&" ? I didn't dig into the protobuf generator, but given line 91, I suspect it does return a const-ref? If not, no worries.
https://codereview.chromium.org/767163005/diff/140001/components/password_man... File components/password_manager/core/browser/affiliation_fetcher.cc (right): https://codereview.chromium.org/767163005/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:30: DCHECK(uri.is_valid()); On 2014/12/12 23:02:19, Ryan Sleevi wrote: > BUG/DANGER: Whenever using debugging macros and conditionals, you should ensure > the conditional has appropriate braces around it, in the event the macros is > optimized out. > > While DCHECK is a "safe" macro for this construct, it encourages safe coding > around potentially dangerous idioms, and isolates you from any future changes to > DCHECK. Done. https://codereview.chromium.org/767163005/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:77: bool AffiliationFetcher::ParseResponse( On 2014/12/12 23:02:19, Ryan Sleevi wrote: > DOCUMENTATION: I think it'd be helpful for future readers/maintainers of this > code to provide a bit of a design-level comment in this file about the > transformation/sanitization that you're doing here (similar to what you > mentioned in the code review). > > Just clarifying the facet cleanups in more detail (that is, your comments on 98, > 104, and 110 are all good, but perhaps a high level view of inputs and outputs > and why, and then leave the code to show & comment the how) Done. https://codereview.chromium.org/767163005/diff/140001/components/password_man... components/password_manager/core/browser/affiliation_fetcher.cc:96: const std::string uri_spec(equivalence_class.facet(j)); On 2014/12/12 23:02:19, Ryan Sleevi wrote: > Should this be "const std::string&" ? > > I didn't dig into the protobuf generator, but given line 91, I suspect it does > return a const-ref? If not, no worries. Argh, of course. Done.
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/767163005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/767163005/240001
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/7411df512d4b3691b4753e65622fc46a50d50b07 Cr-Commit-Position: refs/heads/master@{#308354} |
