Bug 234612

Summary: [Flatpak] Fix a11y tests on some distros including Fedora
Product: WebKit Reporter: Patrick Griffis <pgriffis>
Component: WebKitGTKAssignee: Patrick Griffis <pgriffis>
Status: REOPENED ---    
Severity: Normal CC: bugs-noreply, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Patrick Griffis 2021-12-22 11:19:03 PST
[Flatpak] Fix a11y tests on some distros including Fedora
Comment 1 Patrick Griffis 2021-12-22 11:19:46 PST Comment hidden (obsolete)
Comment 2 Patrick Griffis 2021-12-22 11:30:21 PST Comment hidden (obsolete)
Comment 3 Patrick Griffis 2021-12-22 11:49:12 PST Comment hidden (obsolete)
Comment 4 Michael Catanzaro 2021-12-22 12:27:06 PST
Comment on attachment 447824 [details]
Patch

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

> Tools/ChangeLog:10
> +        On Debian the tests previously worked because the a11y bus is set
> +        to a filesystem path (unix:abstract=/tmp/dbus-YvSOaUDNJe) however
> +        on Fedora it is not a path (unix:abstract=00008). Using --no-a11y-bus fixes this.

Something's wrong with this changelog explanation. unix:abstract=/tmp/dbus-YvSOaUDNJe is not a filesystem path. It's a D-Bus address that corresponds to an abstract socket with a name that *looks* like a filesystem path, but of course abstract sockets definitionally do not exist on the filesystem. Right? So what's *really* the difference between using that vs. unix:abstract=00008?

(To access an abstract socket on the host, we would need to use --allow=net to get the host network namespace.)

> Tools/flatpak/flatpakutils.py:767
> +                           # at-spi-registryd creates directories like `$XDG_RUNTIME_DIR/at-spi2-E6A5E1` on the host
> +                           "--filesystem=" + GLib.get_user_runtime_dir(),

But there is --filesystem=host just above, so surely this is not needed?
Comment 5 Michael Catanzaro 2021-12-22 12:29:32 PST
BTW this might be fixed by https://gitlab.gnome.org/GNOME/at-spi2-core/-/commit/412bdee0a1daa3fe9bc7d1fa289a31495dff3387.
Comment 6 Patrick Griffis 2021-12-22 16:14:44 PST
(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 447824 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447824&action=review
> 
> > Tools/ChangeLog:10
> > +        On Debian the tests previously worked because the a11y bus is set
> > +        to a filesystem path (unix:abstract=/tmp/dbus-YvSOaUDNJe) however
> > +        on Fedora it is not a path (unix:abstract=00008). Using --no-a11y-bus fixes this.
> 
> Something's wrong with this changelog explanation.
> unix:abstract=/tmp/dbus-YvSOaUDNJe is not a filesystem path. It's a D-Bus
> address that corresponds to an abstract socket with a name that *looks* like
> a filesystem path, but of course abstract sockets definitionally do not
> exist on the filesystem. Right? So what's *really* the difference between
> using that vs. unix:abstract=00008?

It is a path on the filesystem.

> (To access an abstract socket on the host, we would need to use --allow=net
> to get the host network namespace.)
> 
> > Tools/flatpak/flatpakutils.py:767
> > +                           # at-spi-registryd creates directories like `$XDG_RUNTIME_DIR/at-spi2-E6A5E1` on the host
> > +                           "--filesystem=" + GLib.get_user_runtime_dir(),
> 
> But there is --filesystem=host just above, so surely this is not needed?

It fails without. The `host` option does not mount that directory.
Comment 7 Patrick Griffis 2021-12-22 16:17:03 PST
(In reply to Michael Catanzaro from comment #5)
> BTW this might be fixed by
> https://gitlab.gnome.org/GNOME/at-spi2-core/-/commit/
> 412bdee0a1daa3fe9bc7d1fa289a31495dff3387.

I do expect that to fix the filesystem permission. Using `--no-a11y-bus` is still correct though because we don't want it to be proxied by flatpak which filters out calls we use.
Comment 8 Patrick Griffis 2021-12-22 16:18:04 PST Comment hidden (obsolete)
Comment 9 Patrick Griffis 2021-12-22 16:19:32 PST
(In reply to Michael Catanzaro from comment #5)
> BTW this might be fixed by
> https://gitlab.gnome.org/GNOME/at-spi2-core/-/commit/
> 412bdee0a1daa3fe9bc7d1fa289a31495dff3387.

Actually this still uses `g_get_user_runtime_dir()` so those files are still unreadable.
Comment 10 Patrick Griffis 2021-12-22 16:29:19 PST
Created attachment 447840 [details]
Patch
Comment 11 Michael Catanzaro 2021-12-22 17:14:10 PST
Comment on attachment 447840 [details]
Patch

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

OK, I don't see anything wrong with the change itself, but I'm almost positive the changelog is not correct.

> Tools/ChangeLog:9
> +        On Debian the tests previously worked because the a11y bus is set
> +        to a filesystem path (unix:abstract=/tmp/dbus-YvSOaUDNJe) however

If this is truly the server address, then there should not be a socket created at /tmp/dbus-YvSOaUDNJe. Instead, there would be a socket created in the abstract socket namespace with name '\0' (NUL character) followed by YvSOaUDNJe. Correct? So I'm almost certain this changelog entry is wrong. I would believe that Debian is using and address like unix:path=/tmp/dbus-YvSOaUDNJe or unix:dir=/tmp. I don't doubt that the socket really gets created on the filesystem. But if so, the server address it's using surely does not start with unix:abstract.

> Tools/flatpak/flatpakutils.py:764
> +                           # --session-bus is only a workaround for https://github.com/flatpak/flatpak/pull/4630

Please say "FIXME" so we know to drop this in the future.

> Tools/flatpak/flatpakutils.py:769
> +                           # at-spi creates directories like `$XDG_RUNTIME_DIR/at-spi2-E6A5E1` on the host
> +                           "--filesystem=" + self.get_user_runtime_dir(),

OK, looking at the flatpak-builder documentation, I see that /tmp is on a denylist so that it doesn't get mounted by --filesystem=host.
Comment 12 Michael Catanzaro 2021-12-22 17:20:16 PST
Oh and I bet the behavior difference is dbus-daemon vs. dbus-broker.
Comment 13 Patrick Griffis 2021-12-22 19:43:05 PST
Created attachment 447854 [details]
Patch
Comment 14 Patrick Griffis 2021-12-22 19:43:47 PST
I've updated the changelog. The exact details may indeed be wrong. This does allow the tests to run on Fedora at the very least.
Comment 15 EWS 2021-12-23 08:54:55 PST
Committed r287396 (245536@main): <https://commits.webkit.org/245536@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447854 [details].
Comment 16 Patrick Griffis 2021-12-23 12:04:25 PST
Reopening to attach new patch.
Comment 17 Patrick Griffis 2021-12-23 12:04:27 PST
Created attachment 447897 [details]
Patch
Comment 18 EWS 2021-12-23 12:38:00 PST
Committed r287407 (245542@main): <https://commits.webkit.org/245542@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447897 [details].
Comment 19 Michael Catanzaro 2021-12-23 16:20:39 PST
Reopening due to the partial revert.