Bug 82830 - [BlackBerry] Parsed Cookie's m_hasDefaultDomain is not needed.
Summary: [BlackBerry] Parsed Cookie's m_hasDefaultDomain is not needed.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-31 03:10 PDT by Jason Liu
Modified: 2012-04-19 03:22 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.70 KB, patch)
2012-04-01 01:07 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (5.73 KB, patch)
2012-04-05 20:16 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (6.47 KB, patch)
2012-04-08 23:56 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (8.37 KB, patch)
2012-04-10 01:20 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (9.25 KB, patch)
2012-04-18 19:21 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (9.25 KB, patch)
2012-04-18 20:49 PDT, Jason Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Liu 2012-03-31 03:10:56 PDT
We don't need to check domain's dot in shouldRejectForSecurityReason since it is ensured in CookieParser::parseOneCookie.
So m_hasDefaultDomain is not needed.
Comment 1 Jason Liu 2012-04-01 01:07:30 PDT
Created attachment 134988 [details]
Patch
Comment 2 Rob Buis 2012-04-01 05:58:24 PDT
Comment on attachment 134988 [details]
Patch

Maybe Konrad can do an internal review first.
Comment 3 Joe Mason 2012-04-01 11:35:24 PDT
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 4 Rob Buis 2012-04-01 13:31:58 PDT
Comment on attachment 134988 [details]
Patch

r- based on Joe's remarks.
Comment 5 Jason Liu 2012-04-04 19:50:43 PDT
(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.
Comment 6 Jason Liu 2012-04-05 19:56:35 PDT
 
> > 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.
Comment 7 Jason Liu 2012-04-05 20:16:45 PDT
Created attachment 135973 [details]
Patch
Comment 8 Jason Liu 2012-04-08 23:56:42 PDT
Created attachment 136190 [details]
Patch
Comment 9 Joe Mason 2012-04-09 09:13:44 PDT
(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.)
Comment 10 Jason Liu 2012-04-10 01:20:57 PDT
Created attachment 136411 [details]
Patch
Comment 11 Jason Liu 2012-04-10 01:51:02 PDT
(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 12 George Staikos 2012-04-18 02:27:40 PDT
Comment on attachment 136411 [details]
Patch

Looks ok to me now
Comment 13 WebKit Review Bot 2012-04-18 02:29:45 PDT
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
Comment 14 Jason Liu 2012-04-18 19:21:37 PDT
Created attachment 137817 [details]
Patch
Comment 15 WebKit Review Bot 2012-04-18 20:24:35 PDT
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
Comment 16 Jason Liu 2012-04-18 20:49:58 PDT
Created attachment 137833 [details]
Patch
Comment 17 WebKit Review Bot 2012-04-18 22:22:08 PDT
Comment on attachment 137833 [details]
Patch

Clearing flags on attachment: 137833

Committed r114608: <http://trac.webkit.org/changeset/114608>
Comment 18 WebKit Review Bot 2012-04-18 22:22:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Takashi Toyoshima 2012-04-19 00:09:52 PDT
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
Comment 20 Jason Liu 2012-04-19 00:16:46 PDT
(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.
Comment 21 Takashi Toyoshima 2012-04-19 01:38:03 PDT
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.
Comment 22 Jason Liu 2012-04-19 01:57:24 PDT
(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.
Comment 23 Takashi Toyoshima 2012-04-19 02:23:56 PDT
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
Comment 24 Jason Liu 2012-04-19 02:38:19 PDT
(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.
Comment 25 Takashi Toyoshima 2012-04-19 03:22:52 PDT
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.