Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
202903002:
[Password Manager] Fix possible crash when updating imported credentials. (Closed)
|
Created:
6 years, 9 months ago by Garrett Casto Modified:
6 years, 9 months ago Reviewers:
vasilii CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Password Manager] Fix possible crash when updating imported credentials.
See bug for more details.
BUG=349138
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258063
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Merge #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/202903002/diff/20001/components/password_mana... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/202903002/diff/20001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:740: ASSERT_EQ(1U, result.size()); EXPECT_EQ https://codereview.chromium.org/202903002/diff/20001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:741: } It's worth to check that result[0] is complete.
https://codereview.chromium.org/202903002/diff/20001/components/password_mana... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/202903002/diff/20001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:740: ASSERT_EQ(1U, result.size()); On 2014/03/18 11:42:22, vasilii wrote: > EXPECT_EQ You should leave ASSERT_EQ if you'll be checking result[0] below.
https://codereview.chromium.org/202903002/diff/20001/components/password_mana... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/202903002/diff/20001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:741: } On 2014/03/18 11:42:22, vasilii wrote: > It's worth to check that result[0] is complete. Done.
https://codereview.chromium.org/202903002/diff/40001/components/password_mana... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:723: // Save a complete version of the previous form. Both forms could exists if "exist" https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:729: complete_form.password_element = ASCIIToUTF16("my_password"); Better use "username_element" and "password_element" strings. https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:732: It would be great to check that there are 2 forms in the db. I see that LoginDatabase::AddLogin() has the "REPLACE" semantic too. https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:738: std::vector<autofill::PasswordForm*> result; Optional: use ScopedVector<autofill::PasswordForm>. https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:744: expected_form.password_value.clear(); Why don't we retrieve the password on Mac?
https://codereview.chromium.org/202903002/diff/40001/components/password_mana... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:723: // Save a complete version of the previous form. Both forms could exists if On 2014/03/18 17:26:54, vasilii wrote: > "exist" Done. https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:729: complete_form.password_element = ASCIIToUTF16("my_password"); On 2014/03/18 17:26:54, vasilii wrote: > Better use "username_element" and "password_element" strings. Done. https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:732: On 2014/03/18 17:26:54, vasilii wrote: > It would be great to check that there are 2 forms in the db. I see that > LoginDatabase::AddLogin() has the "REPLACE" semantic too. Done. https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:738: std::vector<autofill::PasswordForm*> result; On 2014/03/18 17:26:54, vasilii wrote: > Optional: use ScopedVector<autofill::PasswordForm>. Done. https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:744: expected_form.password_value.clear(); On 2014/03/18 17:26:54, vasilii wrote: > Why don't we retrieve the password on Mac? Because we just store metadata in SQL on Mac, passwords are stored in the keychain.
lgtm https://codereview.chromium.org/202903002/diff/40001/components/password_mana... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:744: expected_form.password_value.clear(); On 2014/03/18 17:50:46, Garrett Casto wrote: > On 2014/03/18 17:26:54, vasilii wrote: > > Why don't we retrieve the password on Mac? > > Because we just store metadata in SQL on Mac, passwords are stored in the > keychain. Add a comment please.
https://codereview.chromium.org/202903002/diff/40001/components/password_mana... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/202903002/diff/40001/components/password_mana... components/password_manager/core/browser/login_database_unittest.cc:744: expected_form.password_value.clear(); On 2014/03/18 19:02:52, vasilii wrote: > On 2014/03/18 17:50:46, Garrett Casto wrote: > > On 2014/03/18 17:26:54, vasilii wrote: > > > Why don't we retrieve the password on Mac? > > > > Because we just store metadata in SQL on Mac, passwords are stored in the > > keychain. > > Add a comment please. Done.
The CQ bit was checked by gcasto@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gcasto@chromium.org/202903002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by gcasto@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gcasto@chromium.org/202903002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by gcasto@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gcasto@chromium.org/202903002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by gcasto@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gcasto@chromium.org/202903002/100001
Message was sent while issue was closed.
Change committed as 258063 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
