| Summary: | [Curl] Fix priority issue with multiple cookies with different level of path. | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Basuke Suzuki <Basuke.Suzuki> | ||||||||||||||||||||||||||||
| Component: | Platform | Assignee: | Basuke Suzuki <Basuke.Suzuki> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | achristensen, Basuke.Suzuki, commit-queue, darin, ews-watchlist, galpeter, Hironori.Fujii, rniwa, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Basuke Suzuki
2018-09-24 11:12:13 PDT
Created attachment 350674 [details]
PATCH
Comment on attachment 350674 [details] PATCH Attachment 350674 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9335035 New failing tests: http/tests/cookies/cookie-with-path-multiple-level.html Created attachment 350680 [details]
Archive of layout-test-results from ews103 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 350674 [details] PATCH Attachment 350674 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9335347 New failing tests: http/tests/cookies/cookie-with-path-multiple-level.html Created attachment 350694 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 350674 [details] PATCH Attachment 350674 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9336254 New failing tests: http/tests/cookies/cookie-with-path-multiple-level.html Created attachment 350702 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 350674 [details] PATCH Attachment 350674 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9335832 New failing tests: http/tests/cookies/cookie-with-path-multiple-level.html Created attachment 350705 [details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Based on https://tools.ietf.org/html/rfc6265#section-5.4: > 2. The user agent SHOULD sort the cookie-list in the following > order: > > * Cookies with longer paths are listed before cookies with > shorter paths. > > * Among cookies that have equal-length path fields, cookies with > earlier creation-times are listed before cookies with later > creation-times. > > NOTE: Not all user agents sort the cookie-list in this order, but > this order reflects common practice when this document was > written, and, historically, there have been servers that > (erroneously) depended on this order. Comment on attachment 350674 [details] PATCH Attachment 350674 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9337179 New failing tests: http/tests/cookies/cookie-with-path-multiple-level.html Created attachment 350719 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 350773 [details]
PATCH
Fix cookie path.
Comment on attachment 350773 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=350773&action=review > LayoutTests/ChangeLog:10 > + * http/tests/cookies/resources/cookie-with-multiple-level/foo/get.php: Added. We already have many copies of echo-cookies.php, get-cookies.php, etc. > LayoutTests/ChangeLog:11 > + * http/tests/cookies/resources/cookie-with-multiple-level/foo/set.php: Added. We already have many copies of set-cookie.php or set-a-cookie.php. > LayoutTests/http/tests/cookies/cookie-with-path-multiple-level.html:9 > + This tests flakiness of cookie query of Curl port. This test is not specific to Curl. Comment on attachment 350773 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=350773&action=review >> LayoutTests/ChangeLog:10 >> + * http/tests/cookies/resources/cookie-with-multiple-level/foo/get.php: Added. > > We already have many copies of echo-cookies.php, get-cookies.php, etc. It is required to be in that path. It returns the cookie which sent to that path. >> LayoutTests/ChangeLog:11 >> + * http/tests/cookies/resources/cookie-with-multiple-level/foo/set.php: Added. > > We already have many copies of set-cookie.php or set-a-cookie.php. ditto. >> LayoutTests/http/tests/cookies/cookie-with-path-multiple-level.html:9 >> + This tests flakiness of cookie query of Curl port. > > This test is not specific to Curl. Okay. Comment on attachment 350773 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=350773&action=review > LayoutTests/ChangeLog:9 > + * http/tests/cookies/cookie-with-path-multiple-level.html: Added. This change has only a test case of path lenght of single cookie. Accoring to the spec, multiple cookies should be sorted in longer path and older creation-time order. https://tools.ietf.org/html/rfc6265#section-5.4 Can you create a test case to check them? name1=val1; name2=val2; name3=val3 >>> LayoutTests/ChangeLog:11 >>> + * http/tests/cookies/resources/cookie-with-multiple-level/foo/set.php: Added. >> >> We already have many copies of set-cookie.php or set-a-cookie.php. > > ditto. I think set.php doesn't need to placed in the nested directory. LayoutTests/http/tests/cookies/resources/setCookies.cgi can be used to set a arbitrary cookeis. I think you can use this with " path=/...". >>> LayoutTests/http/tests/cookies/cookie-with-path-multiple-level.html:9 >>> + This tests flakiness of cookie query of Curl port. >> >> This test is not specific to Curl. > > Okay. I think you should follow the style of other HTML files in this directory. I think 'description' should be placed immediately after <script>. Created attachment 351024 [details]
PATCH
- Added created date test. - Use setCookies.cgi Comment on attachment 351024 [details] PATCH Attachment 351024 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9374977 New failing tests: http/tests/cookies/cookie-with-multiple-level-path.html Created attachment 351033 [details]
Archive of layout-test-results from ews100 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 351024 [details] PATCH Attachment 351024 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9375173 New failing tests: http/tests/cookies/cookie-with-multiple-level-path.html Created attachment 351037 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 351024 [details] PATCH Attachment 351024 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9374954 New failing tests: http/tests/cookies/cookie-with-multiple-level-path.html Created attachment 351039 [details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
You greatly improved the test case. Thank you very much for working on it.
Some Mac port EWS bots are red.
> FAIL () => result should be foo=buz; bingo=bongo; foo=bar; secret=42. Was bingo=bongo; foo=buz; secret=42; foo=bar.
It seems that it sorts cookies in newer order.
Could you add a comment about other major browsers results?
(In reply to Fujii Hironori from comment #25) > You greatly improved the test case. Thank you very much for working on it. > > Some Mac port EWS bots are red. > > > FAIL () => result should be foo=buz; bingo=bongo; foo=bar; secret=42. Was bingo=bongo; foo=buz; secret=42; foo=bar. > > It seems that it sorts cookies in newer order. > > Could you add a comment about other major browsers results? Chrome PASS. I'll try on Firefox. But as RFC mentions, some browsers are not following that behavior and that is acceptable because the order of different cookie name is not critical. It differs from multi level path issues that even if the order of sort result caused by creation date is deffer, the parsed result in the context is almost always same in practical. In reality, I don't think we need this test. (In reply to Basuke Suzuki from comment #26) > But as RFC mentions, some browsers are not following that behavior and that > is acceptable because the order of different cookie name is not critical. It > differs from multi level path issues that even if the order of sort result > caused by creation date is deffer, the parsed result in the context is > almost always same in practical. > > In reality, I don't think we need this test. Do you mean you think test cases of both sort order conditions (path lenght and creation-time) are usesless? Or, creation-time test is usesless? If the latter case, I can agree on you. If the former case, I don't understand you. Could you elaborate? (In reply to Fujii Hironori from comment #27) > (In reply to Basuke Suzuki from comment #26) > > But as RFC mentions, some browsers are not following that behavior and that > > is acceptable because the order of different cookie name is not critical. It > > differs from multi level path issues that even if the order of sort result > > caused by creation date is deffer, the parsed result in the context is > > almost always same in practical. > > > > In reality, I don't think we need this test. > > Do you mean you think test cases of both sort order conditions (path lenght > and creation-time) are usesless? > Or, creation-time test is usesless? > > If the latter case, I can agree on you. > If the former case, I don't understand you. Could you elaborate? Of course latter. The path length sort order is the main issue this patch fixes on curl port. Comment on attachment 351024 [details] PATCH Attachment 351024 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9376985 New failing tests: http/tests/cookies/cookie-with-multiple-level-path.html Created attachment 351054 [details]
Archive of layout-test-results from ews201 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 351055 [details]
PATCH
Remove timestamp tests
Comment on attachment 351055 [details] PATCH Clearing flags on attachment: 351055 Committed r236593: <https://trac.webkit.org/changeset/236593> All reviewed patches have been landed. Closing bug. |