Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1369173002:
Implement origin-based deletion for passwords in PasswordDefaultStore (Closed)
|
Created:
5 years, 2 months ago by Timo Reimann Modified:
5 years, 2 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, Mike West, mkwst+watchlist-passwords_chromium.org, vabr+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement origin-based deletion for passwords in PasswordDefaultStore
This is the first in a series of CLs designated to introduce support for
origin-based deletion of passwords.
We will tackle one password store implementation at a time, starting with
PasswordDefaultStore.
BUG=113973
TESTS=unit tests
Committed: https://crrev.com/48300fa190a7b2052a981363bc102d2c450122cc
Cr-Commit-Position: refs/heads/master@{#353864}
Patch Set 1 #
Total comments: 27
Patch Set 2 : Incorporate first round of feedback #Patch Set 3 : Incorporate remaining feedback #
Total comments: 11
Patch Set 4 : Address latest comments #Patch Set 5 : Remove redundant ::Time to fix trybot compile errors #Patch Set 6 : Drain message loop during fixture teardown #
Messages
Total messages: 34 (11 generated)
ttr314@googlemail.com changed reviewers: + vasilii@chromium.org
Vasilii, would you mind reviewing my CL? Primarily, I'd be interested to know if you agree to the approach taken. I also think there might be some need to do a bit of refactoring to reduce code duplicates; I'd be happy to do that once we agree on the general direction. I also left some in-code comments already. Thanks. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store.cc:355: } The method shares most logic with RemoveLoginsCreatedBetweenInternal. We could consider moving most of it into a private helper. This could be accomplished by encapsulating the actual call to RemoveLogins*Impl in a closure. Then again, the two methods aren't that big, so I'm wondering if refactoring here would make things harder to understand than necessary. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default.cc:102: } I wasn't completely sure if the approach of removing one login at a time may be inferior from a performance perspective. Apart from trying to keep it simple, my assumption was that there are only few passwords stored per origin on average, so we don't need to fire off too many queries for a single origin. Alternatively, we could collect the IDs of all same-origin forms and create a custom DELETE SQL query that wipes out all matching records in a single bulk operation. (We could also re-implement the same-origin logic in SQL, but personally I'd rather not take that route.)
https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store.cc:355: } On 2015/09/28 18:43:07, Timo Reimann wrote: > The method shares most logic with RemoveLoginsCreatedBetweenInternal. We could > consider moving most of it into a private helper. This could be accomplished by > encapsulating the actual call to RemoveLogins*Impl in a closure. > > Then again, the two methods aren't that big, so I'm wondering if refactoring > here would make things harder to understand than necessary. It's too simple to introduce one more helper. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default.cc:102: } On 2015/09/28 18:43:07, Timo Reimann wrote: > I wasn't completely sure if the approach of removing one login at a time may be > inferior from a performance perspective. Apart from trying to keep it simple, my > assumption was that there are only few passwords stored per origin on average, > so we don't need to fire off too many queries for a single origin. > > Alternatively, we could collect the IDs of all same-origin forms and create a > custom DELETE SQL query that wipes out all matching records in a single bulk > operation. (We could also re-implement the same-origin logic in SQL, but > personally I'd rather not take that route.) I really depends on how you want the feature to work. Is there a UI proposal for this? Currently you use IsSameOriginWith. That means if one may need to run the deletion twice (for HTTP and HTTPS) to accomplish what expected. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:97: return new LoginDatabase(test_login_db_file_path()); This method can be merged to CreateInitializedStore https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:111: const std::string origin_url) { const std::string& https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:304: scoped_ptr<autofill::PasswordForm> form = CreatePasswordForm(origin_url); No need for a pointer. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:310: EXPECT_CALL(observer, OnLoginsChanged(Contains(PasswordStoreChange( It should be exactly one element. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:313: const url::Origin origin((GURL(origin_url))); Does it compile without extra braces? https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:315: base::Time::Max(), base::Closure()); You can use RunLoop and pass run_loop.QuitClosure() to make the test stronger. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:320: base::MessageLoop::current()->RunUntilIdle(); Why is it needed?
First round of (undisputed :-) ) feedback incorporated. Mike, I put you on CC because Vasilii asked for how the feature is going to be used (see comment on password_store_default.cc in first patchset) as it may have an impact on the implementation. Please shed some light if you can. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store.cc:355: } On 2015/09/29 at 13:53:56, vasilii wrote: > On 2015/09/28 18:43:07, Timo Reimann wrote: > > The method shares most logic with RemoveLoginsCreatedBetweenInternal. We could > > consider moving most of it into a private helper. This could be accomplished by > > encapsulating the actual call to RemoveLogins*Impl in a closure. > > > > Then again, the two methods aren't that big, so I'm wondering if refactoring > > here would make things harder to understand than necessary. > > It's too simple to introduce one more helper. Okay. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default.cc:102: } On 2015/09/29 at 13:53:56, vasilii wrote: > On 2015/09/28 18:43:07, Timo Reimann wrote: > > I wasn't completely sure if the approach of removing one login at a time may be > > inferior from a performance perspective. Apart from trying to keep it simple, my > > assumption was that there are only few passwords stored per origin on average, > > so we don't need to fire off too many queries for a single origin. > > > > Alternatively, we could collect the IDs of all same-origin forms and create a > > custom DELETE SQL query that wipes out all matching records in a single bulk > > operation. (We could also re-implement the same-origin logic in SQL, but > > personally I'd rather not take that route.) > > I really depends on how you want the feature to work. Is there a UI proposal for this? > Currently you use IsSameOriginWith. That means if one may need to run the deletion twice (for HTTP and HTTPS) to accomplish what expected. Not sure about any concrete UI proposals, but there are certainly a few use cases: [1] and [2] to allow extension developers more fine-grained control over their privacy data; and Mike West's Clear Site API proposal [3]. Maybe Mike can share more details from his perspective. Personally, I believe that deleting data for just one schema is a valid use case. (Say, an extension that enables the user to delete non-secure HTTP passwords only.) IMHO, if we later run into a case where we need support for broader origin-related operations, we can change/extend the interface once again or put the logic in a layer above the password store. [1] https://code.google.com/p/chromium/issues/detail?id=113621 [2] https://code.google.com/p/chromium/issues/detail?id=78093 [3] https://mikewest.github.io/webappsec/specs/clear-site-data/ https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:97: return new LoginDatabase(test_login_db_file_path()); On 2015/09/29 at 13:53:56, vasilii wrote: > This method can be merged to CreateInitializedStore Done. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:111: const std::string origin_url) { On 2015/09/29 at 13:53:56, vasilii wrote: > const std::string& Done. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:304: scoped_ptr<autofill::PasswordForm> form = CreatePasswordForm(origin_url); On 2015/09/29 at 13:53:56, vasilii (OOO til 08.10) wrote: > No need for a pointer. CreatePasswordForm is essentially a wrapper around CreatePasswordFormFromDataForTesting, so I decided to stick to the same return type for consistency reasons. FWIW, turning this into a variable would move the dereferencing operation from the test case (when calling AddLogin) to the CreatePasswordForm implementation (when returning). On a second glimpse, I noticed there's already a method CreateTestPasswordFormData that serves a related purpose. I'm thinking that we might be better off creating a similar method and composing it with CreatePasswordFormFromDataForTesting too (e.g., CreatePasswordFormFromDataForTesting(CreateTestPasswordFormData(origin_url)); ). We'd then return a pointer for sure, but the code would look more consistent and we could reuse a common base PasswordFormData structure. WDYT? https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:310: EXPECT_CALL(observer, OnLoginsChanged(Contains(PasswordStoreChange( On 2015/09/29 at 13:53:56, vasilii wrote: > It should be exactly one element. Ah, that's a left over from when I had the mock observer registered before AddLogin was called. Done. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:313: const url::Origin origin((GURL(origin_url))); On 2015/09/29 at 13:53:56, vasilii wrote: > Does it compile without extra braces? Unfortunately not: We have a case of most vexing parse here. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:315: base::Time::Max(), base::Closure()); On 2015/09/29 at 13:53:56, vasilii wrote: > You can use RunLoop and pass run_loop.QuitClosure() to make the test stronger. Done. Haven't used RunLoop before -- please double-check I haven't missed anything. Can you explain to me or list some references on why RunLoop is preferred over base::Closure()? I would like to understand what makes it better. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:320: base::MessageLoop::current()->RunUntilIdle(); On 2015/09/29 at 13:53:56, vasilii wrote: > Why is it needed? Frankly, I am not completely sure; I simply copied it over from the existing tests. I assumed it guarantees that the test wouldn't finish before the shutdown sequence was completed, thereby avoiding hard-to-track bugs and flaky cases. My understanding of how to work with the message loop during tests is very rough though, so chances are I am completely wrong. The tests do run just fine when I remove the call to RunUntilIdle(). Do you think it's save to leave it out?
Any updates on this one? Vasilii, can we try to move forward as much as possible without Mike's input? Mike, are you around to provide some feedback? Thanks all.
Vasilii is out of office; I'll do my best to get to this tomorrow. Sorry for the delay thus far. My fault. :/ -Mike On Mon, Oct 5, 2015 at 3:23 PM, <ttr314@googlemail.com> wrote: > Any updates on this one? > > Vasilii, can we try to move forward as much as possible without Mike's > input? > > Mike, are you around to provide some feedback? > > Thanks all. > > https://codereview.chromium.org/1369173002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Regarding the scheme/port treatment I am waiting for the Mike's input. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:304: scoped_ptr<autofill::PasswordForm> form = CreatePasswordForm(origin_url); On 2015/09/29 22:31:42, Timo Reimann wrote: > On 2015/09/29 at 13:53:56, vasilii (OOO til 08.10) wrote: > > No need for a pointer. > > CreatePasswordForm is essentially a wrapper around > CreatePasswordFormFromDataForTesting, so I decided to stick to the same return > type for consistency reasons. FWIW, turning this into a variable would move the > dereferencing operation from the test case (when calling AddLogin) to the > CreatePasswordForm implementation (when returning). > > On a second glimpse, I noticed there's already a method > CreateTestPasswordFormData that serves a related purpose. I'm thinking that we > might be better off creating a similar method and composing it with > CreatePasswordFormFromDataForTesting too (e.g., > > CreatePasswordFormFromDataForTesting(CreateTestPasswordFormData(origin_url)); > > ). We'd then return a pointer for sure, but the code would look more consistent > and we could reuse a common base PasswordFormData structure. WDYT? I welcome the use of CreateTestPasswordFormData in the test. And you can use the scoped pointer. I see it's common for this file. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:315: base::Time::Max(), base::Closure()); On 2015/09/29 22:31:42, Timo Reimann wrote: > On 2015/09/29 at 13:53:56, vasilii wrote: > > You can use RunLoop and pass run_loop.QuitClosure() to make the test stronger. > > Done. Haven't used RunLoop before -- please double-check I haven't missed > anything. > > Can you explain to me or list some references on why RunLoop is preferred over > base::Closure()? I would like to understand what makes it better. Currently you don't test that the callback is called. With run_loop.QuitClosure() you'll actually test it. https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:320: base::MessageLoop::current()->RunUntilIdle(); On 2015/09/29 22:31:42, Timo Reimann wrote: > On 2015/09/29 at 13:53:56, vasilii wrote: > > Why is it needed? > > Frankly, I am not completely sure; I simply copied it over from the existing > tests. I assumed it guarantees that the test wouldn't finish before the shutdown > sequence was completed, thereby avoiding hard-to-track bugs and flaky cases. My > understanding of how to work with the message loop during tests is very rough > though, so chances are I am completely wrong. > > The tests do run just fine when I remove the call to RunUntilIdle(). Do you > think it's save to leave it out? Yes, remove it.
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default.cc:102: } On 2015/09/29 at 22:31:42, Timo Reimann wrote: > On 2015/09/29 at 13:53:56, vasilii wrote: > > On 2015/09/28 18:43:07, Timo Reimann wrote: > > > I wasn't completely sure if the approach of removing one login at a time may be > > > inferior from a performance perspective. Apart from trying to keep it simple, my > > > assumption was that there are only few passwords stored per origin on average, > > > so we don't need to fire off too many queries for a single origin. > > > > > > Alternatively, we could collect the IDs of all same-origin forms and create a > > > custom DELETE SQL query that wipes out all matching records in a single bulk > > > operation. (We could also re-implement the same-origin logic in SQL, but > > > personally I'd rather not take that route.) > > > > I really depends on how you want the feature to work. Is there a UI proposal for this? > > Currently you use IsSameOriginWith. That means if one may need to run the deletion twice (for HTTP and HTTPS) to accomplish what expected. > > Not sure about any concrete UI proposals, but there are certainly a few use cases: [1] and [2] to allow extension developers more fine-grained control over their privacy data; and Mike West's Clear Site API proposal [3]. Maybe Mike can share more details from his perspective. > > Personally, I believe that deleting data for just one schema is a valid use case. (Say, an extension that enables the user to delete non-secure HTTP passwords only.) IMHO, if we later run into a case where we need support for broader origin-related operations, we can change/extend the interface once again or put the logic in a layer above the password store. > > [1] https://code.google.com/p/chromium/issues/detail?id=113621 > [2] https://code.google.com/p/chromium/issues/detail?id=78093 > [3] https://mikewest.github.io/webappsec/specs/clear-site-data/ I'd suggest that we shouldn't complicate the implementation. Origins are (scheme, host, port) tuples. Removing data for an origin should respect that tuple. If we decide that we wish to build a feature that goes beyond an origin (removing secure and non-secure variants of a host, for instance, or spanning all ports), we can implement that with a simple loop on top of the origin-based primitive. If that ends up being too slow, we can build something to support it. So: * For data types which respect the origin model, we should do the same. * For data types which do not (cookies), we'll need to be a bit laxer. Here, we should respect the complete origin.
https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:315: base::Time::Max(), base::Closure()); On 2015/10/08 at 13:53:38, vasilii wrote: > On 2015/09/29 22:31:42, Timo Reimann wrote: > > On 2015/09/29 at 13:53:56, vasilii wrote: > > > You can use RunLoop and pass run_loop.QuitClosure() to make the test stronger. > > > > Done. Haven't used RunLoop before -- please double-check I haven't missed > > anything. > > > > Can you explain to me or list some references on why RunLoop is preferred over > > base::Closure()? I would like to understand what makes it better. > > Currently you don't test that the callback is called. With run_loop.QuitClosure() you'll actually test it. Isn't the expectation set via EXPECT_CALL(observer, OnLoginsChanged(ElementsAre(PasswordStoreChange( PasswordStoreChange::REMOVE, *form)))); taking care of verifying that the callback is being called?
The last patch should address all remaining comments: - Replace CreatePasswordForm() method by composition CreatePasswordFormFromDataForTesting(CreateTestPasswordFormDataByOrigin(origin_url)). - Skip unnecessary calls to base::MessageLoop::current()->RunUntilIdle() both in new and existing tests. - Fix inconsistent order of calls to RemoveObserver and Shutdown in RemoveLoginsByOriginAndTimeImpl_NotWithinTimeInterval (wasn't addressed). Please let me know if you find anything missing. Thanks! https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:304: scoped_ptr<autofill::PasswordForm> form = CreatePasswordForm(origin_url); On 2015/10/08 at 13:53:38, vasilii wrote: > On 2015/09/29 22:31:42, Timo Reimann wrote: > > On 2015/09/29 at 13:53:56, vasilii (OOO til 08.10) wrote: > > > No need for a pointer. > > > > CreatePasswordForm is essentially a wrapper around > > CreatePasswordFormFromDataForTesting, so I decided to stick to the same return > > type for consistency reasons. FWIW, turning this into a variable would move the > > dereferencing operation from the test case (when calling AddLogin) to the > > CreatePasswordForm implementation (when returning). > > > > On a second glimpse, I noticed there's already a method > > CreateTestPasswordFormData that serves a related purpose. I'm thinking that we > > might be better off creating a similar method and composing it with > > CreatePasswordFormFromDataForTesting too (e.g., > > > > CreatePasswordFormFromDataForTesting(CreateTestPasswordFormData(origin_url)); > > > > ). We'd then return a pointer for sure, but the code would look more consistent > > and we could reuse a common base PasswordFormData structure. WDYT? > > I welcome the use of CreateTestPasswordFormData in the test. And you can use the scoped pointer. I see it's common for this file. Done.
https://codereview.chromium.org/1369173002/diff/1/components/password_manager... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1369173002/diff/1/components/password_manager... components/password_manager/core/browser/password_store_default_unittest.cc:315: base::Time::Max(), base::Closure()); On 2015/10/10 20:23:05, Timo Reimann wrote: > On 2015/10/08 at 13:53:38, vasilii wrote: > > On 2015/09/29 22:31:42, Timo Reimann wrote: > > > On 2015/09/29 at 13:53:56, vasilii wrote: > > > > You can use RunLoop and pass run_loop.QuitClosure() to make the test > stronger. > > > > > > Done. Haven't used RunLoop before -- please double-check I haven't missed > > > anything. > > > > > > Can you explain to me or list some references on why RunLoop is preferred > over > > > base::Closure()? I would like to understand what makes it better. > > > > Currently you don't test that the callback is called. With > run_loop.QuitClosure() you'll actually test it. > > Isn't the expectation set via > > EXPECT_CALL(observer, OnLoginsChanged(ElementsAre(PasswordStoreChange( > PasswordStoreChange::REMOVE, *form)))); > > taking care of verifying that the callback is being called? It's a different thing. You test that the observers are notified on the changes (which can be a result of any call). I propose in addition to that test that the callback parameter is called. The callback signals to the caller that the method has finished. https://codereview.chromium.org/1369173002/diff/40001/components/password_man... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:29: using testing::SizeIs; unused https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:84: const std::string& origin_url) { I'm worried about the lifetime of origin_url. You save raw pointers to its internals in PasswordFormData. If somebody calls CreateTestPasswordFormDataByOrigin("hello") then the test will crash. You need to pass const char*. https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:118: base::ThreadTaskRunnerHandle::Get(), make_scoped_ptr(database))); Why not creating the database here? https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:295: const std::string origin_url = "http://foo.example.com"; const char[] https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:311: base::MessageLoop::current()->RunUntilIdle(); Nope. RunLoop::Run should be called here. That's how you test that the callback is actually called. Same below.
https://codereview.chromium.org/1369173002/diff/40001/components/password_man... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:29: using testing::SizeIs; On 2015/10/12 at 17:01:29, vasilii wrote: > unused Thanks, another leftover. cpplint doesn't seem to catch unused using statements. If there's a tool/option to detect such issues, I'd be happy to know. https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:84: const std::string& origin_url) { On 2015/10/12 at 17:01:30, vasilii wrote: > I'm worried about the lifetime of origin_url. You save raw pointers to its internals in PasswordFormData. If somebody calls CreateTestPasswordFormDataByOrigin("hello") then the test will crash. > You need to pass const char*. It didn't crash on me when I passed in a string literal; I guess I was in the happy path of undefined behavior. Done. https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:118: base::ThreadTaskRunnerHandle::Get(), make_scoped_ptr(database))); On 2015/10/12 at 17:01:30, vasilii wrote: > Why not creating the database here? Personally I found it harder to read due to the deep level of nested calls involved (store initialized with PasswordStoreDefault initialized with make_scoped_ptr initialized with a LoginDatabase instance initialized with a file path). I made the change nevertheless; if you prefer this approach I'd be happy to stick to it. https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:295: const std::string origin_url = "http://foo.example.com"; On 2015/10/12 at 17:01:30, vasilii wrote: > const char[] Done for all origin_url instances across all new tests. https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:311: base::MessageLoop::current()->RunUntilIdle(); On 2015/10/12 at 17:01:30, vasilii wrote: > Nope. RunLoop::Run should be called here. That's how you test that the callback is actually called. > Same below. Gotcha. Done again for this and all other new tests.
lgtm https://codereview.chromium.org/1369173002/diff/40001/components/password_man... File components/password_manager/core/browser/password_store_default_unittest.cc (right): https://codereview.chromium.org/1369173002/diff/40001/components/password_man... components/password_manager/core/browser/password_store_default_unittest.cc:84: const std::string& origin_url) { On 2015/10/12 22:08:06, Timo Reimann wrote: > On 2015/10/12 at 17:01:30, vasilii wrote: > > I'm worried about the lifetime of origin_url. You save raw pointers to its > internals in PasswordFormData. If somebody calls > CreateTestPasswordFormDataByOrigin("hello") then the test will crash. > > You need to pass const char*. > > It didn't crash on me when I passed in a string literal; I guess I was in the > happy path of undefined behavior. > > Done. Because your string was alive during the test.
The CQ bit was checked by ttr314@googlemail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1369173002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1369173002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by ttr314@googlemail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1369173002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1369173002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ttr314@googlemail.com
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/1369173002/#ps80001 (title: "Remove redundant ::Time to fix trybot compile errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1369173002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1369173002/80001
I guess PasswordStore is still referenced by the message loop after the test. You may try to put base::MessageLoop::current()->RunUntilIdle(); in the TearDown before deleting the directory.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2015/10/13 at 17:33:34, vasilii wrote: > I guess PasswordStore is still referenced by the message loop after the test. You may try to put base::MessageLoop::current()->RunUntilIdle(); in the TearDown before deleting the directory. Would make sense as we decided previously to remove the call to RunUntilIdle() following the store shutdown with each test. I'd actually prefer the call to be locate inside TearDown since it expresses the purpose more clearly IMHO. Let's see if it works out as desired.
The CQ bit was checked by ttr314@googlemail.com
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/1369173002/#ps100001 (title: "Drain message loop during fixture teardown")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1369173002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1369173002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/48300fa190a7b2052a981363bc102d2c450122cc Cr-Commit-Position: refs/heads/master@{#353864} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
