Multi process notifications on linux (dbus)

Multi process notifications on linux (dbus)

BUG=none
TEST=unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72645

[dmac, 2011-01-21 19:20:01]

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
File chrome/browser/multi_process_notification_linux.cc (right):

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:101: bool success = true;
move success decl down near where it is used.

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:104: dbus_error_init(&error);
check error here?

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:122: DBusMessage* message =
dbus_message_new_signal(kNotifyPath, kNotifyInterface,
create a wrapper class for this that does the unref for us. I think if you
search the code base you will find that there is already one defined.

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:136: LOG(ERROR) << "Failed to
set signal name.";
maybe pass the name in along with the error?

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:165: dbus_error_init(&error);
create a wrapper class for dbus_error handling as well? Maybe there is one of
these that is shared too?

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:285: if (!g_thread_) {
need a lock around this thread creation. Multiple threads can call startListener
so you would have a race condition.

[garykac, 2011-01-21 21:46:19]

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
File chrome/browser/multi_process_notification_linux.cc (right):

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:101: bool success = true;
On 2011/01/21 19:20:01, dmac wrote:
> move success decl down near where it is used.

Done.

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:104: dbus_error_init(&error);
On 2011/01/21 19:20:01, dmac wrote:
> check error here?

This is initializing the DBusError struct, so there's no error to check.

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:122: DBusMessage* message =
dbus_message_new_signal(kNotifyPath, kNotifyInterface,
On 2011/01/21 19:20:01, dmac wrote:
> create a wrapper class for this that does the unref for us. I think if you
> search the code base you will find that there is already one defined.

There's no generic 'scoped_ptr' wrapper for these types and this file contains
the only occurrences of DbusMessage in Chrome.

wifi_data_provider_linux.cc has something that could be adapted if needed.

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:136: LOG(ERROR) << "Failed to
set signal name.";
On 2011/01/21 19:20:01, dmac wrote:
> maybe pass the name in along with the error?

Done.

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:165: dbus_error_init(&error);
On 2011/01/21 19:20:01, dmac wrote:
> create a wrapper class for dbus_error handling as well? Maybe there is one of
> these that is shared too?

wifi_data_provider_linux.cc uses CheckError, and I considered doing something
similar.
The downside of this approach is that they have a single generic error message
and rely on error.message for context. I wanted to provide more background info
for the error.

http://codereview.chromium.org/6320011/diff/1/chrome/browser/multi_process_no...
chrome/browser/multi_process_notification_linux.cc:285: if (!g_thread_) {
On 2011/01/21 19:20:01, dmac wrote:
> need a lock around this thread creation. Multiple threads can call
startListener
> so you would have a race condition.

Yikes!  Done.

[dmac, 2011-01-21 22:28:33]

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
File chrome/browser/multi_process_notification_linux.cc (right):

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
chrome/browser/multi_process_notification_linux.cc:51: virtual ~ListenerImpl();
no need for this to be virtual. Nothing else is here, and this class doesn't
inherit.

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
chrome/browser/multi_process_notification_linux.cc:54: void OnListen(const
std::string& name);
can OnListen be private?

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
chrome/browser/multi_process_notification_linux.cc:269: fullname_(name),
why bother with fullname_ you use it in two places rarely. I would recommend
just generating it when you need it. It's being initialized incorrectly here
anyways.

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
chrome/browser/multi_process_notification_linux.cc:274: if (g_thread_) {
do you need to lock around this reference to g_thread_ as well? appears there is
an error in the Mac version as well regarding this. Would you be so kind as to
add a todo for me to the Mac code?

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
chrome/browser/multi_process_notification_linux.cc:299: if (!g_thread_) {
double checked lock... not going to work unfortunately

http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf

[garykac, 2011-01-22 00:09:05]

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
File chrome/browser/multi_process_notification_linux.cc (right):

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
chrome/browser/multi_process_notification_linux.cc:51: virtual ~ListenerImpl();
On 2011/01/21 22:28:33, dmac wrote:
> no need for this to be virtual. Nothing else is here, and this class doesn't
> inherit.

Done.

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
chrome/browser/multi_process_notification_linux.cc:54: void OnListen(const
std::string& name);
On 2011/01/21 22:28:33, dmac wrote:
> can OnListen be private?

ListenerThread calls it.

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
chrome/browser/multi_process_notification_linux.cc:269: fullname_(name),
On 2011/01/21 22:28:33, dmac wrote:
> why bother with fullname_ you use it in two places rarely. I would recommend
> just generating it when you need it. It's being initialized incorrectly here
> anyways.

Done.

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
chrome/browser/multi_process_notification_linux.cc:274: if (g_thread_) {
On 2011/01/21 22:28:33, dmac wrote:
> do you need to lock around this reference to g_thread_ as well? appears there
is
> an error in the Mac version as well regarding this. Would you be so kind as to
> add a todo for me to the Mac code?

Done.  I checked the Mac code and it looks correct (ti has a lock in the
destructor).

http://codereview.chromium.org/6320011/diff/5001/chrome/browser/multi_process...
chrome/browser/multi_process_notification_linux.cc:299: if (!g_thread_) {
On 2011/01/21 22:28:33, dmac wrote:
> double checked lock... not going to work unfortunately
> 
> http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf

Damn.

[dmac, 2011-01-22 00:32:02]

http://codereview.chromium.org/6320011/diff/16001/chrome/browser/multi_proces...
File chrome/browser/multi_process_notification_linux.cc (right):

http://codereview.chromium.org/6320011/diff/16001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:213: while (!done) {
just while (true) {
}

http://codereview.chromium.org/6320011/diff/16001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:271: base::AutoLock
autolock(thread_create_lock_);
this needs to be static. Mac has bug as well.

[garykac, 2011-01-25 17:47:13]

http://codereview.chromium.org/6320011/diff/16001/chrome/browser/multi_proces...
File chrome/browser/multi_process_notification_linux.cc (right):

http://codereview.chromium.org/6320011/diff/16001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:213: while (!done) {
On 2011/01/22 00:32:02, dmac wrote:
> just while (true) {
> }

Done.

http://codereview.chromium.org/6320011/diff/16001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:271: base::AutoLock
autolock(thread_create_lock_);
On 2011/01/22 00:32:02, dmac wrote:
> this needs to be static. Mac has bug as well.

Moved into singleton since we don't want statics.

[dmac, 2011-01-25 18:06:27]

http://codereview.chromium.org/6320011/diff/21001/chrome/browser/multi_proces...
File chrome/browser/multi_process_notification_linux.cc (right):

http://codereview.chromium.org/6320011/diff/21001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:69: base::Lock
thread_access_lock_;
do we really need two locks? One is probably sufficient. i don't expect much
contention.

http://codereview.chromium.org/6320011/diff/21001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:268: char* name;
const char *name?

http://codereview.chromium.org/6320011/diff/21001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:315:
io_loop_to_listen_on->PostTask(FROM_HERE, task);
you were going to get rid of this use of "io_loop_to_listen_on" for me.

http://codereview.chromium.org/6320011/diff/21001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:384: if (thread_ &&
thread_->Init()) {
if init fails we leak the thread here.

[garykac, 2011-01-25 20:39:49]

http://codereview.chromium.org/6320011/diff/21001/chrome/browser/multi_proces...
File chrome/browser/multi_process_notification_linux.cc (right):

http://codereview.chromium.org/6320011/diff/21001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:69: base::Lock
thread_access_lock_;
On 2011/01/25 18:06:28, dmac wrote:
> do we really need two locks? One is probably sufficient. i don't expect much
> contention.

Separated into two locks to distinguish between thread creation (internal-only)
and accessing thread members )lock used externally).

Merged into one per discussion.

http://codereview.chromium.org/6320011/diff/21001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:268: char* name;
On 2011/01/25 18:06:28, dmac wrote:
> const char *name?

Done.

http://codereview.chromium.org/6320011/diff/21001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:315:
io_loop_to_listen_on->PostTask(FROM_HERE, task);
On 2011/01/25 18:06:28, dmac wrote:
> you were going to get rid of this use of "io_loop_to_listen_on" for me.

Breaks unittests - leaving the same for now.

http://codereview.chromium.org/6320011/diff/21001/chrome/browser/multi_proces...
chrome/browser/multi_process_notification_linux.cc:384: if (thread_ &&
thread_->Init()) {
On 2011/01/25 18:06:28, dmac wrote:
> if init fails we leak the thread here.

Done.

[dmac, 2011-01-25 21:10:34]

LGTM