ManagePasswordsUIController should make a deep copy of values which it doesn't own.

ManagePasswordsUIController should make a deep copy of values which it doesn't own.

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

[vasilii, 2014-08-04 15:12:58]

Hi Vaclav,

please review the CL.

[vabr (Chromium), 2014-08-04 15:36:57]

Thanks, Vasilii. LGTM with comments.

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:40: void
DeepCopyMap(const autofill::PasswordFormMap& map,
Please comment on the use and expectations. Things I suggest to mention:
* ret and deleter being output params
* ret keeping the mapping of |map|, but with weak pointers to copies of forms
stored in |map|
* deleter being the owner of the copies (has to outlive |ret|)

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:47:
autofill::PasswordForm* form(new autofill::PasswordForm(*i->second));
Please don't let naked pointers contain new-allocated objects; if someone adds
code between this line and the following, they can introduce leaks.

Instead call new directly inside push_back and use back() to retrieve it for
i->second, or use a scoped_ptr and fill first i->second, and then release the
scoped_ptr into deleter.

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:120:
autofill::ConstPasswordFormMap password_form_map_;
This declaration order means, that new_password_forms_ get deleted before
password_form_map_ on the controller destruction. That can create dangling
pointers -- please swap the declaration order, and also comment on the fact,
that new_password_forms_ can own what is pointed to by password_form_map_, so if
new_password_forms_ gets cleared, then so needs to be cleared
password_form_map_.

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:98:
EXPECT_NE(&test_form(),
Please say in a comment, that you check the pointers to make sure that the
controller has an independently allocated copy.
Also below.

[vasilii, 2014-08-04 16:20:59]

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:40: void
DeepCopyMap(const autofill::PasswordFormMap& map,
On 2014/08/04 15:36:56, vabr (Chromium) wrote:
> Please comment on the use and expectations. Things I suggest to mention:
> * ret and deleter being output params
> * ret keeping the mapping of |map|, but with weak pointers to copies of forms
> stored in |map|
> * deleter being the owner of the copies (has to outlive |ret|)

Done.

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:47:
autofill::PasswordForm* form(new autofill::PasswordForm(*i->second));
On 2014/08/04 15:36:56, vabr (Chromium) wrote:
> Please don't let naked pointers contain new-allocated objects; if someone adds
> code between this line and the following, they can introduce leaks.
> 
> Instead call new directly inside push_back and use back() to retrieve it for
> i->second, or use a scoped_ptr and fill first i->second, and then release the
> scoped_ptr into deleter.

Done.

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:120:
autofill::ConstPasswordFormMap password_form_map_;
On 2014/08/04 15:36:56, vabr (Chromium) wrote:
> This declaration order means, that new_password_forms_ get deleted before
> password_form_map_ on the controller destruction. That can create dangling
> pointers -- please swap the declaration order, and also comment on the fact,
> that new_password_forms_ can own what is pointed to by password_form_map_, so
if
> new_password_forms_ gets cleared, then so needs to be cleared
> password_form_map_.

Done.

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/441643007/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:98:
EXPECT_NE(&test_form(),
On 2014/08/04 15:36:56, vabr (Chromium) wrote:
> Please say in a comment, that you check the pointers to make sure that the
> controller has an independently allocated copy.
> Also below.

Done.

[vasilii, 2014-08-04 16:21:24]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-08-04 16:22:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/441643007/40001

[commit-bot: I haz the power, 2014-08-05 00:08:21]

Change committed as 287412