Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2882063002:
Add a SetCanonicalCookie method for CookieMonster. (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, asvitkine+watch_chromium.org, net-reviews_chromium.org, msarda Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a SetCanonicalCookie method for CookieMonster.
This includes some refactoring of creation time defaulting, as well
as a histogram revision bump because the information about whether a
cookie is being set based on a null URL is no longer available at
the point of histogram creation.
BUG=721395, 723734
R=mmenke@chromium.org
Review-Url: https://codereview.chromium.org/2882063002
Cr-Commit-Position: refs/heads/master@{#481227}
Committed: https://chromium.googlesource.com/chromium/src/+/a6ce44474ee162592d04a31cf0b243c31a53f57d
Patch Set 1 #Patch Set 2 : Pushed new interface into CookieStore. #Patch Set 3 : Fix IO Braino. #Patch Set 4 : Fixed try job problems. #Patch Set 5 : Fix try job problems. #Patch Set 6 : Sync'd to p272216 #Patch Set 7 : Results of self review. #Patch Set 8 : Conditionalize http_only test on support. #Patch Set 9 : Fix try jobs and do some cleanup. #
Total comments: 5
Patch Set 10 : Fix behavior change. #Patch Set 11 : Rebased on top of dependency CL. #Patch Set 12 : Adapt test to new secure source definition. #Patch Set 13 : Fix iOS behavior for secure cookies. #
Total comments: 14
Patch Set 14 : Integrated Matt's comments. #Patch Set 15 : Fix iOS compile. #Patch Set 16 : Fix AW cookie store wrapper. #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 106 (69 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Add a SetCanonicalCookie method for CookieMonster. This includes some refactoring of creation time defaulting, as well as a histogram revision bump because the information about whether a cookie is being set based on a null URL is no longer available at the point of histogram creation. BUG=721395 R=mmenke@chromium.org ========== to ========== Add a SetCanonicalCookie method for CookieMonster. This includes some refactoring of creation time defaulting, as well as a histogram revision bump because the information about whether a cookie is being set based on a null URL is no longer available at the point of histogram creation. BUG=721395, 723734 R=mmenke@chromium.org ==========
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.
Matt, PTAL? I sketched out one issue you may want to think about in https://bugs.chromium.org/p/chromium/issues/detail?id=723734#c4. The other area where I think there's a >0 chance you'll disagree with my approach is around creating a setter specifically for creation time in the null case on canonical cookies, to allow a null creation time to mean "default" when passing into the CookieMonster (and a minor sub-choice of that design, which is requiring creation time to actually be non-null in CC::Create(url,cookie_line,creation_time, options)).
Not a full review, just a tiny little question. https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1797: !options.exclude_httponly()); This is a web-visible behavior change, right? This path didn't used to prevent non-secure URLs from setting secure cookies, it only prevented from overwriting non-secure cookies.
https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1797: !options.exclude_httponly()); On 2017/05/22 18:51:27, mmenke wrote: > This is a web-visible behavior change, right? This path didn't used to prevent > non-secure URLs from setting secure cookies, it only prevented from overwriting > non-secure cookies. If this is a web-visible behavior change, this should be done in a way it can be independently reverted, and we should have Mike West on board, and maybe even an intent email. I actually think the current behavior is bizarre (And I think it violate's Mike Wests's own spec (Google leave secure cookies alone)), but we got plenty of complains about it. We may see even more about this change, just after people fixed their code to work with the new behavior.
https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1797: !options.exclude_httponly()); On 2017/05/22 19:07:04, mmenke wrote: > On 2017/05/22 18:51:27, mmenke wrote: > > This is a web-visible behavior change, right? This path didn't used to > prevent > > non-secure URLs from setting secure cookies, it only prevented from > overwriting > > non-secure cookies. Aw, drat, it looks like I refactored a check from SetCookieWithDetails down into the guts of the system. Right you are. (Presuming you meant "prevented non-secure URLs from overwriting secure cookies".) > > If this is a web-visible behavior change, this should be done in a way it can be > independently reverted, and we should have Mike West on board, and maybe even an > intent email. I actually think the current behavior is bizarre (And I think it > violate's Mike Wests's own spec (Google leave secure cookies alone)), but we got > plenty of complains about it. We may see even more about this change, just > after people fixed their code to work with the new behavior. I don't intend any web visible change. I'll refactor to preserve the old behavior. You can either wait for that for your review or review now, as you choose.
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...
Behavior change believed fixed (easier than I thought) and a test added for the oddity. PTAL.
On 2017/05/22 22:11:35, Randy Smith (Not in Mondays) wrote: > Behavior change believed fixed (easier than I thought) and a test added for the > oddity. PTAL. (On a side note, I misread the leave secure cookies alone doc - this weird behavior does seem to match it). I'll plan to take a look tomorrow, if I can find the time. If not, Wednesday.
On 2017/05/22 22:17:03, mmenke wrote: > On 2017/05/22 22:11:35, Randy Smith (Not in Mondays) wrote: > > Behavior change believed fixed (easier than I thought) and a test added for > the > > oddity. PTAL. > > (On a side note, I misread the leave secure cookies alone doc - this weird > behavior does seem to match it). > > I'll plan to take a look tomorrow, if I can find the time. If not, Wednesday. No rush--while this is on my main path, I'm finding it pretty easy to stack CLs, and I've got a couple of reviews of my own. Wednesday's fine.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/05/22 22:18:54, Randy Smith (Not in Mondays) wrote: > On 2017/05/22 22:17:03, mmenke wrote: > > On 2017/05/22 22:11:35, Randy Smith (Not in Mondays) wrote: > > > Behavior change believed fixed (easier than I thought) and a test added for > > the > > > oddity. PTAL. > > > > (On a side note, I misread the leave secure cookies alone doc - this weird > > behavior does seem to match it). > > > > I'll plan to take a look tomorrow, if I can find the time. If not, Wednesday. > > No rush--while this is on my main path, I'm finding it pretty easy to stack CLs, > and I've got a couple of reviews of my own. Wednesday's fine. Randy, sorry for the delay. I'm just having trouble making sense of the changes to net/cookies/cookie_monster.cc. Would you mind splitting off the new method and actual_creation_time logic relocation into their own CL, separate from the argument refactor. There are just too many Set methods here for me to keep them straight.
On 2017/05/25 15:35:07, mmenke wrote: > On 2017/05/22 22:18:54, Randy Smith (Not in Mondays) wrote: > > On 2017/05/22 22:17:03, mmenke wrote: > > > On 2017/05/22 22:11:35, Randy Smith (Not in Mondays) wrote: > > > > Behavior change believed fixed (easier than I thought) and a test added > for > > > the > > > > oddity. PTAL. > > > > > > (On a side note, I misread the leave secure cookies alone doc - this weird > > > behavior does seem to match it). > > > > > > I'll plan to take a look tomorrow, if I can find the time. If not, > Wednesday. > > > > No rush--while this is on my main path, I'm finding it pretty easy to stack > CLs, > > and I've got a couple of reviews of my own. Wednesday's fine. > > Randy, sorry for the delay. I'm just having trouble making sense of the changes > to net/cookies/cookie_monster.cc. Would you mind splitting off the new method > and actual_creation_time logic relocation into their own CL, separate from the > argument refactor. There are just too many Set methods here for me to keep them > straight. (The argument refactor meaning the URL+options to bool+bool change, which I assume would be a fairly simple CL, and I could review first, without mixing that with behavior changes)
https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1797: !options.exclude_httponly()); On 2017/05/22 21:54:03, Randy Smith (Not in Mondays) wrote: > On 2017/05/22 19:07:04, mmenke wrote: > > On 2017/05/22 18:51:27, mmenke wrote: > > > This is a web-visible behavior change, right? This path didn't used to > > prevent > > > non-secure URLs from setting secure cookies, it only prevented from > > overwriting > > > non-secure cookies. > > Aw, drat, it looks like I refactored a check from SetCookieWithDetails down into > the guts of the system. Right you are. > > (Presuming you meant "prevented non-secure URLs from overwriting secure > cookies".) > > > > > If this is a web-visible behavior change, this should be done in a way it can > be > > independently reverted, and we should have Mike West on board, and maybe even > an > > intent email. I actually think the current behavior is bizarre (And I think > it > > violate's Mike Wests's own spec (Google leave secure cookies alone)), but we > got > > plenty of complains about it. We may see even more about this change, just > > after people fixed their code to work with the new behavior. > > I don't intend any web visible change. I'll refactor to preserve the old > behavior. You can either wait for that for your review or review now, as you > choose. Matt: I was digging into this issue more closely (basically, writing tests to enforce the behavior at the CookieMonster API level for those interfaces that can set secure cookies from non-secure origins) and discovered that the interface I *thought* could (SetCookieWithOptions) actually can't because the CanonicalCookie constructor it uses does the check. This makes me suspect that there aren't any interfaces that allow this; SetCookieWithDetails has the check explicitly. I'll do a more thorough exploration of the source, but: Do you know of any existing external interfaces you think should have the odd behavior? Because if there aren't any I'm inclined to refactor it down below SetCanonicalCookie as in my first patchset.
On 2017/05/25 20:45:35, Randy Smith (Not in Mondays) wrote: > https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... > File net/cookies/cookie_monster.cc (right): > > https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... > net/cookies/cookie_monster.cc:1797: !options.exclude_httponly()); > On 2017/05/22 21:54:03, Randy Smith (Not in Mondays) wrote: > > On 2017/05/22 19:07:04, mmenke wrote: > > > On 2017/05/22 18:51:27, mmenke wrote: > > > > This is a web-visible behavior change, right? This path didn't used to > > > prevent > > > > non-secure URLs from setting secure cookies, it only prevented from > > > overwriting > > > > non-secure cookies. > > > > Aw, drat, it looks like I refactored a check from SetCookieWithDetails down > into > > the guts of the system. Right you are. > > > > (Presuming you meant "prevented non-secure URLs from overwriting secure > > cookies".) > > > > > > > > If this is a web-visible behavior change, this should be done in a way it > can > > be > > > independently reverted, and we should have Mike West on board, and maybe > even > > an > > > intent email. I actually think the current behavior is bizarre (And I think > > it > > > violate's Mike Wests's own spec (Google leave secure cookies alone)), but we > > got > > > plenty of complains about it. We may see even more about this change, just > > > after people fixed their code to work with the new behavior. > > > > I don't intend any web visible change. I'll refactor to preserve the old > > behavior. You can either wait for that for your review or review now, as you > > choose. > > Matt: I was digging into this issue more closely (basically, writing tests to > enforce the behavior at the CookieMonster API level for those interfaces that > can set secure cookies from non-secure origins) and discovered that the > interface I *thought* could (SetCookieWithOptions) actually can't because the > CanonicalCookie constructor it uses does the check. This makes me suspect that > there aren't any interfaces that allow this; SetCookieWithDetails has the check > explicitly. I'll do a more thorough exploration of the source, but: Do you know > of any existing external interfaces you think should have the odd behavior? > Because if there aren't any I'm inclined to refactor it down below > SetCanonicalCookie as in my first patchset. The SetCookieWithOptions variant that takes a string - I don't see any checks in SetCookieWithCreationTimeAndOptions.
On 2017/05/25 20:51:06, mmenke wrote: > On 2017/05/25 20:45:35, Randy Smith (Not in Mondays) wrote: > > > https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... > > File net/cookies/cookie_monster.cc (right): > > > > > https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... > > net/cookies/cookie_monster.cc:1797: !options.exclude_httponly()); > > On 2017/05/22 21:54:03, Randy Smith (Not in Mondays) wrote: > > > On 2017/05/22 19:07:04, mmenke wrote: > > > > On 2017/05/22 18:51:27, mmenke wrote: > > > > > This is a web-visible behavior change, right? This path didn't used to > > > > prevent > > > > > non-secure URLs from setting secure cookies, it only prevented from > > > > overwriting > > > > > non-secure cookies. > > > > > > Aw, drat, it looks like I refactored a check from SetCookieWithDetails down > > into > > > the guts of the system. Right you are. > > > > > > (Presuming you meant "prevented non-secure URLs from overwriting secure > > > cookies".) > > > > > > > > > > > If this is a web-visible behavior change, this should be done in a way it > > can > > > be > > > > independently reverted, and we should have Mike West on board, and maybe > > even > > > an > > > > intent email. I actually think the current behavior is bizarre (And I > think > > > it > > > > violate's Mike Wests's own spec (Google leave secure cookies alone)), but > we > > > got > > > > plenty of complains about it. We may see even more about this change, > just > > > > after people fixed their code to work with the new behavior. > > > > > > I don't intend any web visible change. I'll refactor to preserve the old > > > behavior. You can either wait for that for your review or review now, as > you > > > choose. > > > > Matt: I was digging into this issue more closely (basically, writing tests to > > enforce the behavior at the CookieMonster API level for those interfaces that > > can set secure cookies from non-secure origins) and discovered that the > > interface I *thought* could (SetCookieWithOptions) actually can't because the > > CanonicalCookie constructor it uses does the check. This makes me suspect > that > > there aren't any interfaces that allow this; SetCookieWithDetails has the > check > > explicitly. I'll do a more thorough exploration of the source, but: Do you > know > > of any existing external interfaces you think should have the odd behavior? > > Because if there aren't any I'm inclined to refactor it down below > > SetCanonicalCookie as in my first patchset. > > The SetCookieWithOptions variant that takes a string - I don't see any checks in > SetCookieWithCreationTimeAndOptions. (DeleteAnyEquivalentCookie prevents overwriting a secure cookie from an unsecure origin, but not writing on in the first place, and I don't see any checks in InternalInsertCookie)
rdsmith@chromium.org changed reviewers: + droger@chromium.org
+droger@ for the question below; this CL is not yet ready for external review. https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1797: !options.exclude_httponly()); On 2017/05/25 20:45:35, Randy Smith (Not in Mondays) wrote: > On 2017/05/22 21:54:03, Randy Smith (Not in Mondays) wrote: > > On 2017/05/22 19:07:04, mmenke wrote: > > > On 2017/05/22 18:51:27, mmenke wrote: > > > > This is a web-visible behavior change, right? This path didn't used to > > > prevent > > > > non-secure URLs from setting secure cookies, it only prevented from > > > overwriting > > > > non-secure cookies. > > > > Aw, drat, it looks like I refactored a check from SetCookieWithDetails down > into > > the guts of the system. Right you are. > > > > (Presuming you meant "prevented non-secure URLs from overwriting secure > > cookies".) > > > > > > > > If this is a web-visible behavior change, this should be done in a way it > can > > be > > > independently reverted, and we should have Mike West on board, and maybe > even > > an > > > intent email. I actually think the current behavior is bizarre (And I think > > it > > > violate's Mike Wests's own spec (Google leave secure cookies alone)), but we > > got > > > plenty of complains about it. We may see even more about this change, just > > > after people fixed their code to work with the new behavior. > > > > I don't intend any web visible change. I'll refactor to preserve the old > > behavior. You can either wait for that for your review or review now, as you > > choose. > > Matt: I was digging into this issue more closely (basically, writing tests to > enforce the behavior at the CookieMonster API level for those interfaces that > can set secure cookies from non-secure origins) and discovered that the > interface I *thought* could (SetCookieWithOptions) actually can't because the > CanonicalCookie constructor it uses does the check. This makes me suspect that > there aren't any interfaces that allow this; SetCookieWithDetails has the check > explicitly. I'll do a more thorough exploration of the source, but: Do you know > of any existing external interfaces you think should have the odd behavior? > Because if there aren't any I'm inclined to refactor it down below > SetCanonicalCookie as in my first patchset. Followup: The only place where this might be a behavior change is iOS, and comments in the code suggest that it's not expected to work. SetCanonicalCookie (which would allow it) is according to codesearch called from three places: * SetCookieWithDetails: Explicitly disallowed. * SetCookieWithCreationTimeAndOptions: Uses a CC constructor that disallows * SetCanonicalCookies: Ok, there's a behavior change here, but I think the current behavior is unintended. This is called from the SetAllCookiesAsync() interface, which doesn't take a URL. It deletes all the cookies *not* in the new list, then sets all the ones that are. So it'll currently succeed in setting a secure cookie, but if there's an existing one it'll fail. But there's a comment in the source // Use an empty GURL. This method does not support setting secure cookies. And the method is only called outside of tests on IOS when copying system cookies into the cookie monster in CookieStoreIOS::WriteToCookieMonster. So changing this would be a behavior change in a corner case, with comments suggesting it wasn't intended to be supported. Oh joy. droger@: Any opinions on whether or not this is needed for the IOS Case mentioned above? What currently happens is that secure cookies that aren't replacing existing secure cookies of the same name in the cookie monster will be store, but secure cookies that collide with already existing secure cookies will be rejected. If I make the change I want to, WriteToCookieMonster/SetAllCookiesAsync would simply refuse to write secure cookies. WDYT?
On 2017/05/25 20:51:06, mmenke wrote: > On 2017/05/25 20:45:35, Randy Smith (Not in Mondays) wrote: > > > https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... > > File net/cookies/cookie_monster.cc (right): > > > > > https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... > > net/cookies/cookie_monster.cc:1797: !options.exclude_httponly()); > > On 2017/05/22 21:54:03, Randy Smith (Not in Mondays) wrote: > > > On 2017/05/22 19:07:04, mmenke wrote: > > > > On 2017/05/22 18:51:27, mmenke wrote: > > > > > This is a web-visible behavior change, right? This path didn't used to > > > > prevent > > > > > non-secure URLs from setting secure cookies, it only prevented from > > > > overwriting > > > > > non-secure cookies. > > > > > > Aw, drat, it looks like I refactored a check from SetCookieWithDetails down > > into > > > the guts of the system. Right you are. > > > > > > (Presuming you meant "prevented non-secure URLs from overwriting secure > > > cookies".) > > > > > > > > > > > If this is a web-visible behavior change, this should be done in a way it > > can > > > be > > > > independently reverted, and we should have Mike West on board, and maybe > > even > > > an > > > > intent email. I actually think the current behavior is bizarre (And I > think > > > it > > > > violate's Mike Wests's own spec (Google leave secure cookies alone)), but > we > > > got > > > > plenty of complains about it. We may see even more about this change, > just > > > > after people fixed their code to work with the new behavior. > > > > > > I don't intend any web visible change. I'll refactor to preserve the old > > > behavior. You can either wait for that for your review or review now, as > you > > > choose. > > > > Matt: I was digging into this issue more closely (basically, writing tests to > > enforce the behavior at the CookieMonster API level for those interfaces that > > can set secure cookies from non-secure origins) and discovered that the > > interface I *thought* could (SetCookieWithOptions) actually can't because the > > CanonicalCookie constructor it uses does the check. This makes me suspect > that > > there aren't any interfaces that allow this; SetCookieWithDetails has the > check > > explicitly. I'll do a more thorough exploration of the source, but: Do you > know > > of any existing external interfaces you think should have the odd behavior? > > Because if there aren't any I'm inclined to refactor it down below > > SetCanonicalCookie as in my first patchset. > > The SetCookieWithOptions variant that takes a string - I don't see any checks in > SetCookieWithCreationTimeAndOptions. SetCookieWithCreationTimeAndOptions uses a CanonicalCookie Create() method that returns null in this case.
On 2017/05/25 20:51:06, mmenke wrote: > On 2017/05/25 20:45:35, Randy Smith (Not in Mondays) wrote: > > > https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... > > File net/cookies/cookie_monster.cc (right): > > > > > https://codereview.chromium.org/2882063002/diff/160001/net/cookies/cookie_mon... > > net/cookies/cookie_monster.cc:1797: !options.exclude_httponly()); > > On 2017/05/22 21:54:03, Randy Smith (Not in Mondays) wrote: > > > On 2017/05/22 19:07:04, mmenke wrote: > > > > On 2017/05/22 18:51:27, mmenke wrote: > > > > > This is a web-visible behavior change, right? This path didn't used to > > > > prevent > > > > > non-secure URLs from setting secure cookies, it only prevented from > > > > overwriting > > > > > non-secure cookies. > > > > > > Aw, drat, it looks like I refactored a check from SetCookieWithDetails down > > into > > > the guts of the system. Right you are. > > > > > > (Presuming you meant "prevented non-secure URLs from overwriting secure > > > cookies".) > > > > > > > > > > > If this is a web-visible behavior change, this should be done in a way it > > can > > > be > > > > independently reverted, and we should have Mike West on board, and maybe > > even > > > an > > > > intent email. I actually think the current behavior is bizarre (And I > think > > > it > > > > violate's Mike Wests's own spec (Google leave secure cookies alone)), but > we > > > got > > > > plenty of complains about it. We may see even more about this change, > just > > > > after people fixed their code to work with the new behavior. > > > > > > I don't intend any web visible change. I'll refactor to preserve the old > > > behavior. You can either wait for that for your review or review now, as > you > > > choose. > > > > Matt: I was digging into this issue more closely (basically, writing tests to > > enforce the behavior at the CookieMonster API level for those interfaces that > > can set secure cookies from non-secure origins) and discovered that the > > interface I *thought* could (SetCookieWithOptions) actually can't because the > > CanonicalCookie constructor it uses does the check. This makes me suspect > that > > there aren't any interfaces that allow this; SetCookieWithDetails has the > check > > explicitly. I'll do a more thorough exploration of the source, but: Do you > know > > of any existing external interfaces you think should have the odd behavior? > > Because if there aren't any I'm inclined to refactor it down below > > SetCanonicalCookie as in my first patchset. > > The SetCookieWithOptions variant that takes a string - I don't see any checks in > SetCookieWithCreationTimeAndOptions. SetCookieWithCreationTimeAndOptions uses a CanonicalCookie Create() method that returns null in this case.
And, for the record - I verified the Randy is correct, and I *think* that the behavior actually matches spec (The part about not overwriting secure cookies and not creating new ones are in separate sections...of course).
Reply to comment #55: CC msarda FYI (for the signin interaction) and eugenebut for the WKWebView question at the end. This piece of code would be used when copying cookies from the iOS native store to the net CookieMonster. This used to be called all the time, but since the move to WKWebview, it's not clear how useful it is. I see that a failure causes an early return in SetCanonicalCookies(), which means that potentially all the cookies could be dropped if the failure happens on the first item of the list. We never noticed that some cookies failed to copy though, even when this was called extensively. I wonder why. If you remove the ability to set secure cookies, I think you might as well remove the SetAllCookiesAsync() function entirely. I don't think we need a function that is able to set only insecure cookies. I can think of potentially two cases where this code might be still needed: - signin still uses UIWebView, and does some cookie swapping. After discussion with msarda (CC'ed), I think that should be fine, but at least we should check that the no cookies are lost on Chrome signin before actually landing the change. - I don't know how WKWebView actually persist cookies. Does it have its own internal mechanism, or does it periodically read and write to the NSHTTPCookieStorage? In the latter case, changing that code might cause regressions. eugenebut, do you know more about that?
> - I don't know how WKWebView actually persist cookies. Does it have its own > internal mechanism, or does it periodically read and write to the > NSHTTPCookieStorage? In the latter case, changing that code might cause > regressions. eugenebut, do you know more about that? WKWebView periodically flushes coolies to NSHTTPCookieStorage and also reads them from the storage when web process starts. Writing to NSHTTPCookieStorage will affect WKWebView.
On 2017/05/30 14:32:57, Eugene But wrote: > > - I don't know how WKWebView actually persist cookies. Does it have its own > > internal mechanism, or does it periodically read and write to the > > NSHTTPCookieStorage? In the latter case, changing that code might cause > > regressions. eugenebut, do you know more about that? > WKWebView periodically flushes coolies to NSHTTPCookieStorage and also reads > them from the storage when web process starts. Writing to NSHTTPCookieStorage > will affect WKWebView. Thanks Eugene! Randy: Then my gut feeling is that we can probably remove it (because if we really needed it we would have noticed that some secure cookies would fail to persist), but we need to dig a bit more into it and do some debugging to be sure. If this is still something you are interested in, I can probably find some time to investigate a bit more for you, just let me know.
On 2017/05/30 14:58:11, droger wrote: > On 2017/05/30 14:32:57, Eugene But wrote: > > > - I don't know how WKWebView actually persist cookies. Does it have its own > > > internal mechanism, or does it periodically read and write to the > > > NSHTTPCookieStorage? In the latter case, changing that code might cause > > > regressions. eugenebut, do you know more about that? > > WKWebView periodically flushes coolies to NSHTTPCookieStorage and also reads > > them from the storage when web process starts. Writing to NSHTTPCookieStorage > > will affect WKWebView. > > Thanks Eugene! > > Randy: Then my gut feeling is that we can probably remove it (because if we > really needed it we would have noticed that some secure cookies would fail to > persist), but we need to dig a bit more into it and do some debugging to be > sure. If this is still something you are interested in, I can probably find some > time to investigate a bit more for you, just let me know. Thanks, David! I think I would like to look into removing it; it's a noticeable wart on the side of the API at the moment, and while I presume I could mimic the current behavior it would be messy. If it helps, I'd be happy to change the behavior of SetCanonicalCookies() to not return early, i.e. to set all cookies it's reasonable to set. What kind of timing is that likely to happen on? I'm debating how I can move forward on this CL while you're doing that. Thanks again.
On 2017/05/31 15:14:58, Randy Smith (Not in Mondays) wrote: > What kind of timing is that likely to happen on? I'm debating how I can move > forward on this CL while you're doing that. I'll look at this tomorrow, if I can remember to bring my mac laptop in the office.
On 2017/06/01 15:21:53, droger wrote: > On 2017/05/31 15:14:58, Randy Smith (Not in Mondays) wrote: > > What kind of timing is that likely to happen on? I'm debating how I can move > > forward on this CL while you're doing that. > > I'll look at this tomorrow, if I can remember to bring my mac laptop in the > office. Sorry for the delay. I managed to make a fresh build for iOS and put breakpoints in SetAllCookiesAsync(). I could only have this code called from the signin flow, but this flow does not actually care about actually saving cookies. I did not see the function fire as a result from WKWebView loading or saving cookies. So I think it is fine to break the behavior, or even to remove the function. Feel free to remove the call to SetAllCookiesAsync() from WriteToCookieMonster(). If we want to cleanly remove this code though, I am not sure how to proceed. It seems that a lot of this code is now useless and probably dead or rotting since the migration to WKWebView. Because then WriteToCookieMonster() no longer makes sense (because its only purpose was to set the cookies). If you do this recursively and remove WriteToCookieMonster(), then the problem repeats with FlushStore(), and the rabbit hole can go very deep... I don't recommend going down there as part of your work.
On 2017/06/02 13:09:28, droger wrote: > On 2017/06/01 15:21:53, droger wrote: > > On 2017/05/31 15:14:58, Randy Smith (Not in Mondays) wrote: > > > What kind of timing is that likely to happen on? I'm debating how I can > move > > > forward on this CL while you're doing that. > > > > I'll look at this tomorrow, if I can remember to bring my mac laptop in the > > office. > > Sorry for the delay. I managed to make a fresh build for iOS and put breakpoints > in SetAllCookiesAsync(). > > I could only have this code called from the signin flow, but this flow does not > actually care about actually saving cookies. > I did not see the function fire as a result from WKWebView loading or saving > cookies. > > So I think it is fine to break the behavior, or even to remove the function. > Feel free to remove the call to SetAllCookiesAsync() from > WriteToCookieMonster(). > > If we want to cleanly remove this code though, I am not sure how to proceed. It > seems that a lot of this code is now useless and probably dead or rotting since > the migration to WKWebView. > Because then WriteToCookieMonster() no longer makes sense (because its only > purpose was to set the cookies). > If you do this recursively and remove WriteToCookieMonster(), then the problem > repeats with FlushStore(), and the rabbit hole can go very deep... I don't > recommend going down there as part of your work. David: It feels really funny to just remove the function in WriteToCookieMonster() that does the writing :-}. I think I agree with you I don't want to go down the rabbit hole of doing the full code removal though--I'm already pretty deep in one (a couple?) of my own. So what I think I'm going to do is assume based on your response that it doesn't matter if I make minor variations in behavior for SetAllCookiesAsync(), and feel free to do such changes if it helps the refactor I'm doing. I'll keep the basic functionality (setting all the cookies), though. Let me know if you see any problems with that.
On 2017/06/09 02:20:04, Randy Smith (Not in Mondays) wrote: > On 2017/06/02 13:09:28, droger wrote: > > On 2017/06/01 15:21:53, droger wrote: > > > On 2017/05/31 15:14:58, Randy Smith (Not in Mondays) wrote: > > > > What kind of timing is that likely to happen on? I'm debating how I can > > move > > > > forward on this CL while you're doing that. > > > > > > I'll look at this tomorrow, if I can remember to bring my mac laptop in the > > > office. > > > > Sorry for the delay. I managed to make a fresh build for iOS and put > breakpoints > > in SetAllCookiesAsync(). > > > > I could only have this code called from the signin flow, but this flow does > not > > actually care about actually saving cookies. > > I did not see the function fire as a result from WKWebView loading or saving > > cookies. > > > > So I think it is fine to break the behavior, or even to remove the function. > > Feel free to remove the call to SetAllCookiesAsync() from > > WriteToCookieMonster(). > > > > If we want to cleanly remove this code though, I am not sure how to proceed. > It > > seems that a lot of this code is now useless and probably dead or rotting > since > > the migration to WKWebView. > > Because then WriteToCookieMonster() no longer makes sense (because its only > > purpose was to set the cookies). > > If you do this recursively and remove WriteToCookieMonster(), then the problem > > repeats with FlushStore(), and the rabbit hole can go very deep... I don't > > recommend going down there as part of your work. > > David: It feels really funny to just remove the function in > WriteToCookieMonster() that does the writing :-}. I think I agree with you I > don't want to go down the rabbit hole of doing the full code removal though--I'm > already pretty deep in one (a couple?) of my own. > > So what I think I'm going to do is assume based on your response that it doesn't > matter if I make minor variations in behavior for SetAllCookiesAsync(), and feel > free to do such changes if it helps the refactor I'm doing. I'll keep the basic > functionality (setting all the cookies), though. Let me know if you see any > problems with that. I'm fine with this.
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
Ok, I think this CL is once again ready for review. mmenke, droger: PTAL? Matt: This obviously has several dependent CLs which are on your queue; you're welcome to put off dealing with it until we resolve the underlying CLs, http://codereview.chromium.org/2898953008 most saliently.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ios lgtm
https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:65: #include "base/time/time.h" nit: Not needed, already included in header. https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:949: this, base::MakeUnique<CanonicalCookie>(cookie), secure_source, Should this take a unique_ptr<CanonicalCookie> instead? Does anyone need to have their cookie and eat it too (Or at least feed it to the monster, too)? https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1874: GarbageCollect(creation_date, key); So we just accept a creation date from the consumer, and we garbage collect, just assuming it's the current time? That seems not great. https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:2362: "Cookie.CookieSourceScheme", 1, COOKIE_SOURCE_LAST_ENTRY - 1, Isn't this the histogram you renamed in the XML file? Is that supposed to be part of another CL? https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... net/cookies/cookie_monster_unittest.cc:2320: CanonicalCookie::Create(kUrl, "a=b", base::Time::Now(), CookieOptions())); Why is this change needed in this CL? https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_sto... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_sto... net/cookies/cookie_store_unittest.h:564: ASSERT_TRUE(++it == cookies.end()); Maybe instead ASSERT on the size (x3)? "ASSERT_EQ(1u, cookies.size());" at the top may give us a bit less info if we see two or more cookies unexpectedly, but think it makes the test much clearer. (Also, if we choose to go with checking iterators instead, this one can be an EXPECT_TRUE) https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_sto... net/cookies/cookie_store_unittest.h:567: // that excludes them and getting an empty result. Does this get us anything? We check it->IsHttpOnly() below, and I think that's all we check when running GetCookieListWithOptions, which has its own tests? If it does get us something, maybe move this below the www_foo_bar_ test? "the cookie" sounds to me like it's referring to the previously checked cookie.
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...
Comments incorporated; PTAL? https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:65: #include "base/time/time.h" On 2017/06/16 15:56:28, mmenke wrote: > nit: Not needed, already included in header. Done. https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:949: this, base::MakeUnique<CanonicalCookie>(cookie), secure_source, On 2017/06/16 15:56:28, mmenke wrote: > Should this take a unique_ptr<CanonicalCookie> instead? Does anyone need to > have their cookie and eat it too (Or at least feed it to the monster, too)? I think I'm going to say "Yeah, that makes sense", though it pushes a problem off until the next CL. This interface is being created for use by the Mojo interface, and I don't think I can get mojo to give me a std::unique_ptr<>, so I'm just going to have to copy it at that level. But maybe I can find a way around that when I'm focussing on the mojo CL, and I don't think there's any gain even if I can't in doing the copy here instead of there. Done. https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1874: GarbageCollect(creation_date, key); On 2017/06/16 15:56:28, mmenke wrote: > So we just accept a creation date from the consumer, and we garbage collect, > just assuming it's the current time? That seems not great. Good point, but I think it's currently existing behavior: SetCookieWithDetails -> SetCanonicalCookie -> GarbageCollect()? https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:2362: "Cookie.CookieSourceScheme", 1, COOKIE_SOURCE_LAST_ENTRY - 1, On 2017/06/16 15:56:28, mmenke wrote: > Isn't this the histogram you renamed in the XML file? Is that supposed to be > part of another CL? Whoops. Removed renaming from the xml file. https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_mon... net/cookies/cookie_monster_unittest.cc:2320: CanonicalCookie::Create(kUrl, "a=b", base::Time::Now(), CookieOptions())); On 2017/06/16 15:56:28, mmenke wrote: > Why is this change needed in this CL? Because I added a DCHECK() in CanonicalCookie::Create() that creation time was not null. That combination of changes isn't strictly needed in this CL, but I was refining the specification of creation time (because I wanted to allow consumers to pass in a null creation time and have it set on insertion for the new method so that the new method didn't result in duplicate creation time in the backing store) and I took the attitude that if a routine specifically took creation time as one of a small number of arguments, the caller should actually set it. I can pull this change (or split it out into a separate CL, but it's pretty small) if you want, but I called out somewhere else that CC::Create() does depend on a non-null creation time, so I think there's some value in forcing it to be non null by spec. This, obviously, doesn't do jack for consumers manually setting the creation time on two cookies to be the same; that'll mess up the store. Existing problem, but one that continues to bother me :-{. https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_sto... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_sto... net/cookies/cookie_store_unittest.h:564: ASSERT_TRUE(++it == cookies.end()); On 2017/06/16 15:56:28, mmenke wrote: > Maybe instead ASSERT on the size (x3)? > > "ASSERT_EQ(1u, cookies.size());" at the top may give us a bit less info if we > see two or more cookies unexpectedly, but think it makes the test much clearer. > > (Also, if we choose to go with checking iterators instead, this one can be an > EXPECT_TRUE) Done. (For context, this test was copied&modified from SetCookieWithDetailsAsync.) https://codereview.chromium.org/2882063002/diff/240001/net/cookies/cookie_sto... net/cookies/cookie_store_unittest.h:567: // that excludes them and getting an empty result. On 2017/06/16 15:56:28, mmenke wrote: > Does this get us anything? We check it->IsHttpOnly() below, and I think that's > all we check when running GetCookieListWithOptions, which has its own tests? > > If it does get us something, maybe move this below the www_foo_bar_ test? "the > cookie" sounds to me like it's referring to the previously checked cookie. Done.
LGTM!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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...
rdsmith@chromium.org changed reviewers: + alexclarke@chromium.org, rdevlin.cronin@chromium.org, sgurun@chromium.org
Thanks, Matt & David! Now seeking owners stamps: * android_webview: sgurun@ * chrome/browser/extensions/api/cookies: rdevlin.cronin * headless: alexclarke@ PTAL?
chrome/browser/extensions/api/cookies/cookies_unittest.cc lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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.
headless/ LGTM
On 2017/06/19 15:37:31, alex clarke wrote: > headless/ LGTM aw lgtm
The CQ bit was checked by rdsmith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, mmenke@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2882063002/#ps300001 (title: "Fix AW cookie store wrapper.")
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
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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": 300001, "attempt_start_ts": 1498060500644750,
"parent_rev": "84dc05fd513e3c4e7aa64f5dba954f8d0a641939", "commit_rev":
"a6ce44474ee162592d04a31cf0b243c31a53f57d"}
Message was sent while issue was closed.
Description was changed from ========== Add a SetCanonicalCookie method for CookieMonster. This includes some refactoring of creation time defaulting, as well as a histogram revision bump because the information about whether a cookie is being set based on a null URL is no longer available at the point of histogram creation. BUG=721395, 723734 R=mmenke@chromium.org ========== to ========== Add a SetCanonicalCookie method for CookieMonster. This includes some refactoring of creation time defaulting, as well as a histogram revision bump because the information about whether a cookie is being set based on a null URL is no longer available at the point of histogram creation. BUG=721395, 723734 R=mmenke@chromium.org Review-Url: https://codereview.chromium.org/2882063002 Cr-Commit-Position: refs/heads/master@{#481227} Committed: https://chromium.googlesource.com/chromium/src/+/a6ce44474ee162592d04a31cf0b2... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/a6ce44474ee162592d04a31cf0b2... |
