Fix cloud policy duplicate registrations issue.

Fix cloud policy duplicate registrations issue.

The automatic retry behavior in case of misconfigured proxies and
ERR_NETWORK_CHANGED could sometimes cause duplicated requests at the server,
which then refuses the 2nd request with the same device-id. Setting the
reregister flag in subsequent requests fixes that problem.

BUG=173768
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182871

[Joao da Silva, 2013-02-08 18:01:27]

@Mattias: please review.

You'll immediately notice that there are no new tests yet; I'd like your opinion
on the changes before writing them.

@Bin: FYI

[Mattias Nissler (ping if slow), 2013-02-11 11:30:37]

I guess I'm OK with this. The fact that the retry logic is split up now doesn't
delight me, but I don't have a better idea either.

https://codereview.chromium.org/12209070/diff/1/chrome/browser/policy/cloud_p...
File chrome/browser/policy/cloud_policy_client.cc (right):

https://codereview.chromium.org/12209070/diff/1/chrome/browser/policy/cloud_p...
chrome/browser/policy/cloud_policy_client.cc:210: // arrive to the client then
retrying with the same device-id will fail.
nit: s/to/at/

https://codereview.chromium.org/12209070/diff/1/chrome/browser/policy/cloud_p...
chrome/browser/policy/cloud_policy_client.cc:210: // arrive to the client then
retrying with the same device-id will fail.
nit: s/device-id/client ID/ (for consistency with the file)

https://codereview.chromium.org/12209070/diff/1/chrome/browser/policy/cloud_p...
chrome/browser/policy/cloud_policy_client.cc:212: // If the server hasn't seen
the device-id before then it will also accept
ditto

[Joao da Silva, 2013-02-11 17:38:19]

Lots of tests added, please have a look.

I guess this won't make it before the branch point, and may have to be merged.

https://codereview.chromium.org/12209070/diff/1/chrome/browser/policy/cloud_p...
File chrome/browser/policy/cloud_policy_client.cc (right):

https://codereview.chromium.org/12209070/diff/1/chrome/browser/policy/cloud_p...
chrome/browser/policy/cloud_policy_client.cc:210: // arrive to the client then
retrying with the same device-id will fail.
On 2013/02/11 11:30:37, Mattias Nissler wrote:
> nit: s/to/at/

Done.

https://codereview.chromium.org/12209070/diff/1/chrome/browser/policy/cloud_p...
chrome/browser/policy/cloud_policy_client.cc:210: // arrive to the client then
retrying with the same device-id will fail.
On 2013/02/11 11:30:37, Mattias Nissler wrote:
> nit: s/device-id/client ID/ (for consistency with the file)

Done.

https://codereview.chromium.org/12209070/diff/1/chrome/browser/policy/cloud_p...
chrome/browser/policy/cloud_policy_client.cc:212: // If the server hasn't seen
the device-id before then it will also accept
On 2013/02/11 11:30:37, Mattias Nissler wrote:
> ditto

Done.

[Mattias Nissler (ping if slow), 2013-02-11 18:23:49]

https://codereview.chromium.org/12209070/diff/3002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/12209070/diff/3002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_browsertest.cc:102: // posted to IO after all
expected requests are complete, if |once_| is false.
That's a weird protocol. Let's have only one way of unregistration.

https://codereview.chromium.org/12209070/diff/3002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_browsertest.cc:291: // registration with that
token if the re-register flag is set.
This is odd, because the error handling is now in two places: In the
ErrorInjector _and_ the test server. I think we should implement that in one
place. It may make sense to run the failure tests not against the test server
but against a fake server implementation based on the injector stuff. Note that
device_management_service_browsertest.cc has almost-suitable code already.

https://codereview.chromium.org/12209070/diff/3002/net/tools/testserver/devic...
File net/tools/testserver/device_management.py (right):

https://codereview.chromium.org/12209070/diff/3002/net/tools/testserver/devic...
net/tools/testserver/device_management.py:221: return (400, 'Expected reregister
request')
I'm concerned that we keep adding weirdness to the test server to the point
where the code becomes completely incomprehensible.

[Joao da Silva, 2013-02-12 16:33:41]

@Mattias: PTAL

https://codereview.chromium.org/12209070/diff/3002/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/12209070/diff/3002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_browsertest.cc:102: // posted to IO after all
expected requests are complete, if |once_| is false.
On 2013/02/11 18:23:50, Mattias Nissler wrote:
> That's a weird protocol. Let's have only one way of unregistration.

Obsolete.

https://codereview.chromium.org/12209070/diff/3002/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_browsertest.cc:291: // registration with that
token if the re-register flag is set.
On 2013/02/11 18:23:50, Mattias Nissler wrote:
> This is odd, because the error handling is now in two places: In the
> ErrorInjector _and_ the test server. I think we should implement that in one
> place. It may make sense to run the failure tests not against the test server
> but against a fake server implementation based on the injector stuff. Note
that
> device_management_service_browsertest.cc has almost-suitable code already.

Fair. I think there's value in having both kinds of test: against the
testserver, and with canned responses.

Let's leave this file with tests against the testserver; these changes will be
reverted because it's hard to verify the register request in this way.

I'll add a new browsertest that is similar to this one but runs with canned
responses, similar to device_management_service_browsertest. I'm not adding
these tests there because those tests run with their own
device_management_service instead of the connector's, and there's a lot of setup
for the cloud policy stack that doesn't belong there.

https://codereview.chromium.org/12209070/diff/3002/net/tools/testserver/devic...
File net/tools/testserver/device_management.py (right):

https://codereview.chromium.org/12209070/diff/3002/net/tools/testserver/devic...
net/tools/testserver/device_management.py:221: return (400, 'Expected reregister
request')
On 2013/02/11 18:23:50, Mattias Nissler wrote:
> I'm concerned that we keep adding weirdness to the test server to the point
> where the code becomes completely incomprehensible.

Reverted.

[Mattias Nissler (ping if slow), 2013-02-13 10:50:43]

mostly good

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
File chrome/browser/policy/cloud_policy_manager_browsertest.cc (right):

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:87:
net::NetworkDelegate* network_delegate) {
move kBadHeaders definition here?

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:109:
const_cast<net::UploadDataStream*>(request->get_upload());
so there's no way to read a const UploadDataStream without const_cast'ing it and
destroying it?

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:170: std::string
headers(kGoodHeaders, arraysize(kGoodHeaders));
seems like kGoodHeaders could be a static constant defined here?

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:185: // Intercepts all
requests to "dmserver.com" while in scope. Must be created and
Let's use localhost or example.com to be on the safe side.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:187: class
RequestInterceptor {
It seems this both large and generic enough to move to a sepparte file.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:210: size_t
pending_size = 0xffff;
That's a weird choice. How about std::numeric_limits<size_t>::max()?

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:278:
DISALLOW_COPY_AND_ASSIGN(RequestInterceptor);
#include "base/basictypes.h"

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:309: }
This should probably be factored out now that we have the code in two places.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:331: #endif
I'm wondering whether this test would be simpler to follow if you'd just create
a separate CloudPolicyManager (or CloudPolicyCore) instance for testing. Given
the extensive setup you do, the test doesn't really test the real thing as it
runs in the browser.

[Joao da Silva, 2013-02-13 15:57:15]

PTAL

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
File chrome/browser/policy/cloud_policy_manager_browsertest.cc (right):

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:87:
net::NetworkDelegate* network_delegate) {
On 2013/02/13 10:50:43, Mattias Nissler wrote:
> move kBadHeaders definition here?

Done.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:109:
const_cast<net::UploadDataStream*>(request->get_upload());
On 2013/02/13 10:50:43, Mattias Nissler wrote:
> so there's no way to read a const UploadDataStream without const_cast'ing it
and
> destroying it?

It's actually possible to read from the const object and without destroying it,
yes.

The fetchers from device_management_service.cc set the upload data as a single
string, to the UploadDataStream has a single UploadBytesElementsReader with the
data in memory, and it's possible to read it using only const objects.

This has been done in the new file.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:170: std::string
headers(kGoodHeaders, arraysize(kGoodHeaders));
On 2013/02/13 10:50:43, Mattias Nissler wrote:
> seems like kGoodHeaders could be a static constant defined here?

Done.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:185: // Intercepts all
requests to "dmserver.com" while in scope. Must be created and
On 2013/02/13 10:50:43, Mattias Nissler wrote:
> Let's use localhost or http://example.com to be on the safe side.

Done.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:187: class
RequestInterceptor {
On 2013/02/13 10:50:43, Mattias Nissler wrote:
> It seems this both large and generic enough to move to a sepparte file.

Done. Also updated device_management_service_browsertest.cc to use the new
interceptor class.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:210: size_t
pending_size = 0xffff;
On 2013/02/13 10:50:43, Mattias Nissler wrote:
> That's a weird choice. How about std::numeric_limits<size_t>::max()?

Done.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:278:
DISALLOW_COPY_AND_ASSIGN(RequestInterceptor);
On 2013/02/13 10:50:43, Mattias Nissler wrote:
> #include "base/basictypes.h"

Done.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:309: }
On 2013/02/13 10:50:43, Mattias Nissler wrote:
> This should probably be factored out now that we have the code in two places.

Done. Added policy/test_utils.h to contain helpers like this; it's a bit empty
for now, but we'll probably put more utilities there now that it exists.

https://codereview.chromium.org/12209070/diff/11012/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:331: #endif
On 2013/02/13 10:50:43, Mattias Nissler wrote:
> I'm wondering whether this test would be simpler to follow if you'd just
create
> a separate CloudPolicyManager (or CloudPolicyCore) instance for testing. Given
> the extensive setup you do, the test doesn't really test the real thing as it
> runs in the browser.

I'd prefer not to do this setup too. Unfortunately it's required to setup a
signed-in state on the desktop, which is besides what we want to test here.

I still think this is better than creating a manager directly. This covers the
connector creating the cloud policy stuff on startup, unblocking cache loading,
etc.

[Mattias Nissler (ping if slow), 2013-02-15 08:07:36]

LGTM with nits.

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
File chrome/browser/policy/cloud_policy_manager_browsertest.cc (right):

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:13: #include
"chrome/browser/policy/cloud_policy_constants.h"
needed?

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:20: #include
"content/public/browser/browser_thread.h"
needed?

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:21: #include
"googleurl/src/gurl.h"
needed?

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:23: #include
"policy/policy_constants.h"
needed?

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:28: #include
"chrome/browser/chromeos/login/user_manager.h"
needed?

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:46: class
MockCloudPolicyClientObserver : public CloudPolicyClient::Observer {
FWIW, we have this already in could_policy_client_unittest.cc

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/tes...
File chrome/browser/policy/test_request_interceptor.cc (right):

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/tes...
chrome/browser/policy/test_request_interceptor.cc:128: #endif
Should we pass this in? Otherwise this won't work for device policy.

[Joao da Silva, 2013-02-15 12:30:19]

Thanks for the comments!

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
File chrome/browser/policy/cloud_policy_manager_browsertest.cc (right):

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:13: #include
"chrome/browser/policy/cloud_policy_constants.h"
On 2013/02/15 08:07:36, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:20: #include
"content/public/browser/browser_thread.h"
On 2013/02/15 08:07:36, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:21: #include
"googleurl/src/gurl.h"
On 2013/02/15 08:07:36, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:23: #include
"policy/policy_constants.h"
On 2013/02/15 08:07:36, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:28: #include
"chrome/browser/chromeos/login/user_manager.h"
On 2013/02/15 08:07:36, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_manager_browsertest.cc:46: class
MockCloudPolicyClientObserver : public CloudPolicyClient::Observer {
On 2013/02/15 08:07:36, Mattias Nissler wrote:
> FWIW, we have this already in could_policy_client_unittest.cc

And also in cloud_policy_browsertest.cc! Moved to mock_cloud_policy_client.h,
and moved that unit to test_support_common.

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/tes...
File chrome/browser/policy/test_request_interceptor.cc (right):

https://codereview.chromium.org/12209070/diff/15002/chrome/browser/policy/tes...
chrome/browser/policy/test_request_interceptor.cc:128: #endif
On 2013/02/15 08:07:36, Mattias Nissler wrote:
> Should we pass this in? Otherwise this won't work for device policy.

Good idea, done.

[commit-bot: I haz the power, 2013-02-15 12:31:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/12209070/26001

[commit-bot: I haz the power, 2013-02-15 16:52:54]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-02-15 18:06:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/12209070/34001

[commit-bot: I haz the power, 2013-02-15 18:41:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-15 19:23:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/12209070/42001

[commit-bot: I haz the power, 2013-02-15 22:23:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/12209070/42001

[commit-bot: I haz the power, 2013-02-16 00:45:50]

Change committed as 182871