Closed Bug 1266054 Opened 4 years ago Closed 4 years ago

|WakeLockListener|: Automated ref-counting for DBus

Categories

(Core :: Widget: Gtk, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(3 files, 5 obsolete files)

58 bytes, text/x-review-board-request
karlt
: review+
Details
58 bytes, text/x-review-board-request
karlt
: review+
Details
58 bytes, text/x-review-board-request
karlt
: review+
Details
We have helper classes for using DBus structures with |RefPtr|. |WakeLockListener| should use them.
Stores the DBus connection in |UniquePtr| to make sure it's unref'ed correctly.
Attachment #8743243 - Attachment is obsolete: true
Attachment #8743249 - Attachment is obsolete: true
Attachment #8743243 - Flags: review?(karlt)
Attachment #8743249 - Flags: review?(karlt)
Attachment #8744877 - Flags: review?(karlt)
Depends on: 1267214
Attachment #8743248 - Flags: review?(karlt) → review+
Comment on attachment 8744877 [details] [diff] [review]
[01] Bug 1266054: GTK+: Hold private references to |DBusConnection| in |UniquePtr|

>This patch changes |WakeLockListener| to use a private connection
>to the DBus session bus. |WakeLockListener| is supposed to work
>independently from other users of DBus, so a separate connection
>avoids possible interference.

What interference are you anticipating here?

This code is intended for the main thread only.  There are other problems to
solve if this may be used on other threads.

I don't want to open new connections unnecessarily.  I assume dbus_bus_get()
blocks when opening new connections, and file handles are a limited resource.

I like the explicit DeleterType for UniquePtr.  (Specializing DefaultDelete
runs risk of trouble if the header is accidentally missed.)
Attachment #8744877 - Flags: review?(karlt)
> What interference are you anticipating here?
> 
> This code is intended for the main thread only.  There are other problems to
> solve if this may be used on other threads.

We once had such a problem while experimenting with Bluetooth on Linux: the Bluetooth code ran DBus I/O on it's own thread and shared the connection with the GTK main loop. Received DBus messages were processed on either thread, which led to problems with thread synchronization.

Until recently, the code in uriloader/exthandler/nsDBusHandlerApp.cpp used the shared connection to the DBus session bus on a separate thread. It worked because it never received any messages.

> I don't want to open new connections unnecessarily.  I assume dbus_bus_get()
> blocks when opening new connections, and file handles are a limited resource.

True :/ Maybe the rule should be that at least 'non-main-thread' code requires private DBus connections?
This patch changes |WakeLockListener| to store its connection to
the DBus session bus in an instance of |RefPtr|. The reference will
be released automatically from the class' destructor.

Review commit: https://reviewboard.mozilla.org/r/49981/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49981/
Comment on attachment 8747636 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusConnection| in |RefPtr|, r=karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49981/diff/1-2/
Attachment #8747636 - Attachment description: MozReview Request: Bug 1266054: GTK+: Hold references to |DBusConnection| in |RefPtr| → MozReview Request: Bug 1266054: GTK+: Hold references to |DBusConnection| in |RefPtr|, r?karlt
Attachment #8747637 - Attachment description: MozReview Request: Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr| → MozReview Request: Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr|, r?karlt
Attachment #8747638 - Attachment description: MozReview Request: Bug 1266054: GTK+: Hold references to |DBusPendingCall| in |RefPtr| → MozReview Request: Bug 1266054: GTK+: Hold references to |DBusPendingCall| in |RefPtr|, r=karlt
Attachment #8747636 - Flags: review?(karlt)
Attachment #8747637 - Flags: review?(karlt)
Attachment #8747638 - Flags: review?(karlt)
Comment on attachment 8747637 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr|, r=karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49983/diff/1-2/
Comment on attachment 8747638 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusPendingCall| in |RefPtr|, r=karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49985/diff/1-2/
Attachment #8743245 - Attachment is obsolete: true
Attachment #8743248 - Attachment is obsolete: true
Attachment #8744877 - Attachment is obsolete: true
Changes:

  - moved review to MozReview
  - store shared |DBusConnection| in |RefPtr|
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> > This code is intended for the main thread only.  There are other problems to
> > solve if this may be used on other threads.
> 
> We once had such a problem while experimenting with Bluetooth on Linux: the
> Bluetooth code ran DBus I/O on it's own thread and shared the connection
> with the GTK main loop. Received DBus messages were processed on either
> thread, which led to problems with thread synchronization.

dbus_connection_setup_with_g_main() sets the context for processing messages
sent and received on the connection.  When the context argument is null, I
assume it uses the default GMainContext for that thread.  That's the typical
GLib API.  If that is called on the wrong thread, then yes, I can imagine that
causing problems.

"It's recommended to dispatch from a single thread", but I don't know why
dispatch from multiple threads would be a problem.  Perhaps dispatch is
closely associated with receiving replies, dispatching errors, etc.

I don't know how blocking calls work on other threads.  That may have been
implemented to "work" assuming the main thread never blocks on the calling
thread.  "The most useful function to call from multiple threads at once is
dbus_connection_send_with_reply_and_block().  That is, multiple threads can
make method calls at the same time."  I assume dbus_connection_send() is also
useful.  This is also assuming no bugs in the implementation. 

> Until recently, the code in uriloader/exthandler/nsDBusHandlerApp.cpp used
> the shared connection to the DBus session bus on a separate thread. It
> worked because it never received any messages.

I wonder whether dbus_connection_flush() triggers dispatch, or just waits for
it to happen, assuming the connection has been considered on the main thread
GMainContext.

> Maybe the rule should be that at least 'non-main-thread' code
> requires private DBus connections?

That may be reasonable.  It depends on how the other threads manage events and
want replies.

If the other threads want replies on the same thread, then they need to use a
private connection and consider that in the thread's poll call.

If replies on the main thread is fine, then I guess there's no need for
another connection, but something needs to ensure the connection is integrated
into the main thread event loop.  If there are no replies, the situation is
similar, I guess.

"Prefer dbus_connection_open() to dbus_connection_open_private() unless you
have good reason; connections are expensive enough that it's wasteful to
create lots of connections to the same server."

Now that we have GTK3 builds, they already depend on a GIO version with GDBus,
so maybe we can think about moving to GDBus.  I don't know whether that is
different though.
Comment on attachment 8747636 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusConnection| in |RefPtr|, r=karlt

https://reviewboard.mozilla.org/r/49981/#review47107

::: widget/gtk/WakeLockListener.cpp:319
(Diff revision 2)
> -    dbus_connection_set_exit_on_disconnect(mConnection, false);
> -    dbus_connection_setup_with_g_main(mConnection, nullptr);
> +    dbus_connection_set_exit_on_disconnect(mConnection.get(), false);
> +    dbus_connection_setup_with_g_main(mConnection.get(), nullptr);

These get()s are no longer required.

::: widget/gtk/WakeLockListener.cpp:354
(Diff revision 2)
>    if(!topic.Equals(NS_LITERAL_STRING("screen")))
>      return NS_OK;
>  
>    WakeLockTopic* topicLock = mTopics.Get(topic);
>    if (!topicLock) {
> -    topicLock = new WakeLockTopic(topic, mConnection);
> +    topicLock = new WakeLockTopic(topic, mConnection.get());

Similarly.
Attachment #8747636 - Flags: review?(karlt) → review+
Attachment #8747637 - Flags: review?(karlt) → review+
Comment on attachment 8747637 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr|, r=karlt

https://reviewboard.mozilla.org/r/49983/#review47109

Thanks.  That's much easier to review.
Attachment #8747638 - Flags: review?(karlt) → review+
Comment on attachment 8747638 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusPendingCall| in |RefPtr|, r=karlt

https://reviewboard.mozilla.org/r/49985/#review47111
Comment on attachment 8747636 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusConnection| in |RefPtr|, r=karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49981/diff/2-3/
Attachment #8747636 - Attachment description: MozReview Request: Bug 1266054: GTK+: Hold references to |DBusConnection| in |RefPtr|, r?karlt → MozReview Request: Bug 1266054: GTK+: Hold private references to |DBusConnection| in |UniquePtr|, r=karlt
Attachment #8747637 - Attachment description: MozReview Request: Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr|, r?karlt → MozReview Request: Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr|, r=karlt
Comment on attachment 8747637 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr|, r=karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49983/diff/2-3/
Comment on attachment 8747638 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusPendingCall| in |RefPtr|, r=karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49985/diff/2-3/
Comment on attachment 8747636 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusConnection| in |RefPtr|, r=karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49981/diff/3-4/
Attachment #8747636 - Attachment description: MozReview Request: Bug 1266054: GTK+: Hold private references to |DBusConnection| in |UniquePtr|, r=karlt → MozReview Request: Bug 1266054: GTK+: Hold references to |DBusConnection| in |RefPtr|, r=karlt
Comment on attachment 8747637 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr|, r=karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49983/diff/3-4/
Comment on attachment 8747638 [details]
MozReview Request: Bug 1266054: GTK+: Hold references to |DBusPendingCall| in |RefPtr|, r=karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49985/diff/3-4/
Depends on: 1271477
https://hg.mozilla.org/mozilla-central/rev/5ad856fab921
https://hg.mozilla.org/mozilla-central/rev/825c91333305
https://hg.mozilla.org/mozilla-central/rev/37073e486aca
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.