Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1746303002:
Replace std::string::find with base::StartsWith when comparing cookie paths. (Closed)
DescriptionReplace std::string::find with base::StartsWith when comparing cookie paths.
The old code was correct, and the performance benefits are minimal, but
"path1.find(path2) != 0" is a bit obscure.
Also switch DeleteCookie to CanonicalCookie::IsOnPath to compare cookies,
to match normal cookie semantics.
BUG=583482
Committed: https://crrev.com/23b6717eac3a002b02aa9951d62bec2b641fe947
Cr-Commit-Position: refs/heads/master@{#379092}
Patch Set 1 #Patch Set 2 : Fix inverted check #Patch Set 3 : Add test #Patch Set 4 : Fix #Patch Set 5 : Remove new test case #
Total comments: 8
Messages
Total messages: 23 (10 generated)
Description was changed from ========== Replace std::string::find with base::StartsWith when compare cookie paths. The old code was correct, and the performance benefits are minimal, but "path1.find(path2) != 0" is a bit obscure. Also switch DeleteCookie to CanonicalCookie::IsOnPath to compare cookies, to match normal cookie semantics. BUG=583482 ========== to ========== Replace std::string::find with base::StartsWith when comparing cookie paths. The old code was correct, and the performance benefits are minimal, but "path1.find(path2) != 0" is a bit obscure. Also switch DeleteCookie to CanonicalCookie::IsOnPath to compare cookies, to match normal cookie semantics. BUG=583482 ==========
Patchset #4 (id:60001) has been deleted
mmenke@chromium.org changed reviewers: + mkwst@chromium.org
https://codereview.chromium.org/1746303002/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/1746303002/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:355: if (!base::StartsWith(url_path, path_, base::CompareCase::SENSITIVE)) Does this really mean that you can set cookies with paths "bar" and "bar/"...And the first would not be returned for "bar", but both would be returned for "bar/"? https://codereview.chromium.org/1746303002/diff/100001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1746303002/diff/100001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1217: if (!cookie->IsOnPath(url.path())) Just happened to notice this bug when cleaning up this code. https://codereview.chromium.org/1746303002/diff/100001/net/cookies/cookie_sto... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/1746303002/diff/100001/net/cookies/cookie_sto... net/cookies/cookie_store_unittest.h:1311: "A=A3; path=/bar")); I tried adding a "path=/fo" test, but iOS apparently has different matching behavior here (Applies to getting cookies, too, I think)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1746303002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1746303002/100001
On 2016/03/03 18:08:54, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1746303002/100001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1746303002/100001 Oops...Sorry, try do that myself before sending out a request for review.
And I should submit comments instead of submitting the CL to the CQ. :/ https://codereview.chromium.org/1746303002/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/1746303002/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:355: if (!base::StartsWith(url_path, path_, base::CompareCase::SENSITIVE)) On 2016/03/02 at 20:02:01, mmenke wrote: > Does this really mean that you can set cookies with paths "bar" and "bar/"...And the first would not be returned for "bar", but both would be returned for "bar/"? That sounds wrong. The path matching algorithm at https://tools.ietf.org/html/rfc6265#section-5.1.4 says 'The cookie-path is a prefix of the request-path, and the first character of the request-path that is not included in the cookie-path is a %x2F ("/") character.' `path=/bar` should match `/bar/foo` and `/bar`. https://codereview.chromium.org/1746303002/diff/100001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1746303002/diff/100001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:1217: if (!cookie->IsOnPath(url.path())) On 2016/03/02 at 20:02:01, mmenke wrote: > Just happened to notice this bug when cleaning up this code. \o/ https://codereview.chromium.org/1746303002/diff/100001/net/cookies/cookie_sto... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/1746303002/diff/100001/net/cookies/cookie_sto... net/cookies/cookie_store_unittest.h:1311: "A=A3; path=/bar")); On 2016/03/02 at 20:02:01, mmenke wrote: > I tried adding a "path=/fo" test, but iOS apparently has different matching behavior here (Applies to getting cookies, too, I think) What? What does iOS do?
https://codereview.chromium.org/1746303002/diff/100001/net/cookies/canonical_... File net/cookies/canonical_cookie.cc (right): https://codereview.chromium.org/1746303002/diff/100001/net/cookies/canonical_... net/cookies/canonical_cookie.cc:355: if (!base::StartsWith(url_path, path_, base::CompareCase::SENSITIVE)) On 2016/03/03 18:56:28, Mike West wrote: > On 2016/03/02 at 20:02:01, mmenke wrote: > > Does this really mean that you can set cookies with paths "bar" and > "bar/"...And the first would not be returned for "bar", but both would be > returned for "bar/"? > > That sounds wrong. The path matching algorithm at > https://tools.ietf.org/html/rfc6265#section-5.1.4 says 'The cookie-path is a > prefix of the request-path, and the first character of the request-path that is > not included in the cookie-path is a %x2F ("/") character.' `path=/bar` should > match `/bar/foo` and `/bar`. Sorry, that should be "/bar/" and "/bar"... and was meant to go below line 367. (Notice the path_.back() check, in particular) https://codereview.chromium.org/1746303002/diff/100001/net/cookies/cookie_sto... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/1746303002/diff/100001/net/cookies/cookie_sto... net/cookies/cookie_store_unittest.h:1311: "A=A3; path=/bar")); On 2016/03/03 18:56:28, Mike West wrote: > On 2016/03/02 at 20:02:01, mmenke wrote: > > I tried adding a "path=/fo" test, but iOS apparently has different matching > behavior here (Applies to getting cookies, too, I think) > > What? What does iOS do? iOS seems to return (Or at least delete, think return, too, though) a cookie set on "/fo" when passed a path of "/foo"
Based on our discussion, I think this is an accurate representation of what the spec requires (except for the iOS thing, which is both nuts and intractable), whether or not it makes sense. Whether or not _any of it_ makes sense. :) LGTM.
On 2016/03/03 19:32:52, Mike West wrote: > Based on our discussion, I think this is an accurate representation of what the > spec requires (except for the iOS thing, which is both nuts and intractable), > whether or not it makes sense. Whether or not _any of it_ makes sense. :) > > LGTM. I'm sure it all made sense to whoever was implementing (incorrectly or not) or speccing out (on narcotics or not) any part of it at one time. Thanks!
The CQ bit was unchecked by mmenke@chromium.org
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1746303002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1746303002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1746303002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1746303002/100001
Message was sent while issue was closed.
Description was changed from ========== Replace std::string::find with base::StartsWith when comparing cookie paths. The old code was correct, and the performance benefits are minimal, but "path1.find(path2) != 0" is a bit obscure. Also switch DeleteCookie to CanonicalCookie::IsOnPath to compare cookies, to match normal cookie semantics. BUG=583482 ========== to ========== Replace std::string::find with base::StartsWith when comparing cookie paths. The old code was correct, and the performance benefits are minimal, but "path1.find(path2) != 0" is a bit obscure. Also switch DeleteCookie to CanonicalCookie::IsOnPath to compare cookies, to match normal cookie semantics. BUG=583482 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Replace std::string::find with base::StartsWith when comparing cookie paths. The old code was correct, and the performance benefits are minimal, but "path1.find(path2) != 0" is a bit obscure. Also switch DeleteCookie to CanonicalCookie::IsOnPath to compare cookies, to match normal cookie semantics. BUG=583482 ========== to ========== Replace std::string::find with base::StartsWith when comparing cookie paths. The old code was correct, and the performance benefits are minimal, but "path1.find(path2) != 0" is a bit obscure. Also switch DeleteCookie to CanonicalCookie::IsOnPath to compare cookies, to match normal cookie semantics. BUG=583482 Committed: https://crrev.com/23b6717eac3a002b02aa9951d62bec2b641fe947 Cr-Commit-Position: refs/heads/master@{#379092} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/23b6717eac3a002b02aa9951d62bec2b641fe947 Cr-Commit-Position: refs/heads/master@{#379092}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/1766613002/ by benwells@chromium.org. The reason for reverting is: This CL seems to have caused a cookie test to fail on the Mac valgrind bot. As it is a test failure, not memory error, being reported I think it is worth investigating. See https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%2.... Sample bot output: CookieMonsterTest.PersisentCookieStorageTest: ../../net/cookies/cookie_monster_unittest.cc:2371: Failure Value of: store->commands().size() Actual: 6 Expected: 5u Which is: 5. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
