Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11199007:
Add sender verification of D-Bus signals. (Closed)
|
Created:
8 years, 2 months ago by Haruki Sato Modified:
8 years, 1 month ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd sender verification of D-Bus signals.
The CL does the following:
- Add a match rule for NameOwnerChanged signal from org.freedesktop.DBus.
- Update the owner of the well-known bus name when a NameOwnerChanged comes.
- Call GetNameOwner method to update the latest if ObjectProxy instance does not know the owner.
- Verify the sender of the signal and reject the unknown senders.
- Add UMA_HISTOGRAM_COUNTS "DBus.RejectedSignalCount" for rejected signals.
and a unittest.
BUG=140938
TEST=manual, unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164597
Patch Set 1 : #
Total comments: 41
Patch Set 2 : address comment, fix tests, rebase #Patch Set 3 : remove unnecessary code. #
Total comments: 31
Patch Set 4 : address comments. #
Total comments: 10
Patch Set 5 : restore synchronous UpdateNameOwner and use it only in ConnectToSignalInternal #
Total comments: 14
Patch Set 6 : fixes #
Total comments: 2
Patch Set 7 : #
Total comments: 5
Patch Set 8 : use the default #
Messages
Total messages: 21 (0 generated)
Scott, could you review this especially on the verification logic? Am I doing it right? I just confirmed that we have "Rejecting a message from a wrong sender." when I try $ dbus-send --system --type=signal /org/chromium/PowerManager org.chromium.PowerManager.BrightnessChanged Satoru, I had an issue with the test case for a huge signal. I guess I don't understand the threading and memory management in /dbus code, so I'd like to have some help from you. I will investigate more next week anyway. Thanks!
Initial comments. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcode27 dbus/object_proxy.cc:27: const dbus::ObjectPath kDbusSystemObjectPath("/org/freedesktop/DBus"); ObjectPath may be POD but we should avoid doing this. Please use const char[] instead. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static... http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:378: signal_callback) && when parameters don't fit in one line, please align them vertically. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:381: "org.freedesktop.DBus.NameOwnerChanged"); Looks a bit tricky. Please do if (AddMatchRuleAndCallback(..) && AddMatchRuleWithoutCallback(...)) { success = true; } http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h File dbus/object_proxy.h (right): http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h#newcode239 dbus/object_proxy.h:239: // Add the match rule to the bus and associate the callback with the signal. Adds http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h#newcode240 dbus/object_proxy.h:240: bool AddMatchRuleAndCallback(std::string match_rule, Please use const std::string& for function parameters. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h#newcode241 dbus/object_proxy.h:241: std::string absolute_signal_name, indentation is off http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... File dbus/signal_sender_verification_unittest.cc (right): http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... dbus/signal_sender_verification_unittest.cc:159: sleep(1); We shouldn't use sleep() in tests. Can we remove this?
http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:532: bool ObjectProxy::AddMatchRuleWithoutCallback( The order does not match with .h file? http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:533: std::string match_rule, std::string absolute_signal_name) { Please add this: bus_->AssertOnDBusThread(); http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:534: if (match_rules_.find(match_rule) == match_rules_.end()) { maybe: if (match_rules_.find(match_rule) == match_rules_.end()) return true; This will save one-indentation level. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:549: bool ObjectProxy::AddMatchRuleAndCallback(std::string match_rule, And -> With? http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:551: SignalCallback signal_callback) { Please add this: bus_->AssertOnDBusThread(); http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:573: bool ObjectProxy::UpdateNameOwner() { Please add this: bus_->AssertOnDBusThread(); http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:588: get_name_owner_call.raw_message(), 1000, error.get()); what's 1000? please define a constant. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:596: reader.PopString(&service_name_owner_); It's not guaranteed that PopString() succeeds. Please check the return value: return reader.PopString(&service_name_owner_); http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:600: DBusHandlerResult ObjectProxy::HandleNameOwnerChanged(Signal* signal) { Please put: bus_->AssertOnDBusThread(); DCHECK(signal); http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:609: reader.PopString(&new_owner); Please check return value of PopString() http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h File dbus/object_proxy.h (right): http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h#newcode247 dbus/object_proxy.h:247: std::string absolute_signal_name); indentation is off. http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... File dbus/signal_sender_verification_unittest.cc (right): http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... dbus/signal_sender_verification_unittest.cc:138: TEST_F(SignalSenderVerificationTest, TestSignal) { Please split this into two separate test cases: SignalAccepted and SignalRejected http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... dbus/signal_sender_verification_unittest.cc:159: sleep(1); On 2012/10/22 04:50:42, satorux1 wrote: > We shouldn't use sleep() in tests. Can we remove this? We should probably use base::PlatformThread::Sleep(TestTimeouts::action_max_timeout()); Please also add some comment why you need to wait here. http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... dbus/signal_sender_verification_unittest.cc:193: // NameOwnerChanged does not come. GetNameOwner() is calling a method synchronously and blocks the D-Bus thread. That'd would explain some weirdness here? I think we should call GetNameOwner() asynchronously, and process the incoming signal at a later time, once the owner is resolved. That'd complicate the code a bit, though.
Thank you for the thorough review! I'm rewriting it to add async GetNameOwner, I will get back to you maybe tomorrow. On 2012/10/22 05:23:32, satorux1 wrote: > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc > File dbus/object_proxy.cc (right): > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... > dbus/object_proxy.cc:532: bool ObjectProxy::AddMatchRuleWithoutCallback( > The order does not match with .h file? > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... > dbus/object_proxy.cc:533: std::string match_rule, std::string > absolute_signal_name) { > Please add this: > > bus_->AssertOnDBusThread(); > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... > dbus/object_proxy.cc:534: if (match_rules_.find(match_rule) == > match_rules_.end()) { > maybe: > > if (match_rules_.find(match_rule) == match_rules_.end()) > return true; > > This will save one-indentation level. > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... > dbus/object_proxy.cc:549: bool ObjectProxy::AddMatchRuleAndCallback(std::string > match_rule, > And -> With? > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... > dbus/object_proxy.cc:551: SignalCallback signal_callback) { > Please add this: > > bus_->AssertOnDBusThread(); > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... > dbus/object_proxy.cc:573: bool ObjectProxy::UpdateNameOwner() { > Please add this: > > bus_->AssertOnDBusThread(); > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... > dbus/object_proxy.cc:588: get_name_owner_call.raw_message(), 1000, error.get()); > what's 1000? please define a constant. > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... > dbus/object_proxy.cc:596: reader.PopString(&service_name_owner_); > It's not guaranteed that PopString() succeeds. Please check the return value: > > return reader.PopString(&service_name_owner_); > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... > dbus/object_proxy.cc:600: DBusHandlerResult > ObjectProxy::HandleNameOwnerChanged(Signal* signal) { > Please put: > > bus_->AssertOnDBusThread(); > DCHECK(signal); > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... > dbus/object_proxy.cc:609: reader.PopString(&new_owner); > Please check return value of PopString() > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h > File dbus/object_proxy.h (right): > > http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h#newcode247 > dbus/object_proxy.h:247: std::string absolute_signal_name); > indentation is off. > > http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... > File dbus/signal_sender_verification_unittest.cc (right): > > http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... > dbus/signal_sender_verification_unittest.cc:138: > TEST_F(SignalSenderVerificationTest, TestSignal) { > Please split this into two separate test cases: > > SignalAccepted and SignalRejected > > http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... > dbus/signal_sender_verification_unittest.cc:159: sleep(1); > On 2012/10/22 04:50:42, satorux1 wrote: > > We shouldn't use sleep() in tests. Can we remove this? > > We should probably use > > base::PlatformThread::Sleep(TestTimeouts::action_max_timeout()); > > Please also add some comment why you need to wait here. > > http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... > dbus/signal_sender_verification_unittest.cc:193: // NameOwnerChanged does not > come. > GetNameOwner() is calling a method synchronously and blocks the D-Bus thread. > That'd would explain some weirdness here? > > I think we should call GetNameOwner() asynchronously, and process the incoming > signal at a later time, once the owner is resolved. That'd complicate the code a > bit, though.
Now the tests pass! I've added an async version of UpdateNameOwner and restructured the HandleMessage with it. Could you take another look? Thanks! http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcode27 dbus/object_proxy.cc:27: const dbus::ObjectPath kDbusSystemObjectPath("/org/freedesktop/DBus"); On 2012/10/22 04:50:42, satorux1 wrote: > ObjectPath may be POD but we should avoid doing this. Please use const char[] > instead. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static... Done. Thanks! http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:378: signal_callback) && On 2012/10/22 04:50:42, satorux1 wrote: > when parameters don't fit in one line, please align them vertically. Done. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:381: "org.freedesktop.DBus.NameOwnerChanged"); On 2012/10/22 04:50:42, satorux1 wrote: > Looks a bit tricky. Please do > > if (AddMatchRuleAndCallback(..) && > AddMatchRuleWithoutCallback(...)) { > success = true; > } Done. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:532: bool ObjectProxy::AddMatchRuleWithoutCallback( On 2012/10/22 05:23:32, satorux1 wrote: > The order does not match with .h file? Done. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:533: std::string match_rule, std::string absolute_signal_name) { On 2012/10/22 05:23:32, satorux1 wrote: > Please add this: > > bus_->AssertOnDBusThread(); Done. Thanks. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:534: if (match_rules_.find(match_rule) == match_rules_.end()) { On 2012/10/22 05:23:32, satorux1 wrote: > maybe: > > if (match_rules_.find(match_rule) == match_rules_.end()) > return true; > > This will save one-indentation level. Done. Thanks! http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:549: bool ObjectProxy::AddMatchRuleAndCallback(std::string match_rule, On 2012/10/22 05:23:32, satorux1 wrote: > And -> With? Done. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:551: SignalCallback signal_callback) { On 2012/10/22 05:23:32, satorux1 wrote: > Please add this: > > bus_->AssertOnDBusThread(); Done. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:573: bool ObjectProxy::UpdateNameOwner() { On 2012/10/22 05:23:32, satorux1 wrote: > Please add this: > > bus_->AssertOnDBusThread(); Done. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:588: get_name_owner_call.raw_message(), 1000, error.get()); On 2012/10/22 05:23:32, satorux1 wrote: > what's 1000? please define a constant. Done. Thanks. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:596: reader.PopString(&service_name_owner_); On 2012/10/22 05:23:32, satorux1 wrote: > It's not guaranteed that PopString() succeeds. Please check the return value: > > return reader.PopString(&service_name_owner_); Done. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:600: DBusHandlerResult ObjectProxy::HandleNameOwnerChanged(Signal* signal) { On 2012/10/22 05:23:32, satorux1 wrote: > Please put: > > bus_->AssertOnDBusThread(); > DCHECK(signal); Done. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.cc#newcod... dbus/object_proxy.cc:609: reader.PopString(&new_owner); On 2012/10/22 05:23:32, satorux1 wrote: > Please check return value of PopString() Done. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h File dbus/object_proxy.h (right): http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h#newcode239 dbus/object_proxy.h:239: // Add the match rule to the bus and associate the callback with the signal. On 2012/10/22 04:50:42, satorux1 wrote: > Adds Done. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h#newcode240 dbus/object_proxy.h:240: bool AddMatchRuleAndCallback(std::string match_rule, On 2012/10/22 04:50:42, satorux1 wrote: > Please use const std::string& for function parameters. Done. Thanks. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h#newcode241 dbus/object_proxy.h:241: std::string absolute_signal_name, On 2012/10/22 04:50:42, satorux1 wrote: > indentation is off Done. Thanks. http://codereview.chromium.org/11199007/diff/2001/dbus/object_proxy.h#newcode247 dbus/object_proxy.h:247: std::string absolute_signal_name); On 2012/10/22 05:23:32, satorux1 wrote: > indentation is off. Done. http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... File dbus/signal_sender_verification_unittest.cc (right): http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... dbus/signal_sender_verification_unittest.cc:138: TEST_F(SignalSenderVerificationTest, TestSignal) { On 2012/10/22 05:23:32, satorux1 wrote: > Please split this into two separate test cases: > > SignalAccepted and SignalRejected Done. http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... dbus/signal_sender_verification_unittest.cc:159: sleep(1); On 2012/10/22 05:23:32, satorux1 wrote: > On 2012/10/22 04:50:42, satorux1 wrote: > > We shouldn't use sleep() in tests. Can we remove this? > > We should probably use > > base::PlatformThread::Sleep(TestTimeouts::action_max_timeout()); > > Please also add some comment why you need to wait here. Done. This is the best I can think of for now. Please let me know if any of you have an idea. Thanks. http://codereview.chromium.org/11199007/diff/2001/dbus/signal_sender_verifica... dbus/signal_sender_verification_unittest.cc:193: // NameOwnerChanged does not come. On 2012/10/22 05:23:32, satorux1 wrote: > GetNameOwner() is calling a method synchronously and blocks the D-Bus thread. > That'd would explain some weirdness here? > > I think we should call GetNameOwner() asynchronously, and process the incoming > signal at a later time, once the owner is resolved. That'd complicate the code a > bit, though. > Thank you for the advice. As discussed offline, I've added async version and more robust message handling.
http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:28: const char* kDbusSystemObjectPath = "/org/freedesktop/DBus"; nit: please do: const char kDbusSystemObjectPath[] = "/org/freedesktop/DBus"; http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:31: const static int kGetNameOwnerCallTimeoutMs = 1000; remove static. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:46: dbus::Signal* CloneSignalMessage(dbus::Signal* orig) { function comment is missing. orig -> signal ? http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:392: // Try getting the latest name owner. No pending signal. What does "no pending signal" mean? Please describe more. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:607: bus_->AssertOnDBusThread(); please put a blank line here. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:614: const dbus::ObjectPath obj_path("/org/freedesktop/DBus"); object_path in favor of less abbreviation. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:630: clone = CloneSignalMessage(released_signal); maybe: clone = CloneSignalMessage(pending_signal.get()) ? pending_signal.reset(); http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:632: bus_->ProcessAllIncomingDataIfAny(); why do we need to call this? We rarely need to call this. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:649: bus_->AssertOnDBusThread(); put a blank line here. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:669: void ObjectProxy::OnGetNameOwner(Signal* signal, Response* response) { signal -> pending_signal? DCHECK(pending_signal); http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:679: Signal* signal, Response* response) { ditto. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:680: bus_->AssertOnDBusThread(); put a blank line here. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:683: LOG(ERROR) << "Can not interpret the result of GetNameOwner: " << LOG(ERROR) << "Can not interpret the result of GetNameOwner: " << response->ToString(); http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:687: if (signal) signal should always be non-null here? http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:703: if (signal) ditto. http://codereview.chromium.org/11199007/diff/14003/dbus/signal_sender_verific... File dbus/signal_sender_verification_unittest.cc (right): http://codereview.chromium.org/11199007/diff/14003/dbus/signal_sender_verific... dbus/signal_sender_verification_unittest.cc:197: TEST_F(SignalSenderVerificationTest, TestHugeSignal) { I think we have the same test elsewhere. then please remove this here.
http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:28: const char* kDbusSystemObjectPath = "/org/freedesktop/DBus"; On 2012/10/25 01:55:16, satorux1 wrote: > nit: please do: > > const char kDbusSystemObjectPath[] = "/org/freedesktop/DBus"; Done. Thanks. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:31: const static int kGetNameOwnerCallTimeoutMs = 1000; On 2012/10/25 01:55:16, satorux1 wrote: > remove static. Done. Thanks. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:46: dbus::Signal* CloneSignalMessage(dbus::Signal* orig) { On 2012/10/25 01:55:16, satorux1 wrote: > function comment is missing. > > orig -> signal ? Done. Thanks. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:392: // Try getting the latest name owner. No pending signal. On 2012/10/25 01:55:16, satorux1 wrote: > What does "no pending signal" mean? Please describe more. Done. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:607: bus_->AssertOnDBusThread(); On 2012/10/25 01:55:16, satorux1 wrote: > please put a blank line here. Done. Thanks. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:614: const dbus::ObjectPath obj_path("/org/freedesktop/DBus"); On 2012/10/25 01:55:16, satorux1 wrote: > object_path in favor of less abbreviation. Done. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:630: clone = CloneSignalMessage(released_signal); On 2012/10/25 01:55:16, satorux1 wrote: > maybe: > > clone = CloneSignalMessage(pending_signal.get()) ? > pending_signal.reset(); Done. Thanks! http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:632: bus_->ProcessAllIncomingDataIfAny(); On 2012/10/25 01:55:16, satorux1 wrote: > why do we need to call this? We rarely need to call this. Ah! I somehow added this call here. Confirmed that this is unnecessary. Removed. Thanks! http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:649: bus_->AssertOnDBusThread(); On 2012/10/25 01:55:16, satorux1 wrote: > put a blank line here. Done. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:669: void ObjectProxy::OnGetNameOwner(Signal* signal, Response* response) { On 2012/10/25 01:55:16, satorux1 wrote: > signal -> pending_signal? > > DCHECK(pending_signal); Done. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:679: Signal* signal, Response* response) { On 2012/10/25 01:55:16, satorux1 wrote: > ditto. Done. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:680: bus_->AssertOnDBusThread(); On 2012/10/25 01:55:16, satorux1 wrote: > put a blank line here. Done. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:683: LOG(ERROR) << "Can not interpret the result of GetNameOwner: " << On 2012/10/25 01:55:16, satorux1 wrote: > LOG(ERROR) << "Can not interpret the result of GetNameOwner: " > << response->ToString(); Done. Thanks. http://codereview.chromium.org/11199007/diff/14003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:687: if (signal) On 2012/10/25 01:55:16, satorux1 wrote: > signal should always be non-null here? I am passing NULL as pending_signal in LINE 393, where I intended to retrieve the name owner earlier in ConnectToSignalInternal. Do you have any other idea, or think we don't need to try GetNameOwner in ConnectToSignalInternal? http://codereview.chromium.org/11199007/diff/14003/dbus/signal_sender_verific... File dbus/signal_sender_verification_unittest.cc (right): http://codereview.chromium.org/11199007/diff/14003/dbus/signal_sender_verific... dbus/signal_sender_verification_unittest.cc:197: TEST_F(SignalSenderVerificationTest, TestHugeSignal) { On 2012/10/25 01:55:16, satorux1 wrote: > I think we have the same test elsewhere. then please remove this here. Done.
http://codereview.chromium.org/11199007/diff/20009/dbus/bus.h File dbus/bus.h (right): http://codereview.chromium.org/11199007/diff/20009/dbus/bus.h#newcode447 dbus/bus.h:447: void ProcessAllIncomingDataIfAny(); No need to expose, right? http://codereview.chromium.org/11199007/diff/20009/dbus/end_to_end_async_unit... File dbus/end_to_end_async_unittest.cc (right): http://codereview.chromium.org/11199007/diff/20009/dbus/end_to_end_async_unit... dbus/end_to_end_async_unittest.cc:26: // ObjectProxy::UpdateNameOwnerAsyncfor why the number was chosen. I think you don't need to update the comment. http://codereview.chromium.org/11199007/diff/20009/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/20009/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:395: // update |service_name_owner_|. We might want to add: It's not guaranteed that we can get the name owner at this moment, as the service may not yet be started. If that's the case, we'll get the name owner via NameOwnerChanged signal, as soon as the service is started. http://codereview.chromium.org/11199007/diff/20009/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:448: } I just realized that this would be unnecessary 1) if we are lucky, we can get the name owner when connecting to a signal 2) otherwise, we'll receive the NameOwnerChanged as soon as the service is started If my understanding is correct, we can significantly simplify the code here. What do you think? http://codereview.chromium.org/11199007/diff/20009/dbus/object_proxy.h File dbus/object_proxy.h (right): http://codereview.chromium.org/11199007/diff/20009/dbus/object_proxy.h#newcod... dbus/object_proxy.h:218: // callback if any. callback if any -> callback associated with the signal.
Thank you for the advice! As discussed offline, I restored sync version of UpdateNameOwner and removed tricky signal handling. Could you take another look? Thanks. http://codereview.chromium.org/11199007/diff/20009/dbus/bus.h File dbus/bus.h (right): http://codereview.chromium.org/11199007/diff/20009/dbus/bus.h#newcode447 dbus/bus.h:447: void ProcessAllIncomingDataIfAny(); On 2012/10/25 08:56:23, satorux1 wrote: > No need to expose, right? Done. Thanks. http://codereview.chromium.org/11199007/diff/20009/dbus/end_to_end_async_unit... File dbus/end_to_end_async_unittest.cc (right): http://codereview.chromium.org/11199007/diff/20009/dbus/end_to_end_async_unit... dbus/end_to_end_async_unittest.cc:26: // ObjectProxy::UpdateNameOwnerAsyncfor why the number was chosen. On 2012/10/25 08:56:23, satorux1 wrote: > I think you don't need to update the comment. Done. http://codereview.chromium.org/11199007/diff/20009/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/20009/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:395: // update |service_name_owner_|. On 2012/10/25 08:56:23, satorux1 wrote: > We might want to add: > > It's not guaranteed that we can get the name owner at this moment, as the > service may not yet be started. If that's the case, we'll get the name owner via > NameOwnerChanged signal, as soon as the service is started. Done. http://codereview.chromium.org/11199007/diff/20009/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:448: } On 2012/10/25 08:56:23, satorux1 wrote: > I just realized that this would be unnecessary > > 1) if we are lucky, we can get the name owner when connecting to a signal > 2) otherwise, we'll receive the NameOwnerChanged as soon as the service is > started > > If my understanding is correct, we can significantly simplify the code here. > What do you think? Discussed offline. I confirmed that There's still very small timeframe where the owner of the name can send signal while ObjectProxy has not yet GetNameOwner, but that's very unlikely and happens only in browser's initialization. The services should not send such signal at that stage. Restored sync version of UpdateNameOwner and removed tricky signal handling. Thank you for the advice! http://codereview.chromium.org/11199007/diff/20009/dbus/object_proxy.h File dbus/object_proxy.h (right): http://codereview.chromium.org/11199007/diff/20009/dbus/object_proxy.h#newcod... dbus/object_proxy.h:218: // callback if any. On 2012/10/25 08:56:23, satorux1 wrote: > callback if any -> callback associated with the signal. Removed method.
http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:435: // Handle NameOwnerChanged separately . is missing http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:590: bool ObjectProxy::UpdateNameOwner() { make it void? The return value is not used. http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.h File dbus/object_proxy.h (right): http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.h#newcod... dbus/object_proxy.h:249: // |service_name_owner_| with the current owner of |service_name_|. Please add // // BLOCKING CALL. http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.h#newcod... dbus/object_proxy.h:250: bool UpdateNameOwner(); UpdateNameOwnerAndBlock() http://codereview.chromium.org/11199007/diff/23003/dbus/signal_sender_verific... File dbus/signal_sender_verification_unittest.cc (right): http://codereview.chromium.org/11199007/diff/23003/dbus/signal_sender_verific... dbus/signal_sender_verification_unittest.cc:141: base::Histogram* r_histogram = r_histogram -> reject_signal_histogram http://codereview.chromium.org/11199007/diff/23003/dbus/signal_sender_verific... dbus/signal_sender_verification_unittest.cc:151: base::PlatformThread::Sleep(TestTimeouts::tiny_timeout()); action_timeout() ? tiny_timeout() may be too small http://codereview.chromium.org/11199007/diff/23003/dbus/test_service.cc File dbus/test_service.cc (right): http://codereview.chromium.org/11199007/diff/23003/dbus/test_service.cc#newco... dbus/test_service.cc:107: message_loop()->PostTask( Why do you need to post this to the message loop on the same thread? Can we just call RequestOwnership() here?
http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:435: // Handle NameOwnerChanged separately On 2012/10/26 05:26:00, satorux1 wrote: > . is missing Done.Thanks. http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:590: bool ObjectProxy::UpdateNameOwner() { On 2012/10/26 05:26:00, satorux1 wrote: > make it void? The return value is not used. Done. Thanks. http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.h File dbus/object_proxy.h (right): http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.h#newcod... dbus/object_proxy.h:249: // |service_name_owner_| with the current owner of |service_name_|. On 2012/10/26 05:26:00, satorux1 wrote: > Please add > > // > // BLOCKING CALL. > Done. Thanks. http://codereview.chromium.org/11199007/diff/23003/dbus/object_proxy.h#newcod... dbus/object_proxy.h:250: bool UpdateNameOwner(); On 2012/10/26 05:26:00, satorux1 wrote: > UpdateNameOwnerAndBlock() Done. http://codereview.chromium.org/11199007/diff/23003/dbus/signal_sender_verific... File dbus/signal_sender_verification_unittest.cc (right): http://codereview.chromium.org/11199007/diff/23003/dbus/signal_sender_verific... dbus/signal_sender_verification_unittest.cc:141: base::Histogram* r_histogram = On 2012/10/26 05:26:00, satorux1 wrote: > r_histogram -> reject_signal_histogram Done. http://codereview.chromium.org/11199007/diff/23003/dbus/signal_sender_verific... dbus/signal_sender_verification_unittest.cc:151: base::PlatformThread::Sleep(TestTimeouts::tiny_timeout()); On 2012/10/26 05:26:00, satorux1 wrote: > action_timeout() ? tiny_timeout() may be too small Done. http://codereview.chromium.org/11199007/diff/23003/dbus/test_service.cc File dbus/test_service.cc (right): http://codereview.chromium.org/11199007/diff/23003/dbus/test_service.cc#newco... dbus/test_service.cc:107: message_loop()->PostTask( On 2012/10/26 05:26:00, satorux1 wrote: > Why do you need to post this to the message loop on the same thread? Can we just > call RequestOwnership() here? Hmm, Calling bus->RequestOwnership() here causes check failure at dbus::Bus::AssertOnOriginThread() in dbus::Bus::RequestOwnership . TestService::RequestOwnership is called from the main thread in SignalSenderVerificationTest, so I think we need to do this.
one last thing: http://codereview.chromium.org/11199007/diff/25009/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/25009/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:617: reader.PopString(&service_name_owner_); We should check the return value as it's not guaranteed to succeed, and service_name_owner_ won't change if failed. Hence the code should look like: std::string new_service_name_owner; if (reader.PopString(&new_service_name_owner)) service_name_owner_ = new_service_name_owner; else service_name_owner_.clear();
http://codereview.chromium.org/11199007/diff/25009/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/25009/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:617: reader.PopString(&service_name_owner_); On 2012/10/26 06:07:28, satorux1 wrote: > We should check the return value as it's not guaranteed to succeed, and > service_name_owner_ won't change if failed. Hence the code should look like: > > std::string new_service_name_owner; > if (reader.PopString(&new_service_name_owner)) > service_name_owner_ = new_service_name_owner; > else > service_name_owner_.clear(); Good catch! Thank you very much!
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haruki@chromium.org/11199007/27001
Retried try job too often for step(s) browser_tests
The logic looks good. http://codereview.chromium.org/11199007/diff/27001/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/27001/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:31: const int kGetNameOwnerCallTimeoutMs = 1000; Why the need for this to be special? http://codereview.chromium.org/11199007/diff/27001/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:608: kGetNameOwnerCallTimeoutMs, Why not DBUS_TIMEOUT_USE_DEFAULT ?
http://codereview.chromium.org/11199007/diff/27001/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/27001/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:608: kGetNameOwnerCallTimeoutMs, On 2012/10/26 20:26:00, keybuk wrote: > Why not DBUS_TIMEOUT_USE_DEFAULT ? Agreed. Let's just use the default.
Thank you for the advice! Adding it to CQ again. http://codereview.chromium.org/11199007/diff/27001/dbus/object_proxy.cc File dbus/object_proxy.cc (right): http://codereview.chromium.org/11199007/diff/27001/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:31: const int kGetNameOwnerCallTimeoutMs = 1000; On 2012/10/26 20:26:00, keybuk wrote: > Why the need for this to be special? Thank you for the advice! Removed. http://codereview.chromium.org/11199007/diff/27001/dbus/object_proxy.cc#newco... dbus/object_proxy.cc:608: kGetNameOwnerCallTimeoutMs, On 2012/10/26 23:41:29, satorux1 wrote: > On 2012/10/26 20:26:00, keybuk wrote: > > Why not DBUS_TIMEOUT_USE_DEFAULT ? > > Agreed. Let's just use the default. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haruki@chromium.org/11199007/39001
Change committed as 164597 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
