Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2265103002:
LoginDatabase::Init: rollback before closing a DB (Closed)
|
Created:
4 years, 3 months ago by vabr (Chromium) Modified:
4 years, 3 months ago Reviewers:
vasilii CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLoginDatabase::Init: rollback before closing a DB
Currently, LoginDatabase::Init closes its sql::Connection on error. It also
uses an sql::Transaction to roll back changes made so far. Unfortunately, it
does so in the wrong order.
This CL fixes that and adds a test for it.
BUG=639803
Committed: https://crrev.com/7bdd9928d167b3de18f68219a3ea0b817cb99ad3
Cr-Commit-Position: refs/heads/master@{#413460}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Review #Patch Set 3 : CreateUniqueTempDir #
Total comments: 1
Messages
Total messages: 21 (11 generated)
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
vabr@chromium.org changed reviewers: + vasilii@chromium.org
Hi Vasilii, PTAL. I can confirm that the test crashed without the fix and passed with it. Thanks! Vaclav
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2265103002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database.cc (right): https://codereview.chromium.org/2265103002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:534: transaction.Rollback(); If it didn't begin there is nothing to rollback. The call would DCHECK. https://codereview.chromium.org/2265103002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:611: transaction.Rollback(); That's a bug to call Rollback() after Commit(). https://codereview.chromium.org/2265103002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/2265103002/diff/1/components/password_manager... components/password_manager/core/browser/login_database_unittest.cc:1501: } Not sure why you need those brackets.
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Thanks Vasilii, for catching so many of my mistakes. I guess I was a bit trigger-happy. :) PTAL. Cheers, Vaclav https://codereview.chromium.org/2265103002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database.cc (right): https://codereview.chromium.org/2265103002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:534: transaction.Rollback(); On 2016/08/22 14:31:54, vasilii wrote: > If it didn't begin there is nothing to rollback. The call would DCHECK. Done. https://codereview.chromium.org/2265103002/diff/1/components/password_manager... components/password_manager/core/browser/login_database.cc:611: transaction.Rollback(); On 2016/08/22 14:31:54, vasilii wrote: > That's a bug to call Rollback() after Commit(). Done. https://codereview.chromium.org/2265103002/diff/1/components/password_manager... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/2265103002/diff/1/components/password_manager... components/password_manager/core/browser/login_database_unittest.cc:1501: } On 2016/08/22 14:31:54, vasilii wrote: > Not sure why you need those brackets. I don't, removed.
lgtm
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by vabr@chromium.org
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
One more mistake, but a straightforward one to fix. I'm going to continue landing, please let me know if that's not OK. Cheers, Vaclav https://codereview.chromium.org/2265103002/diff/40001/components/password_man... File components/password_manager/core/browser/login_database_unittest.cc (right): https://codereview.chromium.org/2265103002/diff/40001/components/password_man... components/password_manager/core/browser/login_database_unittest.cc:1483: ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); I forgot to initialise the temp dir. It passed on most bots, because it returned "" as the path, but fortunately failed on iOS simulator. Fixed now.
The CQ bit was checked by vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2265103002/#ps40001 (title: "CreateUniqueTempDir")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== LoginDatabase::Init: rollback before closing a DB Currently, LoginDatabase::Init closes its sql::Connection on error. It also uses an sql::Transaction to roll back changes made so far. Unfortunately, it does so in the wrong order. This CL fixes that and adds a test for it. BUG=639803 ========== to ========== LoginDatabase::Init: rollback before closing a DB Currently, LoginDatabase::Init closes its sql::Connection on error. It also uses an sql::Transaction to roll back changes made so far. Unfortunately, it does so in the wrong order. This CL fixes that and adds a test for it. BUG=639803 Committed: https://crrev.com/7bdd9928d167b3de18f68219a3ea0b817cb99ad3 Cr-Commit-Position: refs/heads/master@{#413460} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7bdd9928d167b3de18f68219a3ea0b817cb99ad3 Cr-Commit-Position: refs/heads/master@{#413460} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
