Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9865018:
Enable cookies per default in net. Add an API to disable them by default, and do that in Chrome (Closed)
|
Created:
8 years, 9 months ago by jochen (gone - plz use gerrit) Modified:
8 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable cookies per default in net. Add an API to disable them by default, and do that in Chrome
BUG=none
TEST=existing tests shouldn't break
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130578
Patch Set 1 #
Total comments: 9
Patch Set 2 : updates #
Messages
Total messages: 17 (0 generated)
plz review
+wtc, +eroman to review instead of me (I'm at IETF) wtc/eroman: FWIW, I'm in favor of this change, although not necessarily the details. When I say "in favor", I mean it's the least disgusting of all alternatives. jochen can probably forward you the discussion details jochen/jam/i have had. On Tue, Mar 27, 2012 at 10:43 AM, <jochen@chromium.org> wrote: > Reviewers: willchan, John Abd-El-Malek, > > Message: > plz review > > Description: > Enable cookies per default in net. Add an API to disable them by default, > and do > that in Chrome > > BUG=none > TEST=existing tests shouldn't break > > > Please review this at https://chromiumcodereview.**appspot.com/9865018/<https://chromiumcodereview.... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M chrome/app/DEPS > M chrome/app/chrome_main_**delegate.cc > M net/url_request/url_request.h > M net/url_request/url_request.cc > > > Index: chrome/app/DEPS > diff --git a/chrome/app/DEPS b/chrome/app/DEPS > index 414df54d2f97908966423a6be6e955**2c9f9d52bc..** > 04303877ec3fd8360304c06a367b66**c74a46cff3 100644 > --- a/chrome/app/DEPS > +++ b/chrome/app/DEPS > @@ -10,6 +10,7 @@ include_rules = [ > "+content/public/browser/**render_process_host.h", > "+grit", # For generated headers > "+media/base", # For initializing media library. > + "+net/url_request/url_request.**h", # For initializing net library. > "+policy", # For generated headers and source > "+sandbox", > "+tools/memory_watcher", > Index: chrome/app/chrome_main_**delegate.cc > diff --git a/chrome/app/chrome_main_**delegate.cc > b/chrome/app/chrome_main_**delegate.cc > index f689512dbc8ada5a69d8381b65452c**fb01586775..** > d9e5b747126288dc3965afc33056b7**dde65fe16e 100644 > --- a/chrome/app/chrome_main_**delegate.cc > +++ b/chrome/app/chrome_main_**delegate.cc > @@ -34,6 +34,7 @@ > #include "content/public/common/**content_client.h" > #include "content/public/common/**content_paths.h" > #include "content/public/common/**content_switches.h" > +#include "net/url_request/url_request.**h" > #include "ui/base/resource/resource_**bundle.h" > #include "ui/base/ui_base_switches.h" > > @@ -394,6 +395,10 @@ struct MainFunction { > } // namespace > > ChromeMainDelegate::**ChromeMainDelegate() { > + // Chrome disallows cookies per default. All code paths that need to use > + // cookies need to go through one of chrome's URLRequestContexts which > have > + // a ChromeNetworkDelegate attached that selectively allows cookies > again. > + net::URLRequest::**SetDefaultCookiePolicyToBlock(**); > } > > ChromeMainDelegate::~**ChromeMainDelegate() { > Index: net/url_request/url_request.cc > diff --git a/net/url_request/url_request.**cc > b/net/url_request/url_request.**cc > index 4c2e2c35385be5fd4baa4fe832b27a**35bc6c1822..** > 41a30cb0d5818241c6c639713a3dd1**ee773616f7 100644 > --- a/net/url_request/url_request.**cc > +++ b/net/url_request/url_request.**cc > @@ -64,6 +64,12 @@ uint64 GenerateURLRequestIdentifier() { > return g_next_url_request_identifier+**+; > } > > +// True once the first URLRequest was started. > +bool g_url_requests_started = false; > + > +// True if cookies are accepted by default. > +bool g_default_cookie_policy = true; > + > } // namespace > > URLRequest::ProtocolFactory* > @@ -321,6 +327,12 @@ int URLRequest::GetResponseCode() { > } > > // static > +void URLRequest::**SetDefaultCookiePolicyToBlock(**) { > + CHECK(!g_url_requests_started)**; > + g_default_cookie_policy = false; > +} > + > +// static > bool URLRequest::IsHandledProtocol(**const std::string& scheme) { > return URLRequestJobManager::**GetInstance()->SupportsScheme(**scheme); > } > @@ -379,6 +391,7 @@ void URLRequest::set_delegate(**Delegate* delegate) { > } > > void URLRequest::Start() { > + g_url_requests_started = true; > response_info_.request_time = Time::Now(); > > // Only notify the delegate for the initial request. > @@ -819,7 +832,7 @@ bool URLRequest::CanGetCookies(**const CookieList& > cookie_list) const { > return context_->network_delegate()->**NotifyReadingCookies(this, > cookie_list); > } > - return false; > + return g_default_cookie_policy; > } > > bool URLRequest::CanSetCookie(const std::string& cookie_line, > @@ -830,7 +843,7 @@ bool URLRequest::CanSetCookie(const std::string& > cookie_line, > cookie_line, > options); > } > - return false; > + return g_default_cookie_policy; > } > > > Index: net/url_request/url_request.h > diff --git a/net/url_request/url_request.**h > b/net/url_request/url_request.**h > index d73d2fec6a34b05861710e347ab248**404af8bb18..** > fa049d65c311896bbc6d84ee74e904**0b8a7101f4 100644 > --- a/net/url_request/url_request.**h > +++ b/net/url_request/url_request.**h > @@ -291,6 +291,14 @@ class NET_EXPORT URLRequest : > NON_EXPORTED_BASE(public base::NonThreadSafe), > // will not have any more of its methods called. > virtual ~URLRequest(); > > + // Changes the default cookie policy from allowing all cookies to > denying all > + // cookies. Embedders that want to implement a more flexible policy > should > + // change the default to blocking all cookies, and provide a > NetworkDelegate > + // with the URLRequestContext that maintains the CookieStore. > + // The cookie policy default has to be set before the first URLRequest > is > + // started. Once it was set to deny all cookies, it cannot be changed > back. > + static void SetDefaultCookiePolicyToBlock(**); > + > // Returns true if the scheme can be handled by URLRequest. False > otherwise. > static bool IsHandledProtocol(const std::string& scheme); > > > >
On 2012/03/27 11:49:37, willchan wrote: > +wtc, +eroman to review instead of me (I'm at IETF) > > wtc/eroman: FWIW, I'm in favor of this change, although not necessarily the > details. When I say "in favor", I mean it's the least disgusting of all > alternatives. jochen can probably forward you the discussion details > jochen/jam/i have had. > In short, the issue is that for chrome, we'd like the default to be blocking all cookies, so services that want to have cookies are forced to go through the easily auditable places like content settings or chrome network delegate For other embedders of net or content, it should be as easy as possible to get started, so cookies should be on by default the available choices boiled down to a compile time constant or a global static in net. Will thinks the global static is the least disgusting alternative, so here it is :)
http://codereview.chromium.org/9865018/diff/1/chrome/app/chrome_main_delegate.cc File chrome/app/chrome_main_delegate.cc (right): http://codereview.chromium.org/9865018/diff/1/chrome/app/chrome_main_delegate... chrome/app/chrome_main_delegate.cc:401: net::URLRequest::SetDefaultCookiePolicyToBlock(); why do this in chrome/app instead of chrome/browser? the former is for chrome.exe while the latter is chrome.dll. and since they'll have different copies of statics on windows, i think you'll need this in browser
Patch set 1 LGTM. Just some nits, and an important question below. https://chromiumcodereview.appspot.com/9865018/diff/1/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://chromiumcodereview.appspot.com/9865018/diff/1/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:398: // Chrome disallows cookies per default. All code paths that need to use Nit: per default => by default https://chromiumcodereview.appspot.com/9865018/diff/1/net/url_request/url_req... File net/url_request/url_request.cc (left): https://chromiumcodereview.appspot.com/9865018/diff/1/net/url_request/url_req... net/url_request/url_request.cc:822: return false; IMPORTANT: this means the original code is more strict. The new code will make URLRequest::CanGetCookies() and URLRequest::CanSetCookie() to return true by default. Is this intended? https://chromiumcodereview.appspot.com/9865018/diff/1/net/url_request/url_req... File net/url_request/url_request.cc (right): https://chromiumcodereview.appspot.com/9865018/diff/1/net/url_request/url_req... net/url_request/url_request.cc:71: bool g_default_cookie_policy = true; This variable name does not sound like a boolean. It sounds like an enum... Based on the how this variable is used below, I guess it should be named something like g_default_can_use_cookies. https://chromiumcodereview.appspot.com/9865018/diff/1/net/url_request/url_req... File net/url_request/url_request.h (right): https://chromiumcodereview.appspot.com/9865018/diff/1/net/url_request/url_req... net/url_request/url_request.h:294: // Changes the default cookie policy from allowing all cookies to denying all Nit: denying => blocking ? which is a little more consistent with the function's name. Also on line 299 "deny all cookies".
On 2012/03/28 17:51:05, wtc wrote: > IMPORTANT: this means the original code is more strict. > The new code will make URLRequest::CanGetCookies() and > URLRequest::CanSetCookie() to return true by default. > > Is this intended? The old code had CanSetCookie/CanGetCookie on URLRequestDelegate which defaulted to true. I then removed these methods in https://chromiumcodereview.appspot.com/9572001 removed these methods, so the default was false (which is what we want for Chrome). For other embedders of net, however, we don't want this strict default. That's why this CL makes the default one-time configurable. Chrome still forces it to false, while all other embedders of net can use cookies.
On 2012/03/27 19:17:35, John Abd-El-Malek wrote: > http://codereview.chromium.org/9865018/diff/1/chrome/app/chrome_main_delegate.cc > File chrome/app/chrome_main_delegate.cc (right): > > http://codereview.chromium.org/9865018/diff/1/chrome/app/chrome_main_delegate... > chrome/app/chrome_main_delegate.cc:401: > net::URLRequest::SetDefaultCookiePolicyToBlock(); > why do this in chrome/app instead of chrome/browser? the former is for > chrome.exe while the latter is chrome.dll. and since they'll have different > copies of statics on windows, i think you'll need this in browser Can you recommend a method where i should invoke this? Ideally, it should be a method all chrome process types have to go through
I'm currently swamped with the stability code yellow, I don't think I'll be able to review this CL promptly. On Tue, Mar 27, 2012 at 4:49 AM, William Chan (ιζΊζ) <willchan@chromium.org>wrote: > +wtc, +eroman to review instead of me (I'm at IETF) > > wtc/eroman: FWIW, I'm in favor of this change, although not necessarily > the details. When I say "in favor", I mean it's the least disgusting of all > alternatives. jochen can probably forward you the discussion details > jochen/jam/i have had. > > On Tue, Mar 27, 2012 at 10:43 AM, <jochen@chromium.org> wrote: > >> Reviewers: willchan, John Abd-El-Malek, >> >> Message: >> plz review >> >> Description: >> Enable cookies per default in net. Add an API to disable them by default, >> and do >> that in Chrome >> >> BUG=none >> TEST=existing tests shouldn't break >> >> >> Please review this at https://chromiumcodereview.**appspot.com/9865018/<https://chromiumcodereview.... >> >> SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> >> >> Affected files: >> M chrome/app/DEPS >> M chrome/app/chrome_main_**delegate.cc >> M net/url_request/url_request.h >> M net/url_request/url_request.cc >> >> >> Index: chrome/app/DEPS >> diff --git a/chrome/app/DEPS b/chrome/app/DEPS >> index 414df54d2f97908966423a6be6e955**2c9f9d52bc..** >> 04303877ec3fd8360304c06a367b66**c74a46cff3 100644 >> --- a/chrome/app/DEPS >> +++ b/chrome/app/DEPS >> @@ -10,6 +10,7 @@ include_rules = [ >> "+content/public/browser/**render_process_host.h", >> "+grit", # For generated headers >> "+media/base", # For initializing media library. >> + "+net/url_request/url_request.**h", # For initializing net library. >> "+policy", # For generated headers and source >> "+sandbox", >> "+tools/memory_watcher", >> Index: chrome/app/chrome_main_**delegate.cc >> diff --git a/chrome/app/chrome_main_**delegate.cc >> b/chrome/app/chrome_main_**delegate.cc >> index f689512dbc8ada5a69d8381b65452c**fb01586775..** >> d9e5b747126288dc3965afc33056b7**dde65fe16e 100644 >> --- a/chrome/app/chrome_main_**delegate.cc >> +++ b/chrome/app/chrome_main_**delegate.cc >> @@ -34,6 +34,7 @@ >> #include "content/public/common/**content_client.h" >> #include "content/public/common/**content_paths.h" >> #include "content/public/common/**content_switches.h" >> +#include "net/url_request/url_request.**h" >> #include "ui/base/resource/resource_**bundle.h" >> #include "ui/base/ui_base_switches.h" >> >> @@ -394,6 +395,10 @@ struct MainFunction { >> } // namespace >> >> ChromeMainDelegate::**ChromeMainDelegate() { >> + // Chrome disallows cookies per default. All code paths that need to >> use >> + // cookies need to go through one of chrome's URLRequestContexts which >> have >> + // a ChromeNetworkDelegate attached that selectively allows cookies >> again. >> + net::URLRequest::**SetDefaultCookiePolicyToBlock(**); >> } >> >> ChromeMainDelegate::~**ChromeMainDelegate() { >> Index: net/url_request/url_request.cc >> diff --git a/net/url_request/url_request.**cc >> b/net/url_request/url_request.**cc >> index 4c2e2c35385be5fd4baa4fe832b27a**35bc6c1822..** >> 41a30cb0d5818241c6c639713a3dd1**ee773616f7 100644 >> --- a/net/url_request/url_request.**cc >> +++ b/net/url_request/url_request.**cc >> @@ -64,6 +64,12 @@ uint64 GenerateURLRequestIdentifier() { >> return g_next_url_request_identifier+**+; >> } >> >> +// True once the first URLRequest was started. >> +bool g_url_requests_started = false; >> + >> +// True if cookies are accepted by default. >> +bool g_default_cookie_policy = true; >> + >> } // namespace >> >> URLRequest::ProtocolFactory* >> @@ -321,6 +327,12 @@ int URLRequest::GetResponseCode() { >> } >> >> // static >> +void URLRequest::**SetDefaultCookiePolicyToBlock(**) { >> + CHECK(!g_url_requests_started)**; >> + g_default_cookie_policy = false; >> +} >> + >> +// static >> bool URLRequest::IsHandledProtocol(**const std::string& scheme) { >> return URLRequestJobManager::**GetInstance()->SupportsScheme(**scheme); >> } >> @@ -379,6 +391,7 @@ void URLRequest::set_delegate(**Delegate* delegate) { >> } >> >> void URLRequest::Start() { >> + g_url_requests_started = true; >> response_info_.request_time = Time::Now(); >> >> // Only notify the delegate for the initial request. >> @@ -819,7 +832,7 @@ bool URLRequest::CanGetCookies(**const CookieList& >> cookie_list) const { >> return context_->network_delegate()->**NotifyReadingCookies(this, >> >> cookie_list); >> } >> - return false; >> + return g_default_cookie_policy; >> } >> >> bool URLRequest::CanSetCookie(const std::string& cookie_line, >> @@ -830,7 +843,7 @@ bool URLRequest::CanSetCookie(const std::string& >> cookie_line, >> cookie_line, >> options); >> } >> - return false; >> + return g_default_cookie_policy; >> } >> >> >> Index: net/url_request/url_request.h >> diff --git a/net/url_request/url_request.**h >> b/net/url_request/url_request.**h >> index d73d2fec6a34b05861710e347ab248**404af8bb18..** >> fa049d65c311896bbc6d84ee74e904**0b8a7101f4 100644 >> --- a/net/url_request/url_request.**h >> +++ b/net/url_request/url_request.**h >> @@ -291,6 +291,14 @@ class NET_EXPORT URLRequest : >> NON_EXPORTED_BASE(public base::NonThreadSafe), >> // will not have any more of its methods called. >> virtual ~URLRequest(); >> >> + // Changes the default cookie policy from allowing all cookies to >> denying all >> + // cookies. Embedders that want to implement a more flexible policy >> should >> + // change the default to blocking all cookies, and provide a >> NetworkDelegate >> + // with the URLRequestContext that maintains the CookieStore. >> + // The cookie policy default has to be set before the first URLRequest >> is >> + // started. Once it was set to deny all cookies, it cannot be changed >> back. >> + static void SetDefaultCookiePolicyToBlock(**); >> + >> // Returns true if the scheme can be handled by URLRequest. False >> otherwise. >> static bool IsHandledProtocol(const std::string& scheme); >> >> >> >> >
On 2012/03/28 17:56:53, jochen wrote: > On 2012/03/27 19:17:35, John Abd-El-Malek wrote: > > > http://codereview.chromium.org/9865018/diff/1/chrome/app/chrome_main_delegate.cc > > File chrome/app/chrome_main_delegate.cc (right): > > > > > http://codereview.chromium.org/9865018/diff/1/chrome/app/chrome_main_delegate... > > chrome/app/chrome_main_delegate.cc:401: > > net::URLRequest::SetDefaultCookiePolicyToBlock(); > > why do this in chrome/app instead of chrome/browser? the former is for > > chrome.exe while the latter is chrome.dll. and since they'll have different > > copies of statics on windows, i think you'll need this in browser > > Can you recommend a method where i should invoke this? Ideally, it should be a > method all chrome process types have to go through oh, I assumed you only want this in the browser process because the other processes shouldn't be making their own requests??
On 2012/03/29 02:36:29, John Abd-El-Malek wrote: > On 2012/03/28 17:56:53, jochen wrote: > > On 2012/03/27 19:17:35, John Abd-El-Malek wrote: > > > > > > http://codereview.chromium.org/9865018/diff/1/chrome/app/chrome_main_delegate.cc > > > File chrome/app/chrome_main_delegate.cc (right): > > > > > > > > > http://codereview.chromium.org/9865018/diff/1/chrome/app/chrome_main_delegate... > > > chrome/app/chrome_main_delegate.cc:401: > > > net::URLRequest::SetDefaultCookiePolicyToBlock(); > > > why do this in chrome/app instead of chrome/browser? the former is for > > > chrome.exe while the latter is chrome.dll. and since they'll have different > > > copies of statics on windows, i think you'll need this in browser > > > > Can you recommend a method where i should invoke this? Ideally, it should be a > > method all chrome process types have to go through > > oh, I assumed you only want this in the browser process because the other > processes shouldn't be making their own requests?? I'm not sure whether e.g. the service process can make its own requests?
Yes it can make its own requests. On Mar 29, 2012 10:25 AM, <jochen@chromium.org> wrote: > On 2012/03/29 02:36:29, John Abd-El-Malek wrote: > >> On 2012/03/28 17:56:53, jochen wrote: >> > On 2012/03/27 19:17:35, John Abd-El-Malek wrote: >> > > >> > >> > > http://codereview.chromium.**org/9865018/diff/1/chrome/app/** > chrome_main_delegate.cc<http://codereview.chromium.org/9865018/diff/1/chrome/app/chrome_main_delegate.cc> > >> > > File chrome/app/chrome_main_**delegate.cc (right): >> > > >> > > >> > >> > > http://codereview.chromium.**org/9865018/diff/1/chrome/app/** > chrome_main_delegate.cc#**newcode401<http://codereview.chromium.org/9865018/diff/1/chrome/app/chrome_main_delegate.cc#newcode401> > >> > > chrome/app/chrome_main_**delegate.cc:401: >> > > net::URLRequest::**SetDefaultCookiePolicyToBlock(**); >> > > why do this in chrome/app instead of chrome/browser? the former is for >> > > chrome.exe while the latter is chrome.dll. and since they'll have >> > different > >> > > copies of statics on windows, i think you'll need this in browser >> > >> > Can you recommend a method where i should invoke this? Ideally, it >> should be >> > a > >> > method all chrome process types have to go through >> > > oh, I assumed you only want this in the browser process because the other >> processes shouldn't be making their own requests?? >> > > I'm not sure whether e.g. the service process can make its own requests? > > https://chromiumcodereview.**appspot.com/9865018/<https://chromiumcodereview.... >
On 2012/03/29 09:02:06, willchan wrote: > Yes it can make its own requests. right because the service process is the only other "host" process. so we can call this net function from both BrowserMain and ServiceProcessMain.
https://chromiumcodereview.appspot.com/9865018/diff/1/chrome/app/chrome_main_... File chrome/app/chrome_main_delegate.cc (right): https://chromiumcodereview.appspot.com/9865018/diff/1/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:398: // Chrome disallows cookies per default. All code paths that need to use On 2012/03/28 17:51:05, wtc wrote: > > Nit: per default => by default Done. https://chromiumcodereview.appspot.com/9865018/diff/1/chrome/app/chrome_main_... chrome/app/chrome_main_delegate.cc:401: net::URLRequest::SetDefaultCookiePolicyToBlock(); On 2012/03/27 19:17:35, John Abd-El-Malek wrote: > why do this in chrome/app instead of chrome/browser? the former is for > chrome.exe while the latter is chrome.dll. and since they'll have different > copies of statics on windows, i think you'll need this in browser Done. https://chromiumcodereview.appspot.com/9865018/diff/1/net/url_request/url_req... File net/url_request/url_request.cc (right): https://chromiumcodereview.appspot.com/9865018/diff/1/net/url_request/url_req... net/url_request/url_request.cc:71: bool g_default_cookie_policy = true; On 2012/03/28 17:51:05, wtc wrote: > > This variable name does not sound like a boolean. > It sounds like an enum... > > Based on the how this variable is used below, I guess > it should be named something like g_default_can_use_cookies. Done. https://chromiumcodereview.appspot.com/9865018/diff/1/net/url_request/url_req... File net/url_request/url_request.h (right): https://chromiumcodereview.appspot.com/9865018/diff/1/net/url_request/url_req... net/url_request/url_request.h:294: // Changes the default cookie policy from allowing all cookies to denying all On 2012/03/28 17:51:05, wtc wrote: > > Nit: denying => blocking ? > > which is a little more consistent with the function's name. > > Also on line 299 "deny all cookies". Done.
Patch set 2 LGTM.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jochen@chromium.org/9865018/15001
Try job failure for 9865018-15001 (retry) on linux_rel for step "unit_tests". It's a second try, previously, steps "ui_tests, unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
