Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(186)

Issue 10492005: dbus: Fix a subtle butterfly-effect bug in handling incoming messages (Closed)

Created:
8 years, 6 months ago by satorux1
Modified:
8 years, 6 months ago
Reviewers:
keybuk, stevenjb, hashimoto
CC:
chromium-reviews
Visibility:
Public.

Description

dbus: Fix a subtle butterfly-effect bug in handling incoming messages libdbus does bookkeeping of the number of bytes in the incoming message queue implicitly, and asks client code (chrome) to stop monitoring the underlying socket, as soon as it exceeds a certain number, which is set to 63MB as of now. This caused a DCHECK failure (and could break the D-Bus message dispatching with Release builds) because the bookkeeping happned on the UI thread via a seemingly harmless call to dbus_message_unref(), but we cannot stop monitoring from UI thread. This patch fixes (or works around) the problem by deleting incoming messages on D-Bus thread, so the bookkeeping is done on D-Bus thread. Note that we don't have to change exported_object.cc, as the method call message is deleted on the D-Bus thread in ExportedObject::OnMethodCompleted() The following is a stacktrace of the DCHECK failure. Here, dbus::Response (method reply) is deleted on UI thread, that results in a call to dbus::Bus::OnToggleWatch, which should only be called on the D-Bus thread, hence crashed as a DCHECK failure Backtrace: base::debug::StackTrace::StackTrace() [0x517972] logging::LogMessage::~LogMessage() [0x4b3a57] <- crashing because we are not base::ThreadRestrictions::AssertIOAllowed() [0x4f0b35] dbus::Bus::AssertOnDBusThread() [0x45ceb6] <- checking if we are on the right thread dbus::Bus::OnToggleWatch() [0x45d0c1] dbus::Bus::OnToggleWatchThunk() [0x45d45d] <-- the change is notified. _dbus_watch_list_toggle_watch [0x7f35e0a15245] protected_change_watch [0x7f35e09f2eef] _dbus_connection_toggle_watch_unlocked [0x7f35e09f302e] check_read_watch [0x7f35e0a1332d] <-- what? why checking socket status here?? socket_live_messages_changed [0x7f35e0a1436c] live_messages_size_notify [0x7f35e0a11996] _dbus_counter_adjust [0x7f35e0a0c098] free_size_counter [0x7f35e0a04423] _dbus_list_foreach [0x7f35e0a180d9] dbus_message_cache_or_finalize [0x7f35e0a0446b] dbus_message_unref [0x7f35e0a05e7e] <-- releasing a message dbus::Message::~Message() [0x46abbb] dbus::Response::~Response() [0x470478] scoped_ptr<>::~scoped_ptr() [0x41e99f] dbus::ObjectProxy::RunResponseCallback() [0x472095] BUG=126217 TEST=added unit tests Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140165

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -1 line) Patch
M dbus/end_to_end_async_unittest.cc View 3 chunks +35 lines, -0 lines 0 comments Download
M dbus/object_proxy.cc View 2 chunks +34 lines, -1 line 0 comments Download

Messages

Total messages: 6 (0 generated)
satorux1
I guess this isn't the culprit of crbug.com/126217, but it's good to fix a bug ...
8 years, 6 months ago (2012-06-01 23:20:00 UTC) #1
satorux1
8 years, 6 months ago (2012-06-01 23:41:47 UTC) #2
stevenjb
Wow, that's subtle. This LGTM. Hope it does the trick!
8 years, 6 months ago (2012-06-01 23:50:56 UTC) #3
hashimoto
lgtm Hope these are the only libdbus function calls we should move from UI thread ...
8 years, 6 months ago (2012-06-02 01:07:57 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/satorux@chromium.org/10492005/1
8 years, 6 months ago (2012-06-02 01:16:06 UTC) #5
commit-bot: I haz the power
8 years, 6 months ago (2012-06-02 02:53:24 UTC) #6
Change committed as 140165

Powered by Google App Engine
This is Rietveld 408576698