SupervisedUsers : Fix transaction cleanup

SupervisedUsers : Fix transaction cleanup
When we create SU, it is a multi-step process. If creation fails on a late stage, we need to undo changes made on earlier stages (like adding user to a list, and creating home directory).
At some point this clean-up process got broken.
This CL fixes that.


BUG=313039
R=nkostylev@chromium.org

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

[Nikita (slow), 2013-11-13 01:16:57]

>> SupervisedUsers : Fix transaction cleanup

Please expand CL description?

[Nikita (slow), 2013-11-13 01:27:54]

These changes are not trivial, please add test coverage.
Especially since we want this in M32.

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.h:327: // Implementation for
RemoveUser method. This is a synchronous part of the
nit: empty line

[Nikita (slow), 2013-11-13 01:37:18]

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/supervised_user_manager_impl.cc (right):

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:279:
owner_->RemoveNonOwnerUserInternal(user_id, NULL);
What was the issue with using just RemoveUser?

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_manager_impl.cc (left):

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.cc:516: EnsureUsersLoaded();
Do you think whether removing this call might have some side effects?

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.cc:484: // This method is used
to implement UserManager::RemoveUser.
nit: drop comment.

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.h:189: // Returns |false|
otherwise. Does not  trigger user loading.
nit: drop double space.

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.h:329: void
RemoveNonOwnerUserInternal(const std::string& user_email,
I think you should fix code that calls RemoveUserInternal instead and there
should be no need to split this into two methods because now we have a way to
call RemoveNonOwnerUserInternal() with owner user_id and user will be
successfully deleted. This should never happen.

[Nikita (slow), 2013-11-13 01:50:38]

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.h:328: // method.
Please expand comment and add how this is different from previous one.

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.h:329: void
RemoveNonOwnerUserInternal(const std::string& user_email,
On 2013/11/13 01:37:19, Nikita Kostylev wrote:
> I think you should fix code that calls RemoveUserInternal instead and there
> should be no need to split this into two methods because now we have a way to
> call RemoveNonOwnerUserInternal() with owner user_id and user will be
> successfully deleted. This should never happen.

As discussed offline, my comments regarding moving this to public API should be
ignored since this one is still part of private API.

[Denis Kuznetsov (DE-MUC), 2013-11-14 01:18:17]

Added test + fixed  nits.

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/supervised_user_manager_impl.cc (right):

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:279:
owner_->RemoveNonOwnerUserInternal(user_id, NULL);
On 2013/11/13 01:37:19, Nikita Kostylev wrote:
> What was the issue with using just RemoveUser?
Yes, it tried to get a User* at some point, and as users were not loaded it just
skipped everything at time.
As I fixed that, the asynchronous issue came up, so this change works around
both issues.

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.h:189: // Returns |false|
otherwise. Does not  trigger user loading.
On 2013/11/13 01:37:19, Nikita Kostylev wrote:
> nit: drop double space.

Done.

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.h:327: // Implementation for
RemoveUser method. This is a synchronous part of the
On 2013/11/13 01:27:54, Nikita Kostylev wrote:
> nit: empty line

Done.

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.h:329: void
RemoveNonOwnerUserInternal(const std::string& user_email,
On 2013/11/13 01:50:39, Nikita Kostylev wrote:
> On 2013/11/13 01:37:19, Nikita Kostylev wrote:
> > I think you should fix code that calls RemoveUserInternal instead and there
> > should be no need to split this into two methods because now we have a way
to
> > call RemoveNonOwnerUserInternal() with owner user_id and user will be
> > successfully deleted. This should never happen.
> 
> As discussed offline, my comments regarding moving this to public API should
be
> ignored since this one is still part of private API.

Done.

[Nikita (slow), 2013-11-14 01:19:12]

lgtm with nits

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.h:189: // Returns |false|
otherwise. Does not  trigger user loading.
On 2013/11/13 01:37:19, Nikita Kostylev wrote:
> nit: drop double space.

Still an issue.

https://codereview.chromium.org/63173012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.h:328: // method.
On 2013/11/13 01:50:39, Nikita Kostylev wrote:
> Please expand comment and add how this is different from previous one.

Please update comment.

https://codereview.chromium.org/63173012/diff/120001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/managed/supervised_user_creation_browsertest.cc
(right):

https://codereview.chromium.org/63173012/diff/120001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_creation_browsertest.cc:298:
private:
nit: Insert empty line.

[commit-bot: I haz the power, 2013-11-14 01:48:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/63173012/230001

[commit-bot: I haz the power, 2013-11-14 02:46:25]

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

[commit-bot: I haz the power, 2013-11-14 11:58:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/63173012/230001

[commit-bot: I haz the power, 2013-11-14 13:10:28]

Change committed as 235134