Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832064:
contacts: Add contacts::ContactDatabase. (Closed)
|
Created:
8 years, 4 months ago by Daniel Erat Modified:
8 years, 4 months ago CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, kochi Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncontacts: Add contacts::ContactDatabase.
This adds a class that uses leveldb to save and load
contacts::Contact protocol buffers to/from disk.
BUG=128805
TEST=none
TBR=ben@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149299
Patch Set 1 #Patch Set 2 : minor updates #
Total comments: 15
Patch Set 3 : apply review feedback and update DEPS #Patch Set 4 : try clearing bitmap pixels to see if it fixes some valgrind errors #
Total comments: 17
Patch Set 5 : more review feedback: update callbacks #Patch Set 6 : add a few more DCHECKs #
Total comments: 7
Patch Set 7 : add callback-running methods #
Total comments: 2
Patch Set 8 : one parameter per line #
Messages
Total messages: 16 (0 generated)
http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... File chrome/browser/chromeos/contacts/contact_database.cc (right): http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.cc:31: condition_variable_.Wait(); I'd suggest not to do this. In gdata_cache.cc we solved the shutdown problem as follows: void GDataCache::DestroyOnUIThread() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); // Invalidate the weak pointer. ui_weak_ptr_factory_.InvalidateWeakPtrs(); // Destroy myself on the blocking pool. blocking_task_runner_->PostTask( FROM_HERE, base::Bind(&GDataCache::Destroy, base::Unretained(this))); } void GDataCache::Destroy() { AssertOnSequencedWorkerPool(); delete this; } GDataCache::~GDataCache() { AssertOnSequencedWorkerPool(); } You may wonrder if Destroy is guaranteed to be called. The answer is yes. Thanks to the sequenced task runner, all tasks posted before Destory are also guaranteed to run before Destroy. browser_thread.h says: // Simplified wrappers for posting to the blocking thread pool. Use this // for doing things like blocking I/O. // .. // These tasks are guaranteed to run before shutdown. // static bool PostBlockingPoolTask(const tracked_objects::Location& from_here, const base::Closure& task); .. // Returns the thread pool used for blocking file I/O. Use this object to // perform random blocking operations such as file writes or querying the // Windows registry. static base::SequencedWorkerPool* GetBlockingPool(); From the comments, it's not clear if GetBlockingPool() also has the same guarantee, but looking at the .cc file, it's the same thing. I'll probably add some comment there. BTW, Destroy() seems to be unnecessary. DeleteSoon() should be used instead. http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.cc:38: BrowserThread::FILE, FROM_HERE, FILE thread is supposed to be dead. Please use SequencedWorkerPool instead. You can create one like: base::SequencedWorkerPool* blocking_pool = BrowserThread::GetBlockingPool(); blocking_task_runner_ = blocking_pool->GetSequencedTaskRunner( blocking_pool->GetSequenceToken()); http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.cc:45: SaveCallback callback) { You might want to add something like: DCHECK(CurrentlyOn(BrowserThread::UI)) http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.cc:91: // Delete the existing database and try again (just once, though). oh this is a nice technique! http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... File chrome/browser/chromeos/contacts/contact_database.h (right): http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.h:33: typedef base::Callback<void(bool)> InitCallback; Having a parameter name would make it a bit more informative. typedef base::Callback<void(bool success)> InitCallback; http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.h:114: base::ConditionVariable condition_variable_; I'm very wary of Lock and friends. Can you go without them? Here's some background: In GDataFileSystem, Lock and WaitableEvent were used, and these were a source of super nasty issues, so we decided to get rid of them and it took a huge amount of work. Once these are gone, everyone was happy. http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... File chrome/browser/chromeos/contacts/contact_database_unittest.cc (right): http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database_unittest.cc:93: message_loop_.Quit(); Just FYI. In gdata_cache_unittest.cc, gdata::test::RunBlockingPoolTask() is heavily used and we don't manually Quit() message loops. // Runs a task posted to the blocking pool, including subquent tasks posted // to the UI message loop and the blocking pool. // // A task is often posted to the blocking pool with PostTaskAndReply(). In // that case, a task is posted back to the UI message loop, which can again // post a task to the blocking pool. This function processes these tasks // repeatedly. void RunBlockingPoolTask();
https://chromiumcodereview.appspot.com/10832064/diff/2001/chrome/browser/chro... File chrome/browser/chromeos/contacts/contact_database.cc (right): https://chromiumcodereview.appspot.com/10832064/diff/2001/chrome/browser/chro... chrome/browser/chromeos/contacts/contact_database.cc:31: condition_variable_.Wait(); On 2012/07/30 17:15:07, satorux1 wrote: > I'd suggest not to do this. In gdata_cache.cc we solved the shutdown problem as > follows: > > void GDataCache::DestroyOnUIThread() { > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); > > // Invalidate the weak pointer. > ui_weak_ptr_factory_.InvalidateWeakPtrs(); > > // Destroy myself on the blocking pool. > blocking_task_runner_->PostTask( > FROM_HERE, > base::Bind(&GDataCache::Destroy, > base::Unretained(this))); > } > > > void GDataCache::Destroy() { > AssertOnSequencedWorkerPool(); > delete this; > } > > > GDataCache::~GDataCache() { > AssertOnSequencedWorkerPool(); > } > > You may wonrder if Destroy is guaranteed to be called. The answer is yes. Thanks > to the sequenced task runner, all tasks posted before Destory are also > guaranteed to run before Destroy. > > > browser_thread.h says: > > // Simplified wrappers for posting to the blocking thread pool. Use this > // for doing things like blocking I/O. > // > .. > // These tasks are guaranteed to run before shutdown. > // > static bool PostBlockingPoolTask(const tracked_objects::Location& from_here, > const base::Closure& task); > .. > > // Returns the thread pool used for blocking file I/O. Use this object to > // perform random blocking operations such as file writes or querying the > // Windows registry. > static base::SequencedWorkerPool* GetBlockingPool(); > > From the comments, it's not clear if GetBlockingPool() also has the same > guarantee, but looking at the .cc file, it's the same thing. I'll probably add > some comment there. Thanks, I didn't know about this, but it's very useful. Updated. > BTW, Destroy() seems to be unnecessary. DeleteSoon() should be used instead. I'm not sure that DeleteSoon() can be used here without making the d'tor public, which seems unsafe. https://chromiumcodereview.appspot.com/10832064/diff/2001/chrome/browser/chro... chrome/browser/chromeos/contacts/contact_database.cc:38: BrowserThread::FILE, FROM_HERE, On 2012/07/30 17:15:07, satorux1 wrote: > FILE thread is supposed to be dead. Please use SequencedWorkerPool instead. You > can create one like: > > > base::SequencedWorkerPool* blocking_pool = BrowserThread::GetBlockingPool(); > blocking_task_runner_ = blocking_pool->GetSequencedTaskRunner( > blocking_pool->GetSequenceToken()); Done. https://chromiumcodereview.appspot.com/10832064/diff/2001/chrome/browser/chro... chrome/browser/chromeos/contacts/contact_database.cc:45: SaveCallback callback) { On 2012/07/30 17:15:07, satorux1 wrote: > You might want to add something like: > > DCHECK(CurrentlyOn(BrowserThread::UI)) Done. https://chromiumcodereview.appspot.com/10832064/diff/2001/chrome/browser/chro... File chrome/browser/chromeos/contacts/contact_database.h (right): https://chromiumcodereview.appspot.com/10832064/diff/2001/chrome/browser/chro... chrome/browser/chromeos/contacts/contact_database.h:33: typedef base::Callback<void(bool)> InitCallback; On 2012/07/30 17:15:07, satorux1 wrote: > Having a parameter name would make it a bit more informative. > > typedef base::Callback<void(bool success)> InitCallback; Thanks, didn't know that this works! Done. https://chromiumcodereview.appspot.com/10832064/diff/2001/chrome/browser/chro... chrome/browser/chromeos/contacts/contact_database.h:114: base::ConditionVariable condition_variable_; On 2012/07/30 17:15:07, satorux1 wrote: > I'm very wary of Lock and friends. Can you go without them? > > Here's some background: > > In GDataFileSystem, Lock and WaitableEvent were used, and these were a source of > super nasty issues, so we decided to get rid of them and it took a huge amount > of work. Once these are gone, everyone was happy. Done. https://chromiumcodereview.appspot.com/10832064/diff/2001/chrome/browser/chro... File chrome/browser/chromeos/contacts/contact_database_unittest.cc (right): https://chromiumcodereview.appspot.com/10832064/diff/2001/chrome/browser/chro... chrome/browser/chromeos/contacts/contact_database_unittest.cc:93: message_loop_.Quit(); On 2012/07/30 17:15:07, satorux1 wrote: > Just FYI. In gdata_cache_unittest.cc, gdata::test::RunBlockingPoolTask() is > heavily used and we don't manually Quit() message loops. > > // Runs a task posted to the blocking pool, including subquent tasks posted > // to the UI message loop and the blocking pool. > // > // A task is often posted to the blocking pool with PostTaskAndReply(). In > // that case, a task is posted back to the UI message loop, which can again > // post a task to the blocking pool. This function processes these tasks > // repeatedly. > void RunBlockingPoolTask(); Do you think it's worthwhile to move this to content/?
LGTM with some nits. Sorry for the belated response. http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... File chrome/browser/chromeos/contacts/contact_database_unittest.cc (right): http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database_unittest.cc:93: message_loop_.Quit(); On 2012/07/30 22:12:09, Daniel Erat wrote: > On 2012/07/30 17:15:07, satorux1 wrote: > > Just FYI. In gdata_cache_unittest.cc, gdata::test::RunBlockingPoolTask() is > > heavily used and we don't manually Quit() message loops. > > > > // Runs a task posted to the blocking pool, including subquent tasks posted > > > // to the UI message loop and the blocking pool. > > > // > > > // A task is often posted to the blocking pool with PostTaskAndReply(). In > > > // that case, a task is posted back to the UI message loop, which can again > > > // post a task to the blocking pool. This function processes these tasks > > > // repeatedly. > > > void RunBlockingPoolTask(); > > Do you think it's worthwhile to move this to content/? That's a good question. Not sure if it's worthwhile. Maybe we could move it to chrome/browser/chromeos as a first step, so you can use it here. having users in two places would make it easier to move to a lower level place. http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database.cc (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:25: ContactDatabase::ContactDatabase() { might want to add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); to ensure that the object is created on the expected thread. http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database.h (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.h:14: #include "base/memory/ref_counted.h" is this needed? http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database_unittest.cc (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database_unittest.cc:53: testing::Test::TearDown(); what does it do? maybe shutdown the blocking pool? might want to add some comment why we need this.
http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database.cc (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:52: task_runner_->PostNonNestableTask( You might want to use PostTaskAndReply instead. That's an idiom to post a task to another thread, and the process the result from the task http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:170: base::Bind(callback, status.ok())); This may be potentially dangerous. by the time |callback| is run on UI thread, the ContactDatabase object may be already deleted on the blocking pool thread. If |callback| uses ContactDatabase object, it'll cause a use-after-free. To solve this problem, I think you can use PosTaskAndReply like: PostTaskAndReply( FROM_HERE, base::Bind(&ContactDatabase::SaveContactsFromTaskRunner, base::Unretained(this), ...), base::Bind(&ContactDatabase::RunCallback, weak_ptr_factory_.GetWeakPtr(), callback)); Then, add the following to DestroyOnUIThread() weak_ptr_factory_.InvalidateWeakPtrs(); This way, the 2nd callback passed to PostTaskAndReply won't be run after the point that the weak pointers are invalidated.
http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... File chrome/browser/chromeos/contacts/contact_database_unittest.cc (right): http://codereview.chromium.org/10832064/diff/2001/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database_unittest.cc:93: message_loop_.Quit(); On 2012/07/31 07:26:30, satorux1 wrote: > On 2012/07/30 22:12:09, Daniel Erat wrote: > > On 2012/07/30 17:15:07, satorux1 wrote: > > > Just FYI. In gdata_cache_unittest.cc, gdata::test::RunBlockingPoolTask() is > > > heavily used and we don't manually Quit() message loops. > > > > > > // Runs a task posted to the blocking pool, including subquent tasks posted > > > > > > // to the UI message loop and the blocking pool. > > > > > > // > > > > > > // A task is often posted to the blocking pool with PostTaskAndReply(). In > > > > > > // that case, a task is posted back to the UI message loop, which can again > > > > > > // post a task to the blocking pool. This function processes these tasks > > > > > > // repeatedly. > > > > > > void RunBlockingPoolTask(); > > > > Do you think it's worthwhile to move this to content/? > > That's a good question. Not sure if it's worthwhile. Maybe we could move it to > chrome/browser/chromeos as a first step, so you can use it here. having users in > two places would make it easier to move to a lower level place. Does something like chrome/browser/chromeos/test_util.h/cc, with a chromeos::test namespace, seem like the right place? http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database.cc (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:25: ContactDatabase::ContactDatabase() { On 2012/07/31 07:26:30, satorux1 wrote: > might want to add > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); > > to ensure that the object is created on the expected thread. Done. http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:52: task_runner_->PostNonNestableTask( On 2012/07/31 16:19:19, satorux1 wrote: > You might want to use PostTaskAndReply instead. That's an idiom to post a task > to another thread, and the process the result from the task Interesting. Setting the reply's parameters seems a bit tricky. I suppose I'd need to change LoadCallback to: base::Callback<void(scoped_refptr<bool> success, scoped_refptr<ScopedVector<Contact> >)> and then pass the scoped_refptrs to LoadContactsFromTaskRunner() so it can update them, right? http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:170: base::Bind(callback, status.ok())); On 2012/07/31 16:19:19, satorux1 wrote: > This may be potentially dangerous. by the time |callback| is run on UI thread, > the ContactDatabase object may be already deleted on the blocking pool thread. > If |callback| uses ContactDatabase object, it'll cause a use-after-free. > > To solve this problem, I think you can use PosTaskAndReply like: > > PostTaskAndReply( > FROM_HERE, > base::Bind(&ContactDatabase::SaveContactsFromTaskRunner, > base::Unretained(this), ...), > base::Bind(&ContactDatabase::RunCallback, > weak_ptr_factory_.GetWeakPtr(), callback)); > > Then, add the following to DestroyOnUIThread() > > weak_ptr_factory_.InvalidateWeakPtrs(); > > This way, the 2nd callback passed to PostTaskAndReply won't be run after the > point that the weak pointers are invalidated. > > I'm not convinced of the risk -- if we initiate deletion of the ContactDatabase from the UI thread, we shouldn't be trying to use it from the UI thread later. That said, I'm not opposed to what you're suggesting. Were you envisioning RunCallback() as just calling callback.Run()? Note that a "success" parameter needs to be passed to the callback in this case (although maybe that should be a pre-bound scoped_refptr, as described above). http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database.h (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.h:14: #include "base/memory/ref_counted.h" On 2012/07/31 07:26:30, satorux1 wrote: > is this needed? Yes, for scoped_refptr. :-) http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database_unittest.cc (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database_unittest.cc:53: testing::Test::TearDown(); On 2012/07/31 07:26:30, satorux1 wrote: > what does it do? maybe shutdown the blocking pool? might want to add some > comment why we need this. I'll remove these; testing::Test appears to have empty implementations.
http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database.cc (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:52: task_runner_->PostNonNestableTask( On 2012/07/31 16:44:12, Daniel Erat wrote: > On 2012/07/31 16:19:19, satorux1 wrote: > > You might want to use PostTaskAndReply instead. That's an idiom to post a task > > to another thread, and the process the result from the task > > Interesting. Setting the reply's parameters seems a bit tricky. I suppose I'd > need to change LoadCallback to: > > base::Callback<void(scoped_refptr<bool> success, > scoped_refptr<ScopedVector<Contact> >)> > > and then pass the scoped_refptrs to LoadContactsFromTaskRunner() so it can > update them, right? No scoped_refptr please. :) Usually, the result is passed from the 1st callback to the 2nd callback like: bool* success = new bool(false); PostTaskAndReply( FROM_HERE, base::Bind(&DoSomehing, success); base::Bind(&ProcessResult, base::Owned(success))); using base::Owned(), success is guaranteed to be deleted. http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:170: base::Bind(callback, status.ok())); On 2012/07/31 16:44:12, Daniel Erat wrote: > On 2012/07/31 16:19:19, satorux1 wrote: > > This may be potentially dangerous. by the time |callback| is run on UI thread, > > the ContactDatabase object may be already deleted on the blocking pool thread. > > If |callback| uses ContactDatabase object, it'll cause a use-after-free. > > > > To solve this problem, I think you can use PosTaskAndReply like: > > > > PostTaskAndReply( > > FROM_HERE, > > base::Bind(&ContactDatabase::SaveContactsFromTaskRunner, > > base::Unretained(this), ...), > > base::Bind(&ContactDatabase::RunCallback, > > weak_ptr_factory_.GetWeakPtr(), callback)); > > > > Then, add the following to DestroyOnUIThread() > > > > weak_ptr_factory_.InvalidateWeakPtrs(); > > > > This way, the 2nd callback passed to PostTaskAndReply won't be run after the > > point that the weak pointers are invalidated. > > > > > > I'm not convinced of the risk -- if we initiate deletion of the ContactDatabase > from the UI thread, we shouldn't be trying to use it from the UI thread later. > That said, I'm not opposed to what you're suggesting. Were you envisioning > RunCallback() as just calling callback.Run()? Note that a "success" parameter > needs to be passed to the callback in this case (although maybe that should be a > pre-bound scoped_refptr, as described above). Yes, RunCallback() just runs callback.Run(). "success" parameter can be passed to RunCallback() as a pointer as described above. BTW, callback accessing a deleted object may not be a real risk here, but it's not uncommon for the callback to access the object that ran the callback, like: some_reader_->Read(base::Bind(&OnRead, ...)); SomeClass::OnRead(...) { if (size == 0) { // All bytes read. Close the reader. some_reader_->Close(); } ... } http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database.h (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.h:14: #include "base/memory/ref_counted.h" On 2012/07/31 16:44:12, Daniel Erat wrote: > On 2012/07/31 07:26:30, satorux1 wrote: > > is this needed? > > Yes, for scoped_refptr. :-) oh you are right. It's used for the task runner. I was wary of making classes ref counted (my professor willchan taught me not to do it in most cases), and checked that ContactDatabase* weren't ref counted. :)
http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database.cc (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:52: task_runner_->PostNonNestableTask( On 2012/07/31 17:34:26, satorux1 wrote: > On 2012/07/31 16:44:12, Daniel Erat wrote: > > On 2012/07/31 16:19:19, satorux1 wrote: > > > You might want to use PostTaskAndReply instead. That's an idiom to post a > task > > > to another thread, and the process the result from the task > > > > Interesting. Setting the reply's parameters seems a bit tricky. I suppose > I'd > > need to change LoadCallback to: > > > > base::Callback<void(scoped_refptr<bool> success, > > scoped_refptr<ScopedVector<Contact> >)> > > > > and then pass the scoped_refptrs to LoadContactsFromTaskRunner() so it can > > update them, right? > > No scoped_refptr please. :) Usually, the result is passed from the 1st callback > to the 2nd callback like: > > bool* success = new bool(false); > PostTaskAndReply( > FROM_HERE, > base::Bind(&DoSomehing, success); > base::Bind(&ProcessResult, base::Owned(success))); > > using base::Owned(), success is guaranteed to be deleted. Nice! Done. http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:170: base::Bind(callback, status.ok())); On 2012/07/31 17:34:26, satorux1 wrote: > On 2012/07/31 16:44:12, Daniel Erat wrote: > > On 2012/07/31 16:19:19, satorux1 wrote: > > > This may be potentially dangerous. by the time |callback| is run on UI > thread, > > > the ContactDatabase object may be already deleted on the blocking pool > thread. > > > If |callback| uses ContactDatabase object, it'll cause a use-after-free. > > > > > > To solve this problem, I think you can use PosTaskAndReply like: > > > > > > PostTaskAndReply( > > > FROM_HERE, > > > base::Bind(&ContactDatabase::SaveContactsFromTaskRunner, > > > base::Unretained(this), ...), > > > base::Bind(&ContactDatabase::RunCallback, > > > weak_ptr_factory_.GetWeakPtr(), callback)); > > > > > > Then, add the following to DestroyOnUIThread() > > > > > > weak_ptr_factory_.InvalidateWeakPtrs(); > > > > > > This way, the 2nd callback passed to PostTaskAndReply won't be run after the > > > point that the weak pointers are invalidated. > > > > > > > > > > I'm not convinced of the risk -- if we initiate deletion of the > ContactDatabase > > from the UI thread, we shouldn't be trying to use it from the UI thread later. > > > That said, I'm not opposed to what you're suggesting. Were you envisioning > > RunCallback() as just calling callback.Run()? Note that a "success" parameter > > needs to be passed to the callback in this case (although maybe that should be > a > > pre-bound scoped_refptr, as described above). > > Yes, RunCallback() just runs callback.Run(). "success" parameter can be passed > to RunCallback() as a pointer as described above. > > > BTW, callback accessing a deleted object may not be a real risk here, but it's > not uncommon for the callback to access the object that ran the callback, like: > > some_reader_->Read(base::Bind(&OnRead, ...)); > > SomeClass::OnRead(...) { > if (size == 0) { > // All bytes read. Close the reader. > some_reader_->Close(); > } > ... > } > > Makes sense; thanks for the explanation. Done (with lots of calls to base::Bind()). http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database_unittest.cc (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database_unittest.cc:104: message_loop_.Quit(); Asked in an earlier patch set re moving RunBlockingPoolTask(): "Does something like chrome/browser/chromeos/test_util.h/cc, with a chromeos::test namespace, seem like the right place?" I've added a TODO, but let me know if you think it should go in this change (or go in first in a separate change, so I can use it in this change).
http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database_unittest.cc (right): http://codereview.chromium.org/10832064/diff/12003/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database_unittest.cc:104: message_loop_.Quit(); On 2012/07/31 19:34:42, Daniel Erat wrote: > Asked in an earlier patch set re moving RunBlockingPoolTask(): "Does something > like chrome/browser/chromeos/test_util.h/cc, with a > chromeos::test namespace, seem like the right place?" > > I've added a TODO, but let me know if you think it should go in this change (or > go in first in a separate change, so I can use it in this change). Forgot to reply. TODO() sounds good for now. http://codereview.chromium.org/10832064/diff/2004/chrome/DEPS File chrome/DEPS (right): http://codereview.chromium.org/10832064/diff/2004/chrome/DEPS#newcode6 chrome/DEPS:6: "+leveldb", I'm slightly worried if we should add this here. We might want to start from a place like chrome/browser/chromeos/DEPS http://codereview.chromium.org/10832064/diff/2004/chrome/browser/chromeos/con... File chrome/browser/chromeos/contacts/contact_database.h (right): http://codereview.chromium.org/10832064/diff/2004/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.h:35: typedef base::Callback<void(const bool* success)> InitCallback; It may be awkward for the client of this class to get a bool pointer, rather than a bool. It's a bit tedious but you might want to convert bool* to bool before passing it to client-supplied callbacks. http://codereview.chromium.org/10832064/diff/2004/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.h:113: base::WeakPtrFactory<ContactDatabase> weak_ptr_factory_; This is very subtle but my understanding is that this should be the last member, so that it's destructed before other members.
http://codereview.chromium.org/10832064/diff/2004/chrome/browser/chromeos/con... File chrome/browser/chromeos/contacts/contact_database.h (right): http://codereview.chromium.org/10832064/diff/2004/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.h:113: base::WeakPtrFactory<ContactDatabase> weak_ptr_factory_; On 2012/07/31 19:56:57, satorux1 wrote: > This is very subtle but my understanding is that this should be the last member, > so that it's destructed before other members. This may not matter here as InvalidateWeakPtrs() is manually called, but usually, it's implicit, and weak pointers have to be invalidated before other members.
http://codereview.chromium.org/10832064/diff/2004/chrome/DEPS File chrome/DEPS (right): http://codereview.chromium.org/10832064/diff/2004/chrome/DEPS#newcode6 chrome/DEPS:6: "+leveldb", On 2012/07/31 19:56:57, satorux1 wrote: > I'm slightly worried if we should add this here. We might want to start from a > place like chrome/browser/chromeos/DEPS I was hoping it'd be non-controversial, since sql is already listed. http://codereview.chromium.org/10832064/diff/2004/chrome/browser/chromeos/con... File chrome/browser/chromeos/contacts/contact_database.h (right): http://codereview.chromium.org/10832064/diff/2004/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.h:35: typedef base::Callback<void(const bool* success)> InitCallback; On 2012/07/31 19:56:57, satorux1 wrote: > It may be awkward for the client of this class to get a bool pointer, rather > than a bool. > > It's a bit tedious but you might want to convert bool* to bool before passing it > to client-supplied callbacks. Done. http://codereview.chromium.org/10832064/diff/2004/chrome/browser/chromeos/con... chrome/browser/chromeos/contacts/contact_database.h:113: base::WeakPtrFactory<ContactDatabase> weak_ptr_factory_; On 2012/07/31 19:58:03, satorux1 wrote: > On 2012/07/31 19:56:57, satorux1 wrote: > > This is very subtle but my understanding is that this should be the last > member, > > so that it's destructed before other members. > > This may not matter here as InvalidateWeakPtrs() is manually called, but > usually, it's implicit, and weak pointers have to be invalidated before other > members. Done.
LGTM! http://codereview.chromium.org/10832064/diff/10009/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database.cc (right): http://codereview.chromium.org/10832064/diff/10009/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:48: database_dir, success), nit: sky would say align parameters vertically (one parameter per line)
http://codereview.chromium.org/10832064/diff/10009/chrome/browser/chromeos/co... File chrome/browser/chromeos/contacts/contact_database.cc (right): http://codereview.chromium.org/10832064/diff/10009/chrome/browser/chromeos/co... chrome/browser/chromeos/contacts/contact_database.cc:48: database_dir, success), On 2012/07/31 21:19:27, satorux1 wrote: > nit: sky would say align parameters vertically (one parameter per line) Done. I was torn on this -- I sort of like grouping the callback arguments together on a single line if possible.
TBR ben for DEPS and *.gypi
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/10832064/4004
Change committed as 149299 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
