Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1393193005:
Implement $Secure- cookie prefix (Closed)
|
Created:
5 years, 2 months ago by estark Modified:
5 years ago CC:
cbentzel+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, extensions-reviews_chromium.org, jam, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement $Secure- cookie prefix
This CL implements the rule that cookies whose names start with
$Secure- can only be set if the Secure attribute is enabled. The
implementation is a runtime-enabled web platform feature for now.
Intent to Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IU5t6eLuS2Y/Uq-7Kat9BwAJ
BUG=541511
TBR=droger@chromium.org
Committed: https://crrev.com/467a098174e062d4380eb0ad8f7b2f0b0b7ed5fa
Cr-Commit-Position: refs/heads/master@{#354676}
Committed: https://crrev.com/cd39c11f87f9c660f6b11173f53c49928c39018d
Cr-Commit-Position: refs/heads/master@{#354832}
Patch Set 1 #Patch Set 2 : add another test #
Total comments: 10
Patch Set 3 : mkwst comments, split out rename into separate CL #
Total comments: 2
Patch Set 4 : add test #
Total comments: 2
Patch Set 5 : remove ContentBrowserClient method #
Total comments: 2
Patch Set 6 : limit prefixed cookies to secure origins #
Total comments: 2
Patch Set 7 : rebase #Patch Set 8 : don't modify context after init #
Total comments: 1
Patch Set 9 : fix capitalization #Patch Set 10 : android fix #Patch Set 11 : rebase #Patch Set 12 : TODO clarification #Patch Set 13 : fix cookie monster tests #Patch Set 14 : test fixes #Patch Set 15 : another ios test fix #Patch Set 16 : move SpawnedTestServer tests into non-ios section #Patch Set 17 : use EmbeddedTestServer to fix android test #Patch Set 18 : test fix #
Total comments: 2
Messages
Total messages: 66 (21 generated)
estark@chromium.org changed reviewers: + mkwst@chromium.org
Mike, here's a rough draft for your reviewing pleasure. It probably needs more tests. Also, the plumbing of the experimental features flag is a little ugly: I made it a cookie option so that URLRequestHTTPJob can set it from the NetworkDelegate when setting a cookie. But that also means that RenderFrameMessageFilter has to be able to set the option when setting a cookie, so it gets the flag value from ContentBrowserClient. Another alternative I considered was telling the cookie store that prefixes should be enforced when constructing the cookie store. That might avoid the need for the ContentBrowserClient method and the cookie option, but cookie stores are created in a bunch of places and it would be a bummer to forget one and have a cookie store that doesn't enforce prefixes. https://codereview.chromium.org/1393193005/diff/20001/net/base/network_delega... File net/base/network_delegate.h (right): https://codereview.chromium.org/1393193005/diff/20001/net/base/network_delega... net/base/network_delegate.h:114: bool ExperimentalFeaturesEnabled() const; Maybe this should be ExperimentalCookieFeaturesEnabled()?
This looks quite good. Sorry for the annoyance of piping through the boolean everywhere. https://codereview.chromium.org/1393193005/diff/20001/net/base/network_delega... File net/base/network_delegate.h (right): https://codereview.chromium.org/1393193005/diff/20001/net/base/network_delega... net/base/network_delegate.h:114: bool ExperimentalFeaturesEnabled() const; On 2015/10/09 at 13:51:57, estark wrote: > Maybe this should be ExperimentalCookieFeaturesEnabled()? Probably better, yes. I'd also suggest splitting the rename out into a separate CL for sanity, but that's entirely up to you. https://codereview.chromium.org/1393193005/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1393193005/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1907: << "SetCookie() not storing cookie that violates prefix rules."; Nit: Add the cookie name? https://codereview.chromium.org/1393193005/diff/20001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1393193005/diff/20001/net/url_request/url_req... net/url_request/url_request_unittest.cc:9569: TestExperimentalFeaturesNetworkDelegate network_delegate; This is better than what I did with the FPO cookie tests. Thanks! https://codereview.chromium.org/1393193005/diff/20001/net/url_request/url_req... net/url_request/url_request_unittest.cc:9611: // not be sent. You can spawn an HTTPS test server. Take a look at URLRequestTestReferrerPolicy for an example. Or URLRequestTestHTTP tests.
https://codereview.chromium.org/1393193005/diff/20001/net/url_request/url_req... File net/url_request/url_request_test_util.cc (right): https://codereview.chromium.org/1393193005/diff/20001/net/url_request/url_req... net/url_request/url_request_test_util.cc:620: return first_party_only_cookies_enabled_; Nit: Should probably rename this as well.
Thanks, Mike. I split out the rename into crrev.com/1392853006 and addressed your comments. (The trybot failures are because they're missing the patch from 1392853006, I think... not sure if there's any way to tell the trybots to patch in 1392853006 in addition to this patch.) https://codereview.chromium.org/1393193005/diff/20001/net/base/network_delega... File net/base/network_delegate.h (right): https://codereview.chromium.org/1393193005/diff/20001/net/base/network_delega... net/base/network_delegate.h:114: bool ExperimentalFeaturesEnabled() const; On 2015/10/09 15:41:33, Mike West (slow) wrote: > On 2015/10/09 at 13:51:57, estark wrote: > > Maybe this should be ExperimentalCookieFeaturesEnabled()? > > Probably better, yes. I'd also suggest splitting the rename out into a separate > CL for sanity, but that's entirely up to you. Done, and split out the rename into https://codereview.chromium.org/1392853006/ https://codereview.chromium.org/1393193005/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1393193005/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1907: << "SetCookie() not storing cookie that violates prefix rules."; On 2015/10/09 15:41:33, Mike West (slow) wrote: > Nit: Add the cookie name? Done. https://codereview.chromium.org/1393193005/diff/20001/net/url_request/url_req... File net/url_request/url_request_test_util.cc (right): https://codereview.chromium.org/1393193005/diff/20001/net/url_request/url_req... net/url_request/url_request_test_util.cc:620: return first_party_only_cookies_enabled_; On 2015/10/09 15:54:32, Mike West (slow) wrote: > Nit: Should probably rename this as well. Done, in https://codereview.chromium.org/1392853006/ https://codereview.chromium.org/1393193005/diff/20001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1393193005/diff/20001/net/url_request/url_req... net/url_request/url_request_unittest.cc:9611: // not be sent. On 2015/10/09 15:41:33, Mike West (slow) wrote: > You can spawn an HTTPS test server. Take a look at URLRequestTestReferrerPolicy > for an example. Or URLRequestTestHTTP tests. Done.
You'll need someone to take a look at //net/url_request, //content, and //chrome/browser, but //net/cookies LGTM. Thanks! https://codereview.chromium.org/1393193005/diff/40001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1393193005/diff/40001/net/url_request/url_req... net/url_request/url_request_unittest.cc:9598: // Verify that the cookie is set. You've tested non-secure non-experimental, secure experimental, and non-secure experimental. Would you mind adding a secure non-experimental, just for completeness? :)
(Nit: Would you mind linking to the Intent to Implement thread in the CL description?)
Patchset #4 (id:60001) has been deleted
On 2015/10/10 09:34:42, Mike West (slow) wrote: > (Nit: Would you mind linking to the Intent to Implement thread in the CL > description?) Sure, done.
estark@chromium.org changed reviewers: + jam@chromium.org, mmenke@chromium.org
jam: could you please review //content? mmenke: could you please review //net/url_request? (This is the follow-up to https://codereview.chromium.org/1392853006/) https://codereview.chromium.org/1393193005/diff/40001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1393193005/diff/40001/net/url_request/url_req... net/url_request/url_request_unittest.cc:9598: // Verify that the cookie is set. On 2015/10/10 09:34:24, Mike West (slow) wrote: > You've tested non-secure non-experimental, secure experimental, and non-secure > experimental. Would you mind adding a secure non-experimental, just for > completeness? :) Done.
jochen@chromium.org changed reviewers: + jochen@chromium.org
https://codereview.chromium.org/1393193005/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1393193005/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:647: ->HasSwitch(switches::kEnableExperimentalWebPlatformFeatures)), that's a content switch, you can just access it in content/ without the chrome content browser client api
https://codereview.chromium.org/1393193005/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1393193005/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:647: ->HasSwitch(switches::kEnableExperimentalWebPlatformFeatures)), On 2015/10/12 09:31:41, jochen wrote: > that's a content switch, you can just access it in content/ without the chrome > content browser client api That might be a little weird for non-chrome content embedders, though. For non-chrome embedders, when the experimental flag is on, prefixes will always be enforced for cookies set from the renderer, but not for cookies received over the network. Do you think that's okay?
On 2015/10/12 at 09:39:19, estark wrote: > https://codereview.chromium.org/1393193005/diff/80001/chrome/browser/chrome_c... > File chrome/browser/chrome_content_browser_client.cc (right): > > https://codereview.chromium.org/1393193005/diff/80001/chrome/browser/chrome_c... > chrome/browser/chrome_content_browser_client.cc:647: ->HasSwitch(switches::kEnableExperimentalWebPlatformFeatures)), > On 2015/10/12 09:31:41, jochen wrote: > > that's a content switch, you can just access it in content/ without the chrome > > content browser client api > > That might be a little weird for non-chrome content embedders, though. For non-chrome embedders, when the experimental flag is on, prefixes will always be enforced for cookies set from the renderer, but not for cookies received over the network. Do you think that's okay? yes
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
On 2015/10/12 09:40:17, jochen wrote: > On 2015/10/12 at 09:39:19, estark wrote: > > > https://codereview.chromium.org/1393193005/diff/80001/chrome/browser/chrome_c... > > File chrome/browser/chrome_content_browser_client.cc (right): > > > > > https://codereview.chromium.org/1393193005/diff/80001/chrome/browser/chrome_c... > > chrome/browser/chrome_content_browser_client.cc:647: > ->HasSwitch(switches::kEnableExperimentalWebPlatformFeatures)), > > On 2015/10/12 09:31:41, jochen wrote: > > > that's a content switch, you can just access it in content/ without the > chrome > > > content browser client api > > > > That might be a little weird for non-chrome content embedders, though. For > non-chrome embedders, when the experimental flag is on, prefixes will always be > enforced for cookies set from the renderer, but not for cookies received over > the network. Do you think that's okay? > > yes Ok! Done.
estark@chromium.org changed reviewers: - jam@chromium.org
jam: removing you as a reviewer because I forgot jochen is a content/ owner and in my time zone right now, but please feel free to review also if you'd like to
https://codereview.chromium.org/1393193005/diff/160001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1393193005/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:336: if (cc->Name().find(kSecurePrefix) == 0) Let's lock this down to CanonicalCookies whose `Source()`s whose `SchemeIsCryptographic()`. I'm updating the spec, as we discussed. :)
chrome & content lgtm
https://codereview.chromium.org/1393193005/diff/160001/net/cookies/cookie_mon... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1393193005/diff/160001/net/cookies/cookie_mon... net/cookies/cookie_monster.cc:336: if (cc->Name().find(kSecurePrefix) == 0) On 2015/10/12 10:59:04, Mike West (slow) wrote: > Let's lock this down to CanonicalCookies whose `Source()`s whose > `SchemeIsCryptographic()`. I'm updating the spec, as we discussed. :) Done.
https://codereview.chromium.org/1393193005/diff/180001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1393193005/diff/180001/net/url_request/url_re... net/url_request/url_request_unittest.cc:9527: default_context_.set_network_delegate(&network_delegate); It's not a good idea to modify a context after initialization - it will often end up in an inconsistent state.
https://codereview.chromium.org/1393193005/diff/180001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1393193005/diff/180001/net/url_request/url_re... net/url_request/url_request_unittest.cc:9527: default_context_.set_network_delegate(&network_delegate); On 2015/10/12 14:21:56, mmenke wrote: > It's not a good idea to modify a context after initialization - it will often > end up in an inconsistent state. Will fix for these tests, but it looks to me like this is how pretty much what almost all the tests in this file do. Are they all in need of fixing, or am I missing something?
On 2015/10/12 14:36:32, estark wrote: > https://codereview.chromium.org/1393193005/diff/180001/net/url_request/url_re... > File net/url_request/url_request_unittest.cc (right): > > https://codereview.chromium.org/1393193005/diff/180001/net/url_request/url_re... > net/url_request/url_request_unittest.cc:9527: > default_context_.set_network_delegate(&network_delegate); > On 2015/10/12 14:21:56, mmenke wrote: > > It's not a good idea to modify a context after initialization - it will often > > end up in an inconsistent state. > > Will fix for these tests, but it looks to me like this is how pretty much what > almost all the tests in this file do. Are they all in need of fixing, or am I > missing something? This is something that "mostly" works, but not completely - the proxy service, for instance, gets the wrong network delegate.... So it's just a bad idea. And yes, the other tests should be fixed (Looks like AllowFileURLs and a bunch of cookie tests that call set_network_delegate after calling Init() on the context).
On 2015/10/12 14:48:06, mmenke wrote: > On 2015/10/12 14:36:32, estark wrote: > > > https://codereview.chromium.org/1393193005/diff/180001/net/url_request/url_re... > > File net/url_request/url_request_unittest.cc (right): > > > > > https://codereview.chromium.org/1393193005/diff/180001/net/url_request/url_re... > > net/url_request/url_request_unittest.cc:9527: > > default_context_.set_network_delegate(&network_delegate); > > On 2015/10/12 14:21:56, mmenke wrote: > > > It's not a good idea to modify a context after initialization - it will > often > > > end up in an inconsistent state. > > > > Will fix for these tests, but it looks to me like this is how pretty much what > > almost all the tests in this file do. Are they all in need of fixing, or am I > > missing something? > > This is something that "mostly" works, but not completely - the proxy service, > for instance, gets the wrong network delegate.... So it's just a bad idea. And > yes, the other tests should be fixed (Looks like AllowFileURLs and a bunch of > cookie tests that call set_network_delegate after calling Init() on the > context). Ok, I'll file a bug for the others. Done.
//net/cookies still LGTM after the cc.Source() change. Matt, are you happy with the changes to //net/url_request tests?
Sorry, thought I signed off before. LGTM (Just looked at net/) Should we have tests with the feature off, to make sure the behavior change isn't sneaking in when experimental features are disabled? https://codereview.chromium.org/1393193005/diff/220001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1393193005/diff/220001/net/url_request/url_re... net/url_request/url_request_unittest.cc:9571: TEST_F(URLRequestTest, SecureCookiePrefixExperimentalNonSecure) { nit: Be consistent about captialization. In this CL, you're using Nonexperimental and Nonsecure in other test names, so I assume you want Nonsecure here? (As long as you're consistent with capitalization after the Non's, I'm fine either way)
On 2015/10/15 15:58:35, mmenke wrote: > Sorry, thought I signed off before. LGTM (Just looked at net/) > > Should we have tests with the feature off, to make sure the behavior change > isn't sneaking in when experimental features are disabled? One simple way to do this would be to put for loops in each test that checks for blocked cookies, and run them with and without the feature enabled, and check that the cookies is only present when it should be. That having been said, I'm not too concerned about breakage, but since we're going to the effort of making this experimental in the first place...
On 2015/10/15 15:58:35, mmenke wrote: > Sorry, thought I signed off before. LGTM (Just looked at net/) > > Should we have tests with the feature off, to make sure the behavior change > isn't sneaking in when experimental features are disabled? Isn't that what SecureCookiePrefixNonexperimental tests for? Or do you mean something else? > > https://codereview.chromium.org/1393193005/diff/220001/net/url_request/url_re... > File net/url_request/url_request_unittest.cc (right): > > https://codereview.chromium.org/1393193005/diff/220001/net/url_request/url_re... > net/url_request/url_request_unittest.cc:9571: TEST_F(URLRequestTest, > SecureCookiePrefixExperimentalNonSecure) { > nit: Be consistent about captialization. > > In this CL, you're using Nonexperimental and Nonsecure in other test names, so I > assume you want Nonsecure here? (As long as you're consistent with > capitalization after the Non's, I'm fine either way)
On 2015/10/15 16:03:34, estark wrote: > On 2015/10/15 15:58:35, mmenke wrote: > > Sorry, thought I signed off before. LGTM (Just looked at net/) > > > > Should we have tests with the feature off, to make sure the behavior change > > isn't sneaking in when experimental features are disabled? > > Isn't that what SecureCookiePrefixNonexperimental tests for? Or do you mean > something else? Erm...I couldn't say. The second of the three requests confuses me.
On 2015/10/15 16:08:50, mmenke wrote: > On 2015/10/15 16:03:34, estark wrote: > > On 2015/10/15 15:58:35, mmenke wrote: > > > Sorry, thought I signed off before. LGTM (Just looked at net/) > > > > > > Should we have tests with the feature off, to make sure the behavior change > > > isn't sneaking in when experimental features are disabled? > > > > Isn't that what SecureCookiePrefixNonexperimental tests for? Or do you mean > > something else? > > Erm...I couldn't say. The second of the three requests confuses me. Oh, two different cookies. Hadn't realized that.
Patchset #10 (id:260001) has been deleted
Patchset #16 (id:400001) has been deleted
- Added a comment to address mmenke's confusion about the nonexperimental test. - Fixed iOS and Android builds (I think the changes are pretty trivial but feel free to take a look before I land if you want)
On 2015/10/16 18:15:38, estark wrote: > - Added a comment to address mmenke's confusion about the nonexperimental test. > - Fixed iOS and Android builds > > (I think the changes are pretty trivial but feel free to take a look before I > land if you want) No need for a re-review, feel free to land at your leisure.
estark@chromium.org changed reviewers: + droger@chromium.org
droger: TBRing you for a small ios cookie test change
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mkwst@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1393193005/#ps420001 (title: "move SpawnedTestServer tests into non-ios section")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393193005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393193005/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393193005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393193005/420001
Message was sent while issue was closed.
Committed patchset #16 (id:420001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/467a098174e062d4380eb0ad8f7b2f0b0b7ed5fa Cr-Commit-Position: refs/heads/master@{#354676}
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:420001) has been created in https://codereview.chromium.org/1409243003/ by mnissler@chromium.org. The reason for reverting is: Broke net_unittests on Android: I 63.275s run_tests_on_device(06b2a92a003bca26) [ RUN ] URLRequestTest.SecureCookiePrefixOnNonsecureOrigin I 63.275s run_tests_on_device(06b2a92a003bca26) [ERROR:spawner_communicator.cc(241)] request failed, status: 3, error: -324 I 63.275s run_tests_on_device(06b2a92a003bca26) ../../net/url_request/url_request_unittest.cc:2737: Failure I 63.276s run_tests_on_device(06b2a92a003bca26) Value of: test_server_https.Start() I 63.276s run_tests_on_device(06b2a92a003bca26) Actual: false I 63.276s run_tests_on_device(06b2a92a003bca26) Expected: true I 63.276s run_tests_on_device(06b2a92a003bca26) [ERROR:spawner_communicator.cc(241)] request failed, status: 3, error: -324 I 63.276s run_tests_on_device(06b2a92a003bca26) [ FAILED ] URLRequestTest.SecureCookiePrefixOnNonsecureOrigin (487 ms) http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%... http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/22819.
Message was sent while issue was closed.
Sorry, I should have caught that - an Android test can only have one spawned test server. If you need two servers, other has to be an EmbeddedTestServer (Or could use mocks).
Message was sent while issue was closed.
On 2015/10/19 14:01:08, mmenke wrote: > Sorry, I should have caught that - an Android test can only have one spawned > test server. If you need two servers, other has to be an EmbeddedTestServer (Or > could use mocks). Not sure why the trybots didn't catch that.
Message was sent while issue was closed.
Description was changed from ========== Implement $Secure- cookie prefix This CL implements the rule that cookies whose names start with $Secure- can only be set if the Secure attribute is enabled. The implementation is a runtime-enabled web platform feature for now. Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/IU5t6eLuS2Y/Uq-7Kat9... BUG=541511 TBR=droger@chromium.org Committed: https://crrev.com/467a098174e062d4380eb0ad8f7b2f0b0b7ed5fa Cr-Commit-Position: refs/heads/master@{#354676} ========== to ========== Implement $Secure- cookie prefix This CL implements the rule that cookies whose names start with $Secure- can only be set if the Secure attribute is enabled. The implementation is a runtime-enabled web platform feature for now. Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/IU5t6eLuS2Y/Uq-7Kat9... BUG=541511 TBR=droger@chromium.org Committed: https://crrev.com/467a098174e062d4380eb0ad8f7b2f0b0b7ed5fa Cr-Commit-Position: refs/heads/master@{#354676} ==========
On 2015/10/19 14:01:08, mmenke wrote: > Sorry, I should have caught that - an Android test can only have one spawned > test server. If you need two servers, other has to be an EmbeddedTestServer (Or > could use mocks). Thanks! Fixed.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, jochen@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1393193005/#ps440001 (title: "use EmbeddedTestServer to fix android test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393193005/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393193005/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, jochen@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1393193005/#ps460001 (title: "test fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393193005/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393193005/460001
Message was sent while issue was closed.
Committed patchset #18 (id:460001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/cd39c11f87f9c660f6b11173f53c49928c39018d Cr-Commit-Position: refs/heads/master@{#354832}
Message was sent while issue was closed.
https://codereview.chromium.org/1393193005/diff/460001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1393193005/diff/460001/net/url_request/url_re... net/url_request/url_request_http_job.cc:752: if (network_delegate()->AreExperimentalCookieFeaturesEnabled()) Is it supported to have a null delegate? I see that a lot of places test for the existence of the delegate before calling it, but this is not done here. See line 663 above for example. There is some code (ios/crnet/crnet_environment) that sets up the network stack without network delegate, and it now crashes here. Should I use a dummy delegate there, or should we fix this call to support the null delegate?
Message was sent while issue was closed.
https://codereview.chromium.org/1393193005/diff/460001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1393193005/diff/460001/net/url_request/url_re... net/url_request/url_request_http_job.cc:752: if (network_delegate()->AreExperimentalCookieFeaturesEnabled()) On 2015/12/01 13:10:23, droger wrote: > Is it supported to have a null delegate? > I see that a lot of places test for the existence of the delegate before calling > it, but this is not done here. > See line 663 above for example. > > There is some code (ios/crnet/crnet_environment) that sets up the network stack > without network delegate, and it now crashes here. Should I use a dummy delegate > there, or should we fix this call to support the null delegate? Hrm...we generally allow a NULL delegate. The fact that apparently not a lot of tests use HTTP requests with NULL delegates, so this wasn't caught indicates that perhaps we should rethink supporting the NULL delegate case. For now, though, this code should be fixed. |
