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

Issue 517070: Revert 35769 - Deleting cookies by setting the expires (Closed)

Created:
10 years, 11 months ago by tyoshino (SeeGerritForStatus)
Modified:
9 years, 7 months ago
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

Reason: Linux Builder (ChromiumOS) failed. http://chrome-buildbot.corp.google.com:8010/builders/Linux%20Builder%20(ChromiumOS)/builds/2050/steps/compile/logs/stdio Please add changes to external_cookie_handler_unittest.cc no to break compilation and reland? ---- Revert 35769 - 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 SetCookie 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 Review URL: http://codereview.chromium.org/518054 TBR=ananta@chromium.org

Patch Set 1 #

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

Messages

Total messages: 3 (0 generated)
tyoshino (SeeGerritForStatus)
10 years, 11 months ago (2010-01-08 03:38:03 UTC) #1
ananta
LGTM. Thanks for the quick fix. -Ananta On Thu, Jan 7, 2010 at 7:38 PM, ...
10 years, 11 months ago (2010-01-08 03:48:11 UTC) #2
tyoshino (SeeGerritForStatus)
10 years, 11 months ago (2010-01-12 04:14:49 UTC) #3
Thank you! Closing.

On 2010/01/08 03:48:11, ananta wrote:
> LGTM. Thanks for the quick fix.
> 
> -Ananta
> 
> On Thu, Jan 7, 2010 at 7:38 PM, <mailto:tyoshino@chromium.org> wrote:
> 
> > Reviewers: ananta,
> >
> > Description:
> > Reason:
> > Linux Builder (ChromiumOS) failed.
> >
> >
> >
>
http://chrome-buildbot.corp.google.com:8010/builders/Linux%2520Builder%2520%2...
> >
> > Please add changes to external_cookie_handler_unittest.cc no to break
> > compilation
> > and reland?
> >
> > ----
> >
> > Revert 35769 - 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 SetCookie 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
> >
> > Review URL: http://codereview.chromium.org/518054
> >
> > mailto:TBR=ananta@chromium.org
> >
> > Please review this at http://codereview.chromium.org/517070
> >
> > SVN Base: svn://svn.chromium.org/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/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
> >  D     chrome_frame/chrome_frame_npapi_entrypoints.h
> >  M     chrome_frame/chrome_frame_npapi_entrypoints.cc
> >  M     chrome_frame/test/chrome_frame_unittests.cc
> >  D     chrome_frame/test/data/fulltab_delete_cookie_test.html
> >  D
> > 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