Bug 181825 - [GTK] USE_UPOWER causes crashes inside a chroot or on systems with broken dbus/upower
Summary: [GTK] USE_UPOWER causes crashes inside a chroot or on systems with broken dbu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks: 177810
  Show dependency treegraph
 
Reported: 2018-01-18 17:55 PST by Carlos Alberto Lopez Perez
Modified: 2018-02-21 11:42 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.31 KB, patch)
2018-01-21 14:29 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (12.32 KB, patch)
2018-02-05 21:19 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (12.18 KB, patch)
2018-02-20 10:33 PST, Michael Catanzaro
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2018-01-18 17:55:50 PST
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.
Comment 1 Michael Catanzaro 2018-01-21 14:29:04 PST
(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.
Comment 2 Michael Catanzaro 2018-01-21 14:29:46 PST
Created attachment 331883 [details]
Patch
Comment 3 Michael Catanzaro 2018-01-21 14:33:26 PST
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.
Comment 4 Michael Catanzaro 2018-01-21 15:48:35 PST
(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.
Comment 5 Michael Catanzaro 2018-01-22 07:09:46 PST
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.
Comment 6 Carlos Alberto Lopez Perez 2018-02-05 07:38:24 PST
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?
Comment 7 Michael Catanzaro 2018-02-05 08:09:42 PST
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.)
Comment 8 Michael Catanzaro 2018-02-05 21:19:45 PST
Created attachment 333155 [details]
Patch
Comment 9 EWS Watchlist 2018-02-05 21:20:52 PST
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.
Comment 10 Michael Catanzaro 2018-02-05 21:22:22 PST
(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.
Comment 11 Michael Catanzaro 2018-02-17 18:41:08 PST
(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.
Comment 12 Yusuke Suzuki 2018-02-17 22:30:44 PST
(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.
Comment 13 Yusuke Suzuki 2018-02-17 23:14:29 PST
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*)
Comment 14 Michael Catanzaro 2018-02-18 07:48:56 PST
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?.
Comment 15 Yusuke Suzuki 2018-02-19 23:28:57 PST
(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
Comment 16 Michael Catanzaro 2018-02-20 10:27:29 PST
Looks good. Let's do this. Thanks!
Comment 17 Michael Catanzaro 2018-02-20 10:33:36 PST
Created attachment 334286 [details]
Patch
Comment 18 Carlos Garcia Campos 2018-02-20 22:55:30 PST
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.
Comment 19 Michael Catanzaro 2018-02-21 11:42:39 PST
Committed r228888: <https://trac.webkit.org/changeset/228888>