Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15897003:
Added UMA measurement for how often we see control characters during cookie parsing. (Closed)
|
Created:
7 years, 7 months ago by jww Modified:
7 years, 6 months ago CC:
chromium-reviews, erikwright (departed), cbentzel+watch_chromium.org, jschuh Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded UMA measurement for how often we see control characters during cookie parsing.
BUG=238041
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202721
Patch Set 1 #Patch Set 2 : Fixed missing brackets that were causing unit test failure. #
Total comments: 15
Patch Set 3 : Erik changes as well as additional measurement of invalid tokens during cookie line parsing. #
Total comments: 2
Patch Set 4 : Added comment to file explaining UMA collection and fixed erikwright nit. #
Total comments: 2
Patch Set 5 : Added TODO to comment regarding UMA stats collection #Messages
Total messages: 12 (0 generated)
After talking with palmer@ and jschuh@, we converged on this patchset as the UMA for data we'd like to collect. The idea is that we'll measure how often we'd reach a parsing pass that would remove a cookie because of control characters, and UMA would take care of measuring this on a per user per load basis.
What's the motivation for measuring the manual name/value/attribute changes that are already being rejected? https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.cc File net/cookies/parsed_cookie.cc (left): https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Was this removed intentionally? https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.cc File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:134: UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieValue", false); This will be called for changing the value but not changing the name via the setters. Consider moving it to SetName and SetValue instead. https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:147: UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieAttributeValue", false); These metrics will be clouded because they cover both changes to attributes via the accessors and the parsing of a cookie-line (both value and attributes) during the constructor. Consider moving them to SetAttributePair instead. https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:396: if (!IsValidCookieAttributeValue(value)) Why not just put this after the assignment to pair.second and do IsValidCookieAttributeValue(pair.second)? Saves a copy, but also makes it easier to just rip this code out later once you have your data. https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:396: if (!IsValidCookieAttributeValue(value)) Note that the call to IsValidCookieAttributeValue also sends its own histogram. https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:397: parsed_invalid_control_char_ = true; I suggest converting this to a local boolean and doing the UMA collection at the end of this method rather than adding member state.
https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.cc File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:397: parsed_invalid_control_char_ = true; > I suggest converting this to a local boolean and doing the UMA collection at the > end of this method rather than adding member state. I'll second that.
The overall motivation about measuring the already rejected cookies is to understand if we're seeing control characters at any point already. That is, are we seeing them during parsing only, or are we seeing them during attribute sets and we're just rejecting them. palmer, jschuh and I believe it will help to elucidate the source of control characters that we're seeing. https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.cc File net/cookies/parsed_cookie.cc (left): https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Nope, not at all. Fixed. On 2013/05/24 14:15:56, erikwright wrote: > Was this removed intentionally? https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.cc File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:134: UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieValue", false); On 2013/05/24 14:15:56, erikwright wrote: > This will be called for changing the value but not changing the name via the > setters. > > Consider moving it to SetName and SetValue instead. Done. https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:147: UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieAttributeValue", false); The goal of this measurement is to get an overall view of how often there are cookie values that are rejected because of control characters On a related note, in SetAttributePair, it seems like the IsValidAttrbitueValue is a redundant call because IsValidToken includes a check for control characters. Am I misunderstanding something there? I'm actually going to add a check in the cookie line parser to check how often parsed cookies are not valid tokens either. On 2013/05/24 14:15:56, erikwright wrote: > These metrics will be clouded because they cover both changes to attributes via > the accessors and the parsing of a cookie-line (both value and attributes) > during the constructor. > > Consider moving them to SetAttributePair instead. https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:396: if (!IsValidCookieAttributeValue(value)) On 2013/05/24 14:15:56, erikwright wrote: > Why not just put this after the assignment to pair.second and do > IsValidCookieAttributeValue(pair.second)? > > Saves a copy, but also makes it easier to just rip this code out later once you > have your data. Done. https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:396: if (!IsValidCookieAttributeValue(value)) See my other responses. We want to measure a couple of different things. On 2013/05/24 14:15:56, erikwright wrote: > Note that the call to IsValidCookieAttributeValue also sends its own histogram. https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:397: parsed_invalid_control_char_ = true; On 2013/05/24 14:15:56, erikwright wrote: > I suggest converting this to a local boolean and doing the UMA collection at the > end of this method rather than adding member state. Done. https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:397: parsed_invalid_control_char_ = true; On 2013/05/24 17:38:43, Chromium Palmer wrote: > > I suggest converting this to a local boolean and doing the UMA collection at > the > > end of this method rather than adding member state. > > I'll second that. Done.
Please add a file-level comment with a link to the bug # that explains why the metrics are there and a time-line for removal of the metrics. I guess the time-line would be post M29 branch-point under the assumption that you need no more than one stable release to get conclusive data? https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.cc File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... net/cookies/parsed_cookie.cc:147: UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieAttributeValue", false); On 2013/05/24 17:56:33, jww wrote: > The goal of this measurement is to get an overall view of how often there are > cookie values that are rejected because of control characters > > On a related note, in SetAttributePair, it seems like the IsValidAttrbitueValue > is a redundant call because IsValidToken includes a check for control > characters. Am I misunderstanding something there? I'm actually going to add a > check in the cookie line parser to check how often parsed cookies are not valid > tokens either. > On 2013/05/24 14:15:56, erikwright wrote: > > These metrics will be clouded because they cover both changes to attributes > via > > the accessors and the parsing of a cookie-line (both value and attributes) > > during the constructor. > > > > Consider moving them to SetAttributePair instead. > The key is checked with IsValidToken and the value is checked against IsValidAttributeValue. https://codereview.chromium.org/15897003/diff/14001/net/cookies/parsed_cookie.cc File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/15897003/diff/14001/net/cookies/parsed_cookie... net/cookies/parsed_cookie.cc:414: parsed_invalid_control_char); indentation (align with ")
On 2013/05/27 19:19:40, erikwright wrote: > Please add a file-level comment with a link to the bug # that explains why the > metrics are there and a time-line for removal of the metrics. I guess the > time-line would be post M29 branch-point under the assumption that you need no > more than one stable release to get conclusive data? Yes, that's the idea, although jschuh@ and palmer@ believe there's a good chance we'll get enough info to make a decision after a few days of canary. However, I think that we should proceed on the assumption that we'll keep them in through 1 stable release. > > https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.cc > File net/cookies/parsed_cookie.cc (right): > > https://codereview.chromium.org/15897003/diff/5001/net/cookies/parsed_cookie.... > net/cookies/parsed_cookie.cc:147: > UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieAttributeValue", false); > On 2013/05/24 17:56:33, jww wrote: > > The goal of this measurement is to get an overall view of how often there are > > cookie values that are rejected because of control characters > > > > On a related note, in SetAttributePair, it seems like the > IsValidAttrbitueValue > > is a redundant call because IsValidToken includes a check for control > > characters. Am I misunderstanding something there? I'm actually going to add a > > check in the cookie line parser to check how often parsed cookies are not > valid > > tokens either. > > On 2013/05/24 14:15:56, erikwright wrote: > > > These metrics will be clouded because they cover both changes to attributes > > via > > > the accessors and the parsing of a cookie-line (both value and attributes) > > > during the constructor. > > > > > > Consider moving them to SetAttributePair instead. > > > > The key is checked with IsValidToken and the value is checked against > IsValidAttributeValue. Ah, got it :-) > > https://codereview.chromium.org/15897003/diff/14001/net/cookies/parsed_cookie.cc > File net/cookies/parsed_cookie.cc (right): > > https://codereview.chromium.org/15897003/diff/14001/net/cookies/parsed_cookie... > net/cookies/parsed_cookie.cc:414: parsed_invalid_control_char); > indentation (align with ")
https://codereview.chromium.org/15897003/diff/14001/net/cookies/parsed_cookie.cc File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/15897003/diff/14001/net/cookies/parsed_cookie... net/cookies/parsed_cookie.cc:414: parsed_invalid_control_char); On 2013/05/27 19:19:41, erikwright wrote: > indentation (align with ") Done.
LGTM. Good luck! https://codereview.chromium.org/15897003/diff/21001/net/cookies/parsed_cookie.cc File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/15897003/diff/21001/net/cookies/parsed_cookie... net/cookies/parsed_cookie.cc:51: // We are collecting several UMA statistics in this file, and they relate to nit: Prefix with 'TODO(jww): Remove metrics code after M29.'
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/15897003/21001
https://codereview.chromium.org/15897003/diff/21001/net/cookies/parsed_cookie.cc File net/cookies/parsed_cookie.cc (right): https://codereview.chromium.org/15897003/diff/21001/net/cookies/parsed_cookie... net/cookies/parsed_cookie.cc:51: // We are collecting several UMA statistics in this file, and they relate to On 2013/05/28 19:59:05, erikwright wrote: > nit: Prefix with 'TODO(jww): Remove metrics code after M29.' Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/15897003/26001
Message was sent while issue was closed.
Change committed as 202721 |
