Deleting cookies by setting the expires attribute on them with an empty value...

Deleting cookies by setting the expires attribute on them with an empty value would not work in ChromeFrame
with the host network stack enabled. When we receive a response in the host browser (IE) we send over the
response headers which include the Set-Cookie header and a list of cookies retreived via the InternetGetCookie
API. We call this API to retrieve the persistent cookies and send them over to Chrome.

However this API returns session cookies as well as persistent cookies. There is no documented way to return
only persistent cookies from IE. As a result we would end up setting duplicate cookies in Chrome, which caused
this issu.e. To workaround this issue when we receive the response in the url request automation job which
handles ChromeFrame network requests, we strip out duplicate cookies sent via InternetGetCookie.

When a script deletes a cookie we now handle it correctly in IE and set the data to an empty string. However
this does not delete the cookie. When such cookies show up in Chrome, we strip them out as well.

Fixes bug http://code.google.com/p/chromium/issues/detail?id=30786

The changes to chrome_frame_npapi.cc/.h are to move the NPAPI functions to the chrome_frame namespace as they
conflict with similar functions in NACL.

Added the DeleteCookie function to the CookieStore interface, which I think missed out by oversight.

Bug=30786
Test=Covered by ChromeFrame unit tests. I also added a unit test to test the newly added
     URLRequestAutomationJob::IsCookiePresentInCookieHeader function


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35769

[amit, 2010-01-07 15:36:13]

Thanks for digging into this bug and fixing it.
LGTM with a few comments below.

http://codereview.chromium.org/518054/diff/1009/4027
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/518054/diff/1009/4027#newcode260
chrome/browser/automation/url_request_automation_job.cc:260: 
nit: remove empty line

http://codereview.chromium.org/518054/diff/1009/4027#newcode261
chrome/browser/automation/url_request_automation_job.cc:261:
std::vector<std::string> response_cookies;
making this into a map or a set would simplify (or eliminate)
IsCookiePresentInCookieHeader.

http://codereview.chromium.org/518054/diff/1009/4027#newcode297
chrome/browser/automation/url_request_automation_job.cc:297:
TrimWhitespace(cookie_string, TRIM_LEADING, &cookie_string);
maybe we should trim from both ends after cookie_string.find succeeds.

http://codereview.chromium.org/518054/diff/1009/4019
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/518054/diff/1009/4019#newcode530
chrome_frame/chrome_frame_activex_base.h:530: net::CookieMonster::ParsedCookie
parsed_cookie = cookie;
I think we should have a separate DeleteCookie IPC instead of propagating IE's
ugliness.

http://codereview.chromium.org/518054/diff/1009/4018
File chrome_frame/chrome_frame_npapi.h (right):

http://codereview.chromium.org/518054/diff/1009/4018#newcode347
chrome_frame/chrome_frame_npapi.h:347: namespace chrome_frame {
nice, now it would be even better of we move these into a separate
source/header.

http://codereview.chromium.org/518054/diff/1009/4020
File chrome_frame/test/chrome_frame_unittests.cc (right):

http://codereview.chromium.org/518054/diff/1009/4020#newcode1766
chrome_frame/test/chrome_frame_unittests.cc:1766: ASSERT_TRUE(
nit: fit in less no of lines, I am not sure if this is more readable

http://codereview.chromium.org/518054/diff/1009/4020#newcode1773
chrome_frame/test/chrome_frame_unittests.cc:1773: ASSERT_TRUE(
nit: should fit in a single line

http://codereview.chromium.org/518054/diff/1009/4021
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/518054/diff/1009/4021#newcode366
chrome_frame/test/html_util_unittests.cc:366: TEST(HttpCookieTest,
IdentifyDuplicateCookieTest) {
nice test, now jsut to be sure, do we need to add 'domain', 'expires' to the
test cases?

[ananta, 2010-01-07 19:35:25]

http://codereview.chromium.org/518054/diff/1009/4027
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/518054/diff/1009/4027#newcode260
chrome/browser/automation/url_request_automation_job.cc:260: 
On 2010/01/07 15:36:13, amit wrote:
> nit: remove empty line

Done.

http://codereview.chromium.org/518054/diff/1009/4027#newcode261
chrome/browser/automation/url_request_automation_job.cc:261:
std::vector<std::string> response_cookies;
On 2010/01/07 15:36:13, amit wrote:
> making this into a map or a set would simplify (or eliminate)
> IsCookiePresentInCookieHeader. 

I did think about changing this. However making it a map would have its own
complications like we would need to parse the cookie string again to get the
key, which ideally would be the name. However vagaries with cookie formats,
cause the name to be empty at times. An example would be a cookie without any
attributes. I decided to leave this as is for now.

http://codereview.chromium.org/518054/diff/1009/4027#newcode297
chrome/browser/automation/url_request_automation_job.cc:297:
TrimWhitespace(cookie_string, TRIM_LEADING, &cookie_string);
On 2010/01/07 15:36:13, amit wrote:
> maybe we should trim from both ends after cookie_string.find succeeds.

Done.

http://codereview.chromium.org/518054/diff/1009/4019
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/518054/diff/1009/4019#newcode530
chrome_frame/chrome_frame_activex_base.h:530: net::CookieMonster::ParsedCookie
parsed_cookie = cookie;
On 2010/01/07 15:36:13, amit wrote:
> I think we should have a separate DeleteCookie IPC instead of propagating IE's
> ugliness.

Unfortunately javascript cookie deletion occurs by means of a SetCookie call. We
do have a DeleteCookie interface in the webkit API. I don't know under what
scenarios that is called.

http://codereview.chromium.org/518054/diff/1009/4018
File chrome_frame/chrome_frame_npapi.h (right):

http://codereview.chromium.org/518054/diff/1009/4018#newcode347
chrome_frame/chrome_frame_npapi.h:347: namespace chrome_frame {
On 2010/01/07 15:36:13, amit wrote:
> nice, now it would be even better of we move these into a separate
> source/header.

Done.

http://codereview.chromium.org/518054/diff/1009/4020
File chrome_frame/test/chrome_frame_unittests.cc (right):

http://codereview.chromium.org/518054/diff/1009/4020#newcode1766
chrome_frame/test/chrome_frame_unittests.cc:1766: ASSERT_TRUE(
On 2010/01/07 15:36:13, amit wrote:
> nit: fit in less no of lines, I am not sure if this is more readable

Done.

http://codereview.chromium.org/518054/diff/1009/4020#newcode1773
chrome_frame/test/chrome_frame_unittests.cc:1773: ASSERT_TRUE(
On 2010/01/07 15:36:13, amit wrote:
> nit: should fit in a single line

Done.

http://codereview.chromium.org/518054/diff/1009/4021
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/518054/diff/1009/4021#newcode366
chrome_frame/test/html_util_unittests.cc:366: TEST(HttpCookieTest,
IdentifyDuplicateCookieTest) {
On 2010/01/07 15:36:13, amit wrote:
> nice test, now jsut to be sure, do we need to add 'domain', 'expires' to the
> test cases?
We could. We only compare the names of the cookies though. Additional attributes
would be ignored.

[amit, 2010-01-07 21:28:25]

OK, lgtm++

On Thu, Jan 7, 2010 at 11:35 AM,  <ananta@chromium.org> wrote:
>
> http://codereview.chromium.org/518054/diff/1009/4027
> File chrome/browser/automation/url_request_automation_job.cc (right):
>
> http://codereview.chromium.org/518054/diff/1009/4027#newcode260
> chrome/browser/automation/url_request_automation_job.cc:260:
> On 2010/01/07 15:36:13, amit wrote:
>>
>> nit: remove empty line
>
> Done.
>
> http://codereview.chromium.org/518054/diff/1009/4027#newcode261
> chrome/browser/automation/url_request_automation_job.cc:261:
> std::vector<std::string> response_cookies;
> On 2010/01/07 15:36:13, amit wrote:
>>
>> making this into a map or a set would simplify (or eliminate)
>> IsCookiePresentInCookieHeader.
>
> I did think about changing this. However making it a map would have its
> own complications like we would need to parse the cookie string again to
> get the key, which ideally would be the name. However vagaries with
> cookie formats, cause the name to be empty at times. An example would be
> a cookie without any attributes. I decided to leave this as is for now.
>
> http://codereview.chromium.org/518054/diff/1009/4027#newcode297
> chrome/browser/automation/url_request_automation_job.cc:297:
> TrimWhitespace(cookie_string, TRIM_LEADING, &cookie_string);
> On 2010/01/07 15:36:13, amit wrote:
>>
>> maybe we should trim from both ends after cookie_string.find succeeds.
>
> Done.
>
> http://codereview.chromium.org/518054/diff/1009/4019
> File chrome_frame/chrome_frame_activex_base.h (right):
>
> http://codereview.chromium.org/518054/diff/1009/4019#newcode530
> chrome_frame/chrome_frame_activex_base.h:530:
> net::CookieMonster::ParsedCookie parsed_cookie = cookie;
> On 2010/01/07 15:36:13, amit wrote:
>>
>> I think we should have a separate DeleteCookie IPC instead of
>
> propagating IE's
>>
>> ugliness.
>
> Unfortunately javascript cookie deletion occurs by means of a SetCookie
> call. We do have a DeleteCookie interface in the webkit API. I don't
> know under what scenarios that is called.
>
> http://codereview.chromium.org/518054/diff/1009/4018
> File chrome_frame/chrome_frame_npapi.h (right):
>
> http://codereview.chromium.org/518054/diff/1009/4018#newcode347
> chrome_frame/chrome_frame_npapi.h:347: namespace chrome_frame {
> On 2010/01/07 15:36:13, amit wrote:
>>
>> nice, now it would be even better of we move these into a separate
>> source/header.
>
> Done.
>
> http://codereview.chromium.org/518054/diff/1009/4020
> File chrome_frame/test/chrome_frame_unittests.cc (right):
>
> http://codereview.chromium.org/518054/diff/1009/4020#newcode1766
> chrome_frame/test/chrome_frame_unittests.cc:1766: ASSERT_TRUE(
> On 2010/01/07 15:36:13, amit wrote:
>>
>> nit: fit in less no of lines, I am not sure if this is more readable
>
> Done.
>
> http://codereview.chromium.org/518054/diff/1009/4020#newcode1773
> chrome_frame/test/chrome_frame_unittests.cc:1773: ASSERT_TRUE(
> On 2010/01/07 15:36:13, amit wrote:
>>
>> nit: should fit in a single line
>
> Done.
>
> http://codereview.chromium.org/518054/diff/1009/4021
> File chrome_frame/test/html_util_unittests.cc (right):
>
> http://codereview.chromium.org/518054/diff/1009/4021#newcode366
> chrome_frame/test/html_util_unittests.cc:366: TEST(HttpCookieTest,
> IdentifyDuplicateCookieTest) {
> On 2010/01/07 15:36:13, amit wrote:
>>
>> nice test, now jsut to be sure, do we need to add 'domain', 'expires'
>
> to the
>>
>> test cases?
>
> We could. We only compare the names of the cookies though. Additional
> attributes would be ignored.
>
> http://codereview.chromium.org/518054
>