Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(199)

Issue 521072: Attempt 2 at landing this.... (Closed)

Created:
10 years, 11 months ago by ananta
Modified:
9 years, 6 months ago
Reviewers:
amit
CC:
chromium-reviews_googlegroups.com, brettw+cc_chromium.org, ben+cc_chromium.org, amit, jam, Paweł Hajdan Jr., darin (slow to review)
Visibility:
Public.

Description

Attempt 2 at landing this. 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 TBR=amit Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35778

Patch Set 1 #

Patch Set 2 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+365 lines, -177 lines) Patch
M chrome/browser/automation/automation_profile_impl.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/automation/url_request_automation_job.h View 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/automation/url_request_automation_job.cc View 5 chunks +32 lines, -5 lines 0 comments Download
M chrome/browser/chromeos/external_cookie_handler_unittest.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.cc View 1 chunk +1 line, -6 lines 0 comments Download
M chrome_frame/chrome_frame.gyp View 2 chunks +5 lines, -0 lines 0 comments Download
M chrome_frame/chrome_frame_activex_base.h View 2 chunks +10 lines, -2 lines 0 comments Download
M chrome_frame/chrome_frame_npapi.cc View 2 chunks +0 lines, -150 lines 0 comments Download
A chrome_frame/chrome_frame_npapi_entrypoints.h View 1 1 chunk +39 lines, -0 lines 0 comments Download
M chrome_frame/chrome_frame_npapi_entrypoints.cc View 3 chunks +165 lines, -11 lines 0 comments Download
M chrome_frame/test/chrome_frame_unittests.cc View 2 chunks +18 lines, -3 lines 0 comments Download
A chrome_frame/test/data/fulltab_delete_cookie_test.html View 1 chunk +47 lines, -0 lines 0 comments Download
A chrome_frame/test/data/fulltab_delete_cookie_test.html.mock-http-headers View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome_frame/test/html_util_unittests.cc View 3 chunks +24 lines, -0 lines 0 comments Download
M net/base/cookie_store.h View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
ananta
10 years, 11 months ago (2010-01-08 04:18:54 UTC) #1
amit
10 years, 11 months ago (2010-01-08 04:37:09 UTC) #2
lg

On Thu, Jan 7, 2010 at 8:18 PM,  <ananta@chromium.org> wrote:
> Reviewers: amit,
>
> Description:
> Attempt 2 at landing this.
>
> 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
>
> TBR=amit
>
>
> Please review this at http://codereview.chromium.org/521072
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/automation/automation_profile_impl.cc
>  M     chrome/browser/automation/url_request_automation_job.h
>  M     chrome/browser/automation/url_request_automation_job.cc
>  M     chrome/browser/chromeos/external_cookie_handler_unittest.cc
>  M     chrome/browser/renderer_host/resource_message_filter.cc
>  M     chrome_frame/chrome_frame.gyp
>  M     chrome_frame/chrome_frame_activex_base.h
>  M     chrome_frame/chrome_frame_npapi.cc
>  A     chrome_frame/chrome_frame_npapi_entrypoints.h
>  M     chrome_frame/chrome_frame_npapi_entrypoints.cc
>  M     chrome_frame/test/chrome_frame_unittests.cc
>  A     chrome_frame/test/data/fulltab_delete_cookie_test.html
>  A
> chrome_frame/test/data/fulltab_delete_cookie_test.html.mock-http-headers
>  M     chrome_frame/test/html_util_unittests.cc
>  M     net/base/cookie_store.h
>
>
>

Powered by Google App Engine
This is Rietveld 408576698