Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903213004:
Refactor transmission of security of source and http_only mods in cookie_monster.cc. (Closed)
|
Created:
3 years, 7 months ago by Randy Smith (Not in Mondays) Modified:
3 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor transmission of security of source and http_only mods in cookie_monster.cc.
Prep CL for https://codereview.chromium.org/2882063002
BUG=721395
R=mmenke@chromium.org
Review-Url: https://codereview.chromium.org/2903213004
Cr-Commit-Position: refs/heads/master@{#481002}
Committed: https://chromium.googlesource.com/chromium/src/+/2709eee26ac162dc961a19bed779a584bb49da02
Patch Set 1 #Patch Set 2 : Tweak ordering in header file. #
Total comments: 6
Patch Set 3 : Rebased to latest version of https://codereview.chromium.org/2898953008. #Patch Set 4 : Incorporated comments and refactored histogram to match old behavior. #Patch Set 5 : Rebased on newest PS on base CL. #Patch Set 6 : Revert test reference to histogram back. #
Total comments: 6
Patch Set 7 : Updated documentation comment. #
Total comments: 8
Patch Set 8 : Refactors to remove uneeded argument and do secure source testing lower in the stack. #Patch Set 9 : Rebased on top of dependent CL. #
Total comments: 4
Patch Set 10 : Fix incorrect arg to DeleteAnyEquivalentCookie. #Patch Set 11 : Rebase on latest PS of depencency CL. #
Total comments: 2
Patch Set 12 : Rebased to p480954 (and off of now committed dependency CL). #Patch Set 13 : Add garbage collection to SetAllCookies() path. #
Dependent Patchsets: Messages
Total messages: 63 (38 generated)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Next CL: just the secure/http_only refactor by itself. PTAL? I'm putting this CLs in a linearly dependent chain rather than creating the slightly more complicated DAG since I don't really care about the ability to commit them in parallel and it's simpler for me to track them this way. The actual dependency graph is that this CL is independent of the others, the other three are linearly dependent, and the last big one is also dependent on this one. But I'm not sure you need to care about that.
So much easier. https://codereview.chromium.org/2903213004/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (left): https://codereview.chromium.org/2903213004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1098: CookieOptions::SameSiteCookieMode::INCLUDE_STRICT_AND_LAX); So the INCLUDE_STRICT_AND_LAX here didn't matter? https://codereview.chromium.org/2903213004/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2903213004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1812: options.set_include_httponly(); Not needed? https://codereview.chromium.org/2903213004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:2249: "Cookie.CookieSourceScheme2", 1, COOKIE_SOURCE_LAST_ENTRY - 1, Need to update histograms.xml
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rdsmith@chromium.org changed reviewers: + droger@chromium.org
Matt: This is ready for another review; PTAL? Do note the comment below in response to your histogram comment--there's been some refactoring since the last round. David: I've added you as a reviewer because of the changes in SetAllCookiesAsync() semantics, but you're optional--as I noted in https://codereview.chromium.org/2882063002#msg65 it sounds like I'm free to fiddle with those semantics as is convenient. But do let me know if that's wrong. https://codereview.chromium.org/2903213004/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (left): https://codereview.chromium.org/2903213004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1098: CookieOptions::SameSiteCookieMode::INCLUDE_STRICT_AND_LAX); On 2017/05/25 20:04:29, mmenke wrote: > So the INCLUDE_STRICT_AND_LAX here didn't matter? I didn't find anything under SetCanonicalCookie that tested that option, no. I presume I would have had a compile failure (or a clear change in behavior somewhere else in the diff) if it had mattered, since the options variable is no longer present to be tested under SetCanonicalCookie. https://codereview.chromium.org/2903213004/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2903213004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1812: options.set_include_httponly(); On 2017/05/25 20:04:29, mmenke wrote: > Not needed? Whoops. Done. https://codereview.chromium.org/2903213004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:2249: "Cookie.CookieSourceScheme2", 1, COOKIE_SOURCE_LAST_ENTRY - 1, On 2017/05/25 20:04:29, mmenke wrote: > Need to update histograms.xml Huh. Coudla sworn I did. I wonder where I dropped that change on the floor. ETA: Well, that escalated quickly. In writing the comment describing more precisely what the difference in the histograms was, I realized that I was making a change that was probably *not* reasonable--I was now included restoration of cookies from the backing database on startup in the histogram when I hadn't been before. So I took a step back and refactored the code s.t. the only places that actually put entries into this histogram were places where the URL existed previously (i.e. the places where histogram entries originally occurred). So I don't need to change the histogram name anymore, as its behavior should now be identical. To give a bit more detail: * The histogram was being entered in InternalInsertCookie (on all calls to that function). * That function was being called from StoreLoadedCookies (initialization path) with a null URL (no histogram entry), and from SetCanonicalCookie() * SetCanonicalCookie() was being called from several places that check if their URL has a cookieable scheme (so presumably not a null URL) and SetCanonicalCookie*s*() with a null URL. * SetCanonicalCookies() was being called only from iOS. I took advantage of droger@'s permission to not worry much about the semantics of the SetCanonicalCookies() call, reimplemented it without calling SetCanonicalCookie(), and pulled the histogram from InternalInsertCookie() into SetCanonicalCookie(). I believe this keeps the histogram behavior identical to before this refactor.
https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... net/cookies/cookie_monster.h:155: // See https://codereview.chromium.org/2882063002/#msg64. If this function is broken in some way (i.e. does not support secure cookies), can you say it in the comment?
https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... net/cookies/cookie_monster.h:155: // See https://codereview.chromium.org/2882063002/#msg64. On 2017/06/09 12:22:52, droger wrote: > If this function is broken in some way (i.e. does not support secure cookies), > can you say it in the comment? It actually isn't, anymore; it's really just doing a "delete all the cookies that match this list, and insert everything in the list in the store." I figured that was what would be assumed from the comment and the name, so I didn't worry about putting extra information in. (A possible mis-assumption would be that this function deletes all cookies, not just the ones that are being overwritten. If you want, I can clarify that and/or make it delete all the cookies; just let me know.)
https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... net/cookies/cookie_monster.h:155: // See https://codereview.chromium.org/2882063002/#msg64. On 2017/06/09 13:43:34, Randy Smith (Not in Mondays) wrote: > On 2017/06/09 12:22:52, droger wrote: > > If this function is broken in some way (i.e. does not support secure cookies), > > can you say it in the comment? > > It actually isn't, anymore; it's really just doing a "delete all the cookies > that match this list, and insert everything in the list in the store." I > figured that was what would be assumed from the comment and the name, so I > didn't worry about putting extra information in. > > (A possible mis-assumption would be that this function deletes all cookies, not > just the ones that are being overwritten. If you want, I can clarify that > and/or make it delete all the cookies; just let me know.) I just thought (from comments on other CL) that this function was not working correctly with secure cookies. But if that's not the case, then it's fine like this.
https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... net/cookies/cookie_monster.h:155: // See https://codereview.chromium.org/2882063002/#msg64. On 2017/06/09 13:43:34, Randy Smith (Not in Mondays) wrote: > (A possible mis-assumption would be that this function deletes all cookies, not > just the ones that are being overwritten. If you want, I can clarify that > and/or make it delete all the cookies; just let me know.) Ah, after reading better... actually I fell into this mis-asumption, so it may be worth clarifying this.
https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... net/cookies/cookie_monster.h:155: // See https://codereview.chromium.org/2882063002/#msg64. On 2017/06/09 15:17:32, droger wrote: > On 2017/06/09 13:43:34, Randy Smith (Not in Mondays) wrote: > > On 2017/06/09 12:22:52, droger wrote: > > > If this function is broken in some way (i.e. does not support secure > cookies), > > > can you say it in the comment? > > > > It actually isn't, anymore; it's really just doing a "delete all the cookies > > that match this list, and insert everything in the list in the store." I > > figured that was what would be assumed from the comment and the name, so I > > didn't worry about putting extra information in. > > > > (A possible mis-assumption would be that this function deletes all cookies, > not > > just the ones that are being overwritten. If you want, I can clarify that > > and/or make it delete all the cookies; just let me know.) > > I just thought (from comments on other CL) that this function was not working > correctly with secure cookies. But if that's not the case, then it's fine like > this. It used to not work properly with secure cookies, but one of the effects of the refactor in this CL is that now it does. https://codereview.chromium.org/2903213004/diff/100001/net/cookies/cookie_mon... net/cookies/cookie_monster.h:155: // See https://codereview.chromium.org/2882063002/#msg64. On 2017/06/09 15:19:25, droger wrote: > On 2017/06/09 13:43:34, Randy Smith (Not in Mondays) wrote: > > (A possible mis-assumption would be that this function deletes all cookies, > not > > just the ones that are being overwritten. If you want, I can clarify that > > and/or make it delete all the cookies; just let me know.) > > Ah, after reading better... actually I fell into this mis-asumption, so it may > be worth clarifying this. Done.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Just a quick note to you both that I uploaded a bit of refactoring in the last PS that I thought was more conceptually in accord with this CL than with one of the dependent ones: Removing an unneeded argument, and doing secure source checking a bit lower in the stack.
So, modulo the new change which I haven't looked at yet, this effectively just moves the histogram logic, and changes some arguments to bools? https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1436: GetKey(cookie_ptr->Domain()), std::move(cookie), false, false); These used to get histogrammed, didn't they? Seems like a significant change to the histogram. https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1846: (cookie.ExpiryDate() - creation_time).InMinutes()); It seems to me like this histogram should only be updated when histogram_cookie_source_scheme_ is. Having them include different sets of cookies seems very confusing. https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1850: true); So you're also removing these from the histogram? That's the only reason for copying duplicate logic to this method?
On 2017/06/09 18:48:52, mmenke wrote: > So, modulo the new change which I haven't looked at yet, this effectively just > moves the histogram logic, and changes some arguments to bools? Yes. (Haven't looked at your other comments yet.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Matt: I think we have a mismatch in how we're reading the behavior of the code before this CL; would you be willing to read my comments and see if you can figure out which reading is correct? https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1436: GetKey(cookie_ptr->Domain()), std::move(cookie), false, false); On 2017/06/09 18:48:52, mmenke wrote: > These used to get histogrammed, didn't they? Seems like a significant change to > the histogram. As I read the code, the null URL resulted in them not getting histogrammed. What are you seeing that I'm not? https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1846: (cookie.ExpiryDate() - creation_time).InMinutes()); On 2017/06/09 18:48:52, mmenke wrote: > It seems to me like this histogram should only be updated when > histogram_cookie_source_scheme_ is. Having them include different sets of > cookies seems very confusing. I am attempting to faithfully replicate the behavior that was present before this CL. If you're suggesting that the original behavior was confusing I'm not going to argue with you, but I am going to request a tell-me-twice before I attempt to change it :-} (and I'll change it in a separate CL, which will include revving histograms). If you're perceiving that I am changing behavior in this CL, my argument that I'm not is that, before this CL, SetCanonicalCookies() called SetCanonicalCookie with a null GURL, which unilaterally did this histogram entry in the !already_expired && IsPersistent() case, and then called InternalInsertCookie, which didn't do the histogram_cookie_source_source_scheme_ insertion because of the null GURL. Do you see a hole in that logic? https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1850: true); On 2017/06/09 18:48:52, mmenke wrote: > So you're also removing these from the histogram? That's the only reason for > copying duplicate logic to this method? Yes, modulo I believe I'm not actually changing behavior. The places that passed in null GURLs disabled the histogram_cookie_source_scheme_ insertion in InternalInsertCookie. One of those cases I could deal with without the wonky null GURL signature just by pulling the histogram insertion up into SetCanonicalCookie(). This is the other one, and duplicating logic struck me as the simplest way to handle the case (as opposed to a boolean saying "Don't enter this into the histogram"), especially since this method isn't actually called by iOS and really should be gotten rid of at some point.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the delay in this one - every time I look at it, I have trouble making sense of all the Set[Foo][Canonical]Cookie[WithBar][AndHats], even though it shouldn't be that complicated. I'll plan to (finally) get back to it tomorrow.
On 2017/06/15 21:37:54, mmenke wrote: > Sorry for the delay in this one - every time I look at it, I have trouble making > sense of all the Set[Foo][Canonical]Cookie[WithBar][AndHats], even though it > shouldn't be that complicated. I'll plan to (finally) get back to it tomorrow. Huh; my apologies. If there's a way to split it up that might be useful, or you just want me to do a written summary of what the code changes are for framework, let me know. Or grab me and I'll scribble on a whiteboard for a while--whatever would help.
On 2017/06/15 21:43:41, Randy Smith (Not in Mondays) wrote: > On 2017/06/15 21:37:54, mmenke wrote: > > Sorry for the delay in this one - every time I look at it, I have trouble > making > > sense of all the Set[Foo][Canonical]Cookie[WithBar][AndHats], even though it > > shouldn't be that complicated. I'll plan to (finally) get back to it > tomorrow. > > Huh; my apologies. If there's a way to split it up that might be useful, or you > just want me to do a written summary of what the code changes are for framework, > let me know. Or grab me and I'll scribble on a whiteboard for a while--whatever > would help. I don't think it's your fault at all, it's not a very big or complicated change, either. I'm not sure why I'm having trouble with it.
Not sure why this one continues to give me so much trouble - maybe because I can't keep the methods straight, or their responsibilities before or after this change. https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1436: GetKey(cookie_ptr->Domain()), std::move(cookie), false, false); On 2017/06/13 20:53:40, Randy Smith (Not in Mondays) wrote: > On 2017/06/09 18:48:52, mmenke wrote: > > These used to get histogrammed, didn't they? Seems like a significant change > to > > the histogram. > > As I read the code, the null URL resulted in them not getting histogrammed. > What are you seeing that I'm not? You're right, I missed that...And it seems like a really weird design decision. Too much magic. :( https://codereview.chromium.org/2903213004/diff/160001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2903213004/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1835: DCHECK(!result); Why can we DCHECK on this? The old method returned false when this happened (Like when an input cookie was secure). https://codereview.chromium.org/2903213004/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1845: InternalInsertCookie(key, base::MakeUnique<CanonicalCookie>(cookie), true); This doesn't run garbage collection. The old method did. Do we care?
Hmmm. Here's an attempt at a top level summary of the change, if it helps: * Change SetCanonicalCookie() helper function to take booleans indicating its privileges. This will allow a follow-on CL to set cookies without reference to URLs. * The one area in which the URL argument had been used for more than just an http/https bit is in the entering of data into the histogram_cookie_source_scheme_; if the URL will null, no entry was entered into this histogram. Access to this histogram needed to be refactored in order to allow the argument to SetCanonicalCookie() to be purely a boolean (secure_source). * InternalInsertCookie (the original location of the histogram) was called from two locations: SetCanonicalCookie and StoreLoadedCookies. StoreLoadedCookies provided a null URL, so hoisting InternalInsertCookie into SetCanonicalCookie() doesn't change behavior. * SetCanonicalCookie() is called from several locations, all of which except provide valid URLs except for SetCanonicalCookies(). Rewriting SetCanonicalCookies() to avoid calling SetCanonicalCookie() and unilaterally entering data in histogram_cookie_source_scheme_ in SetCanonicalCookie() without reference to null URL will thus maintain the current behavior. * SetCanonicalCookies() has only a single consumer, which has approved changes in behavior which make the rewrite simpler (arguably it should be removed, but that's beyond the scope of this CL). Does that help at all? Does it help enough that I should include it in the description? https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2903213004/diff/120001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1436: GetKey(cookie_ptr->Domain()), std::move(cookie), false, false); On 2017/06/16 16:19:48, mmenke wrote: > On 2017/06/13 20:53:40, Randy Smith (Not in Mondays) wrote: > > On 2017/06/09 18:48:52, mmenke wrote: > > > These used to get histogrammed, didn't they? Seems like a significant > change > > to > > > the histogram. > > > > As I read the code, the null URL resulted in them not getting histogrammed. > > What are you seeing that I'm not? > > You're right, I missed that...And it seems like a really weird design decision. > Too much magic. :( I suspect it wasn't a decision :-{. But I'm trying to make this a targeted refactor rather than a cleanup. https://codereview.chromium.org/2903213004/diff/160001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2903213004/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1835: DCHECK(!result); On 2017/06/16 16:19:48, mmenke wrote: > Why can we DCHECK on this? The old method returned false when this happened > (Like when an input cookie was secure). Yes, for SetAllCookies I'm explicitly not keeping behavior as per the conversation with droger@ (owner of only consumer) and the changes in the header documentation comment. But you're quite right that this is a bug--my intent had been to give DeleteAnyEquivalentCookie full privileges and have it just run rampant through the input cookie list, but that requires "true, false" (source_secure true, skip_httponly false) and without that the DCHECK is incorrect. Done. https://codereview.chromium.org/2903213004/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1845: InternalInsertCookie(key, base::MakeUnique<CanonicalCookie>(cookie), true); On 2017/06/16 16:19:48, mmenke wrote: > This doesn't run garbage collection. The old method did. Do we care? I consider it easier to add GC back in than to answer your philosophical question :-} (but also :-|--I certainly don't care a *lot* about the behavior of this method.) I hadn't intended to change the behavior in removing it. Done.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
I believe the try bot failures are due to not rebasing on later versions of dependency CLs and this CL is still ok for review.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2903213004/diff/200001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2903213004/diff/200001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1845: InternalInsertCookie(key, base::MakeUnique<CanonicalCookie>(cookie), true); On 2017/06/16 20:53:58, Randy Smith (Not in Mondays) wrote: > On 2017/06/16 16:19:48, mmenke wrote: > > This doesn't run garbage collection. The old method did. Do we care? > > I consider it easier to add GC back in than to answer your philosophical > question :-} (but also :-|--I certainly don't care a *lot* about the behavior of > this method.) I hadn't intended to change the behavior in removing it. > Done. I don't think this was done?
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Want another pass, Matt? It's just the addition of the same "GarbageCollect()" line as was in SetCanonicalCookie(). https://codereview.chromium.org/2903213004/diff/200001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2903213004/diff/200001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1845: InternalInsertCookie(key, base::MakeUnique<CanonicalCookie>(cookie), true); On 2017/06/19 18:00:18, mmenke wrote: > On 2017/06/16 20:53:58, Randy Smith (Not in Mondays) wrote: > > On 2017/06/16 16:19:48, mmenke wrote: > > > This doesn't run garbage collection. The old method did. Do we care? > > > > I consider it easier to add GC back in than to answer your philosophical > > question :-} (but also :-|--I certainly don't care a *lot* about the behavior > of > > this method.) I hadn't intended to change the behavior in removing it. > > Done. > > I don't think this was done? Whoops, sorry. I *remember* doing it--I wonder which CL that actually landed in? Oh, well, some merge conflict will eventually tell me :-}. Done.
On 2017/06/20 21:04:03, Randy Smith (Not in Mondays) wrote: > Want another pass, Matt? It's just the addition of the same "GarbageCollect()" > line as was in SetCanonicalCookie(). I don't think it's needed, but regardless, still LGTM.
On 2017/06/20 21:59:20, mmenke wrote: > On 2017/06/20 21:04:03, Randy Smith (Not in Mondays) wrote: > > Want another pass, Matt? It's just the addition of the same > "GarbageCollect()" > > line as was in SetCanonicalCookie(). > > I don't think it's needed, but regardless, still LGTM. Given the choice, I'd rather not change behavior in a refactor CL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdsmith@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1497998393868960,
"parent_rev": "56d4ebf5f6661a82c992a2d7a5b9c494cb30f797", "commit_rev":
"2709eee26ac162dc961a19bed779a584bb49da02"}
Message was sent while issue was closed.
Description was changed from ========== Refactor transmission of security of source and http_only mods in cookie_monster.cc. Prep CL for https://codereview.chromium.org/2882063002 BUG=721395 R=mmenke@chromium.org ========== to ========== Refactor transmission of security of source and http_only mods in cookie_monster.cc. Prep CL for https://codereview.chromium.org/2882063002 BUG=721395 R=mmenke@chromium.org Review-Url: https://codereview.chromium.org/2903213004 Cr-Commit-Position: refs/heads/master@{#481002} Committed: https://chromium.googlesource.com/chromium/src/+/2709eee26ac162dc961a19bed779... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/2709eee26ac162dc961a19bed779...
Message was sent while issue was closed.
On 2017/06/20 22:02:28, Randy Smith (Not in Mondays) wrote: > On 2017/06/20 21:59:20, mmenke wrote: > > On 2017/06/20 21:04:03, Randy Smith (Not in Mondays) wrote: > > > Want another pass, Matt? It's just the addition of the same > > "GarbageCollect()" > > > line as was in SetCanonicalCookie(). > > > > I don't think it's needed, but regardless, still LGTM. > > Given the choice, I'd rather not change behavior in a refactor CL. > > Thanks! Sorry, I meant I don't think another review pass is needed. I agree we don't want to change behavior. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
