1266054 - |WakeLockListener|: Automated ref-counting for DBus

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

We have helper classes for using DBus structures with |RefPtr|. |WakeLockListener| should use them.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1266054: GTK+: Hold references to |DBusConnection| in |RefPtr|

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr|

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1266054: GTK+: Hold references to |DBusPendingCall| in |RefPtr|

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1266054: GTK+: Get private DBus connection for |WakeLockListener|

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1266054: GTK+: Hold private references to |DBusConnection| in |UniquePtr|

Stores the DBus connection in |UniquePtr| to make sure it's unref'ed correctly.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8743245 [details] [diff] [review]
[02] Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr|

Please include function signatures and 8 lines of context in patches, or use
mozreview.

https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html

Comment 7

[Karl Tomlinson (:karlt)]

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.)

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> 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?

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: MozReview Request: Bug 1266054: GTK+: Hold references to |DBusConnection| in |RefPtr|, r=karlt

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 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: MozReview Request: Bug 1266054: GTK+: Hold references to |DBusMessage| in |RefPtr|, r=karlt

Review commit: https://reviewboard.mozilla.org/r/49983/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49983/

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: MozReview Request: Bug 1266054: GTK+: Hold references to |DBusPendingCall| in |RefPtr|, r=karlt

Review commit: https://reviewboard.mozilla.org/r/49985/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49985/

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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/

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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/

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Changes:

  - moved review to MozReview
  - store shared |DBusConnection| in |RefPtr|

Comment 16

[Karl Tomlinson (:karlt)]

(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 17

[Karl Tomlinson (:karlt)]

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.

Comment 18

[Karl Tomlinson (:karlt)]

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.

Comment 19

[Karl Tomlinson (:karlt)]

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 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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/

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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 22

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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/

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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 25

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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/

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad856fab921
https://hg.mozilla.org/integration/mozilla-inbound/rev/825c91333305
https://hg.mozilla.org/integration/mozilla-inbound/rev/37073e486aca

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=37073e486aca

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ad856fab921
https://hg.mozilla.org/mozilla-central/rev/825c91333305
https://hg.mozilla.org/mozilla-central/rev/37073e486aca