Bug 82830 – [BlackBerry] Parsed Cookie's m_hasDefaultDomain is not needed.

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Jason Liu]

We don't need to check domain's dot in shouldRejectForSecurityReason since it is ensured in CookieParser::parseOneCookie.
So m_hasDefaultDomain is not needed.

[Jason Liu]

Created attachment 134988 [details]
Patch

[Rob Buis]

Comment on attachment 134988 [details]
Patch

Maybe Konrad can do an internal review first.

[Joe Mason]

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?

[Rob Buis]

Comment on attachment 134988 [details]
Patch

r- based on Joe's remarks.

[Jason Liu]

(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.

[Jason Liu]

 
> > 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.

[Jason Liu]

Created attachment 135973 [details]
Patch

[Jason Liu]

Created attachment 136190 [details]
Patch

[Joe Mason]

(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.)

[Jason Liu]

Created attachment 136411 [details]
Patch

[Jason Liu]

(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.

[George Staikos]

Comment on attachment 136411 [details]
Patch

Looks ok to me now

[WebKit Review Bot]

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

[Jason Liu]

Created attachment 137817 [details]
Patch

[WebKit Review Bot]

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

[Jason Liu]

Created attachment 137833 [details]
Patch

[WebKit Review Bot]

Comment on attachment 137833 [details]
Patch

Clearing flags on attachment: 137833

Committed r114608: <http://trac.webkit.org/changeset/114608>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Takashi Toyoshima]

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

[Jason Liu]

(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.

[Takashi Toyoshima]

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.

[Jason Liu]

(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.

[Takashi Toyoshima]

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

[Jason Liu]

(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.

[Takashi Toyoshima]

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.