Bug 181825 – [GTK] USE_UPOWER causes crashes inside a chroot or on systems with broken dbus/upower

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

I'm trying to run the GTK layout tests inside a chroot  and I get a lot of test crashing.

Hunting this down it seem it happens only when USE_UPOWER is set.

The problem seems to be that on the constructor of LowPowerModeNotifier at Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp the call to up_client_get_display_device() is returning NULL ( which is ok according to the doc https://upower.freedesktop.org/docs/UPower-up-client.html#up-client-get-display-device ) so we end calling g_signal_connect_swapped with NULL and everything blows up.

Also up_client_new() can as well return NULL according to the doc.

Actually in my test case both are returning NULL, so both m_device and m_upClient are nullptr.

I think we need to check at run-time this, and bail out properly in case something is wrong.

Having UPOWER support at build time doesn't means that it will be functional at run-time.

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #0)
> Having UPOWER support at build time doesn't means that it will be functional
> at run-time.

It's also broken in flatpak, but that is a harder problem... we have to create a freedesktop battery status portal to make that case work.

[Michael Catanzaro]

Created attachment 331883 [details]
Patch

[Michael Catanzaro]

Comment on attachment 331883 [details]
Patch

Not ready for review, I missed your warning about up_client_new() also returning NULL.

And the docs seem to indicate that up_client_new() is synchronous, with a recommendation to use a method that does not exist if we want to avoid that behavior. So we're probably going to need to dispatch all the work to a secondary thread.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #3)
> And the docs seem to indicate that up_client_new() is synchronous, with a
> recommendation to use a method that does not exist if we want to avoid that
> behavior. So we're probably going to need to dispatch all the work to a
> secondary thread.

There is up_client_new_full(), which does actually exist, but is missing from the documentation. But it's fully synchronous too.

This is a problem because we don't want LowPowerModeNotifier to take five seconds (or whatever its timeout is) to construct. LowPowerModeNotifier construction ought to complete instantaneously.

[Michael Catanzaro]

The author of upower-glib suggests that we should not use it:

mcatanzaro	Heeey hughsie, I have a question about upower-glib. It looks like the API is fully synchronous, right? up_client_new() and up_client_new_full() will both hang until a timeout if upowerd is not responding via D-Bus? So the whole API is intended to be used either by non-GUI applications, or on a secondary thread?
hughsie	mcatanzaro, i gnome-power-manager I think i did it in a thread, right?
although with GDBus you don't actually need to use the lib
mcatanzaro	I'm trying to decide whether to spin up  another secondary thread to use upower-glib, on to just use the D-Bus API directly.
hughsie	mcatanzaro, go dbus if i were you
mcatanzaro	OK, thanks for the tip

I bet the D-Bus API isn't too hard.

[Carlos Alberto Lopez Perez]

I think we can maybe land the (In reply to Michael Catanzaro from comment #3)
> Comment on attachment 331883 [details]
> Patch
> 
> Not ready for review, I missed your warning about up_client_new() also
> returning NULL.
> 

I think this patch is fine as a first approach.

I suggest to simply add another null check for up_client_new().

> And the docs seem to indicate that up_client_new() is synchronous, with a
> recommendation to use a method that does not exist if we want to avoid that
> behavior. So we're probably going to need to dispatch all the work to a
> secondary thread.

Perhaps we can do that in another bug?

[Michael Catanzaro]

No, even with another NULL check, it won't be OK because the function calls will hang until the D-Bus connection times out. It's on my short-term TODO list. We need to either fix it properly, or, if you don't want to wait, remove the USE(UPOWER) support until we have time to get it right.

(We should also make sure it works in Flatpak before bringing it back. We shouldn't be adding any features that don't work in Flatpak. That means first creating a freedesktop battery status portal.)

[Michael Catanzaro]

Created attachment 333155 [details]
Patch

[EWS Watchlist]

Attachment 333155 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:41:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:43:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:46:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:53:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:56:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:56:  Else clause should never be on same line as else (use 2 lines)  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:57:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:71:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:74:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:76:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:77:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:78:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:81:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:101:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 21 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #8)
> Created attachment 333155 [details]
> Patch

This patch seems to work on my desktop, but that's maybe not very interesting. Any volunteers to test it on a laptop or any device that actually has a battery? I added a bunch of debug output (that's why the style checker is mad) so that it should be easy to check if it's giving sane results.

I can test it on my travel laptop if needed, but I'm lazy and would rather not set up a development environment there.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #10)
> I can test it on my travel laptop if needed, but I'm lazy and would rather
> not set up a development environment there.

Anyone have a quick chance to check on a laptop if the output is sane? Otherwise, I'll take a day next week to set up an environment to check it out.

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #11)
> (In reply to Michael Catanzaro from comment #10)
> > I can test it on my travel laptop if needed, but I'm lazy and would rather
> > not set up a development environment there.
> 
> Anyone have a quick chance to check on a laptop if the output is sane?
> Otherwise, I'll take a day next week to set up an environment to check it
> out.

I have XPS 13 laptop with the development environment.
I'm now applying this patch to my WebKit tree and building.

[Yusuke Suzuki]

Opened Minibrowser, at that time, adapter is connected.

Starting MiniBrowser.
GLib-GIO-Message: Using the 'memory' GSettings backend.  Your settings will not be saved or shared with other applications.
xkbcommon: ERROR: Key "<CAPS>" added to modifier map for multiple modifiers; Using Control, ignoring Lock
GLib-GIO-Message: Using the 'memory' GSettings backend.  Your settings will not be saved or shared with other applications.
WebCore::LowPowerModeNotifier::LowPowerModeNotifier(WebCore::LowPowerModeNotifier::LowPowerModeChangeCallback&&)
void WebCore::LowPowerModeNotifier::updateWarningLevel()
WarningLevel=1, m_lowPowerModeEnabled=0
Connected to gpropertieschanged!
xkbcommon: ERROR: Key "<CAPS>" added to modifier map for multiple modifiers; Using Control, ignoring Lock
WebCore::LowPowerModeNotifier::LowPowerModeNotifier(WebCore::LowPowerModeNotifier::LowPowerModeChangeCallback&&)
GLib-GIO-Message: Using the 'memory' GSettings backend.  Your settings will not be saved or shared with other applications.
void WebCore::LowPowerModeNotifier::updateWarningLevel()
WarningLevel=1, m_lowPowerModeEnabled=0
Connected to gpropertieschanged!
static void WebCore::LowPowerModeNotifier::gPropertiesChangedCallback(WebCore::LowPowerModeNotifier*, GVariant*)
static void WebCore::LowPowerModeNotifier::gPropertiesChangedCallback(WebCore::LowPowerModeNotifier*, GVariant*)
static void WebCore::LowPowerModeNotifier::gPropertiesChangedCallback(WebCore::LowPowerModeNotifier*, GVariant*) // <- adapter disconnected
static void WebCore::LowPowerModeNotifier::gPropertiesChangedCallback(WebCore::LowPowerModeNotifier*, GVariant*)
static void WebCore::LowPowerModeNotifier::gPropertiesChangedCallback(WebCore::LowPowerModeNotifier*, GVariant*) // <- adapter reconnected
static void WebCore::LowPowerModeNotifier::gPropertiesChangedCallback(WebCore::LowPowerModeNotifier*, GVariant*)

[Michael Catanzaro]

OK, thanks Yusuke. To be cautious, do you mind leaving your adapter out and testing what happens once you reach low power state? If the WarningLevel changes once you reach low battery, then I'll strip out the debug and mark it r?.

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #14)
> OK, thanks Yusuke. To be cautious, do you mind leaving your adapter out and
> testing what happens once you reach low power state? If the WarningLevel
> changes once you reach low battery, then I'll strip out the debug and mark
> it r?.

OK, i've got the result.

When low-power mode starts,

static void WebCore::LowPowerModeNotifier::gPropertiesChangedCallback(WebCore::LowPowerModeNotifier*, GVariant*)
void WebCore::LowPowerModeNotifier::warningLevelChanged()
void WebCore::LowPowerModeNotifier::updateWarningLevel()
WarningLevel=3, m_lowPowerModeEnabled=1
static void WebCore::LowPowerModeNotifier::gPropertiesChangedCallback(WebCore::LowPowerModeNotifier*, GVariant*)
void WebCore::LowPowerModeNotifier::warningLevelChanged()
void WebCore::LowPowerModeNotifier::updateWarningLevel()
WarningLevel=3, m_lowPowerModeEnabled=1

is dumped.
After that, I connected the adapter and got the results.

static void WebCore::LowPowerModeNotifier::gPropertiesChangedCallback(WebCore::LowPowerModeNotifier*, GVariant*)
void WebCore::LowPowerModeNotifier::warningLevelChanged()
void WebCore::LowPowerModeNotifier::updateWarningLevel()
WarningLevel=1, m_lowPowerModeEnabled=0
static void WebCore::LowPowerModeNotifier::gPropertiesChangedCallback(WebCore::LowPowerModeNotifier*, GVariant*)
void WebCore::LowPowerModeNotifier::warningLevelChanged()
void WebCore::LowPowerModeNotifier::updateWarningLevel()
WarningLevel=1, m_lowPowerModeEnabled=0

[Michael Catanzaro]

Looks good. Let's do this. Thanks!

[Michael Catanzaro]

Created attachment 334286 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 334286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334286&action=review

> Source/WebCore/platform/glib/LowPowerModeNotifierGLib.cpp:100
> +    if (m_cancellable)
> +        g_cancellable_cancel(m_cancellable.get());

g_cancellable_cancel is null safe.

[Michael Catanzaro]

Committed r228888: <https://trac.webkit.org/changeset/228888>