We don't need to check domain's dot in shouldRejectForSecurityReason since it is ensured in CookieParser::parseOneCookie. So m_hasDefaultDomain is not needed.
Created attachment 134988 [details] Patch
Comment on attachment 134988 [details] Patch Maybe Konrad can do an internal review first.
I think instead of removing the check entirely it should be replaced with an ASSERT. Also, is there a layouttest that makes sure malformed domains are rejected?
Comment on attachment 134988 [details] Patch r- based on Joe's remarks.
(In reply to comment #3) > I think instead of removing the check entirely it should be replaced with an ASSERT. If domain is set to host as a default, it will not begin with ".". We don't need to check for this kind of domain. So I am not sure if we should add ASSERT to check. And It is hard to add ASSERT since we don't need to check all the time. More other codes are needed for adding ASSERT. > Also, is there a layouttest that makes sure malformed domains are rejected? There is not a test as you said. I will try to write this test case.
> > Also, is there a layouttest that makes sure malformed domains are rejected? > > There is not a test as you said. I will try to write this test case. I think this test case is not needed.
Created attachment 135973 [details] Patch
Created attachment 136190 [details] Patch
(In reply to comment #6) > > > Also, is there a layouttest that makes sure malformed domains are rejected? > > > > There is not a test as you said. I will try to write this test case. > > I think this test case is not needed. Why not? If cookies are supposed to be rejecting malformed domains, there should be a test to make sure that refactoring doesn't break that. (It wouldn't surprise me if that's already covered in a layouttest, though.)
Created attachment 136411 [details] Patch
(In reply to comment #9) > (In reply to comment #6) > > > > Also, is there a layouttest that makes sure malformed domains are rejected? > > > > > > There is not a test as you said. I will try to write this test case. > > > > I think this test case is not needed. > > Why not? If cookies are supposed to be rejecting malformed domains, there should be a test to make sure that refactoring doesn't break that. (It wouldn't surprise me if that's already covered in a layouttest, though.) Hi, Joe I can't find a test we need. So make a new one. Please have a look at my new patch. Thanks.
Comment on attachment 136411 [details] Patch Looks ok to me now
Comment on attachment 136411 [details] Patch Rejecting attachment 136411 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: Source/WebCore/platform/blackberry/ParsedCookie.h.rej patching file LayoutTests/http/tests/security/cookies/cookies-wrong-domain-rejected-expected.txt patching file LayoutTests/http/tests/security/cookies/cookies-wrong-domain-rejected-result.php patching file LayoutTests/http/tests/security/cookies/cookies-wrong-domain-rejected.php Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'George Sta..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12426197
Created attachment 137817 [details] Patch
Comment on attachment 137817 [details] Patch Rejecting attachment 137817 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12428470
Created attachment 137833 [details] Patch
Comment on attachment 137833 [details] Patch Clearing flags on attachment: 137833 Committed r114608: <http://trac.webkit.org/changeset/114608>
All reviewed patches have been landed. Closing bug.
Hi Jason, I'm afraid that you miss one another expected.txt. I added it as unreviewed change. Please check it. http://trac.webkit.org/changeset/114619
(In reply to comment #19) > Hi Jason, > > I'm afraid that you miss one another expected.txt. > I added it as unreviewed change. > Please check it. > http://trac.webkit.org/changeset/114619 Hi Takashi, Would you please attach it? I can't load http://trac.webkit.org/changeset/114619. Thanks a lot.
I added LayoutTests/http/tests/security/cookies/cookies-wrong-domain-rejected-result-expected.txt. It is the same content with LayoutTests/http/tests/security/cookies/cookies-wrong-domain-rejected-expected.txt like; PASS: User agent rejected the cookie with a wrong domain.
(In reply to comment #21) > I added LayoutTests/http/tests/security/cookies/cookies-wrong-domain-rejected-result-expected.txt. > It is the same content with LayoutTests/http/tests/security/cookies/cookies-wrong-domain-rejected-expected.txt like; > > PASS: User agent rejected the cookie with a wrong domain. Why add this text? I can run DRT with cookies-wrong-domain-rejected-expected.txt successfully.
I'm sorry if I misunderstand on tests written in php. I understand that you added two tests, 1. cookies-wrong-domain-rejected.php 2. cookies-wrong-domain-rejected-result.php And the change contains expected.txt for only 1. I added expected.txt for 2 because garden-o-matic detects it. See, also http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fsecurity%2Fcookies%2Fcookies-wrong-domain-rejected-result.php
(In reply to comment #23) > I'm sorry if I misunderstand on tests written in php. > > I understand that you added two tests, > 1. cookies-wrong-domain-rejected.php > 2. cookies-wrong-domain-rejected-result.php > > And the change contains expected.txt for only 1. > > I added expected.txt for 2 because garden-o-matic detects it. > > See, also > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fsecurity%2Fcookies%2Fcookies-wrong-domain-rejected-result.php You are welcome. I can't see anything in http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fsecurity%2Fcookies%2Fcookies-wrong-domain-rejected-result.php. Thanks for your information.
OK, this is our error log for the test. > 00:16:00.345 3136 http/tests/security/cookies/cookies-wrong-domain-rejected-result.php -> unexpected no expected result found > 00:16:00.345 3156 worker/0 http/tests/security/cookies/cookies-wrong-domain-rejected-result.php failed: You can see the whole log at http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/24024/steps/webkit_tests/logs/stdio This is bot status before I added the file. http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/24024 And this is the one after I added. http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/24025 I hope you can see these URLs.