1825181 - Nothing is displayed in Wayland mode if running inside a Snap not named "firefox" and without accelerated graphics

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P3)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This is a weird combination of circumstances, and yet, I have run into it. Part of the problem here is bug 1791442, which changed things so we ignore Snap's env vars if they indicate a Snap not named exactly firefox (or whatever MOZ_APP_NAME is). That was done for the use of profile selection, but because the logic was factored out in bug 1757209, it's also used anywhere that calls shm_open — Snap applies a sandbox policy that requires the shm name to have a prefix incorporating the snap instance name.

IPC shared memory can use shm_open, but on Linux ≥ 3.17 (released Oct. 2014) we'll use memfd_create instead. So, unless the kernel is rather old (or /proc is unmounted) this won't immediately break the browser.

However, there's also this code in widget/gtk/WaylandBuffer.cpp. If this is used, and the shm_open call fails, then we never display anything; a window outline appears but the pixels inside it don't change. Fortunately, we seem to use that code only if hardware acceleration is absent; that could happen in a VM with unaccelerated graphics, but I also ran into it in a VM with a virtualized GPU… which we blocklisted because the Snap package has Mesa 21 instead of 22.

As for the Snap name: I ran into this with some patches I have for developing/debugging Snap-specific issues, which add a mach command to repackage a local build as a Snap, which is named firefox-devel in order to not conflict with the real package. (Someone else ran into this in a Parallels VM, and I reproduced it under QEMU.) In that situation, it's easy enough to fix: I can patch the name check to also accept MOZ_APP_NAME "-devel" or otherwise add a way to opt out.

But, I'm a little concerned about the situation mentioned in bug 1791442 — in that case I assume we'd fail to shm_open if we don't use the other app's Snap instance name as seen in the environment, but we don't want to use it for profile selection, so there would need to be more complexity there. Alternately, the code in WaylandBuffer.cpp could be modified to use memfd_create and avoid this problem on any vaguely-recent system the way IPC does.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1791442

:emilio, since you are the author of the regressor, bug 1791442, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 2

[Dave Townsend [:mossop]]

It would be nice if there were a sane way to detect that we're running as a snap. Environment variables are always going to be flaky.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Is there any common shared way how SHM is allocated in Firefox? We can switch WaylandBuffer implementation and use the shared one.

(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #0)

However, there's also [this code in widget/gtk/WaylandBuffer.cpp][wl-buf]. If this is used, and the shm_open call fails, then we never display anything; a window outline appears but the pixels inside it don't change. Fortunately, we seem to use that code [only if hardware acceleration is absent][wl-if]; that could happen in a VM with unaccelerated graphics, but I also ran into it in a VM with a virtualized GPU… [which we blocklisted][block] because the Snap package has Mesa 21 instead of 22.

It's used for popups too as we don't create HW backend for them. So most of the menus/tooltips etc. will be blank.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

So looks like SharedMemory object is used in Gecko, right?

https://searchfox.org/mozilla-central/source/ipc/chromium/src/base/shared_memory.h#27

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #0)

But, I'm a little concerned about the situation mentioned in bug 1791442 — in that case I assume we'd fail to shm_open if we don't use the other app's Snap instance name as seen in the environment, but we don't want to use it for profile selection, so there would need to be more complexity there. Alternately, the code in WaylandBuffer.cpp could be modified to use memfd_create and avoid this problem on any vaguely-recent system the way IPC does.

How so? In that case Firefox is not running as a Snap, so would it really fail to create shared memory? I would expect the sandbox policy not to be in effect then.

Comment 6

[Mike Kaply [:mkaply]]

I asked Canonical about the environment variable thing.

"Checking SNAP_NAME in the environment is a reliable means, but I guess there needs to be some logic to handle alternate valid MOZ_APP_NAME. The current logic was to handle cases where another snap that uses classic confinement, which could leak it's SNAP_NAME when firefox is spawned to open a link. For example, vscode documentation links would result in the non-snap version of firefox appearing as if it was running as a snap. This is less of an issue now since many users are already using firefox as a snap, so this wouldn't be leaked. But it is something that should be considered.

I guess MOZ_APP_NAME could be firefox-nightly, firefox-beta, firefox-devel or firefox but all use the same SNAP_NAME just different channels. Perhaps instead of a string comparison, it could check that it starts with "firefox"?"

Comment 7

[Dave Townsend [:mossop]]

(In reply to Mike Kaply [:mkaply] from comment #6)

I guess MOZ_APP_NAME could be firefox-nightly, firefox-beta, firefox-devel or firefox but all use the same SNAP_NAME just different channels. Perhaps instead of a string comparison, it could check that it starts with "firefox"?"

Is there any way we can do a custom build or repack Firefox for snap in such a way that we just have an easy flag available to us? Alternatively it looks like maybe we could add our own environment variable for this use rather than relying on the snap environment variables: https://searchfox.org/mozilla-central/source/taskcluster/docker/firefox-snap/firefox.snapcraft.yaml.in#16

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Jed, does it help if we remove shm_open() from WaylandBuffer.cpp and switch to SharedMemory?

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Mike Kaply [:mkaply] from comment #6)

I asked Canonical about the environment variable thing.

"Checking SNAP_NAME in the environment is a reliable means, but I guess there needs to be some logic to handle alternate valid MOZ_APP_NAME. The current logic was to handle cases where another snap that uses classic confinement, which could leak it's SNAP_NAME when firefox is spawned to open a link. For example, vscode documentation links would result in the non-snap version of firefox appearing as if it was running as a snap. This is less of an issue now since many users are already using firefox as a snap, so this wouldn't be leaked. But it is something that should be considered.

If “classic confinement” means that it's not subject to the AppArmor rules that require a shm prefix (and the documentation seems to be saying that), then our current behavior should work in that case, and this bug is effectively WONTFIX, because I can already fix the one case where it does happen.

Incidentally, I notice that under Flatpak we have a private /dev/shm, and it looks like Snap can also do that, but we currently don't. I don't know if that's because it would break things, or just because the option to do it wasn't available with Snap until recently.

I guess MOZ_APP_NAME could be firefox-nightly, firefox-beta, firefox-devel or firefox but all use the same SNAP_NAME just different channels. Perhaps instead of a string comparison, it could check that it starts with "firefox"?"

As I understand it, Firefox Nightly/Beta/etc. are handled as channels within the firefox snap; there is no firefox-nightly etc. The firefox-devel snap is something I've been working on to allow running a local build as a Snap, to debug/test issues that are specific to the Snap environment / sandboxing; it has a separate name in order to not interfere with the “real” Firefox install, and I've already added a patch to that topic branch to recognize the "-devel" suffix. So, if that's the only use case that would actually be affected, then this is basically already fixed.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Martin Stránský [:stransky] (ni? me) from comment #8)

Jed, does it help if we remove shm_open() from WaylandBuffer.cpp and switch to SharedMemory?

It would, and you'd also get the other benefits of memfd (faster creation, no issues with /dev/shm running out of space), but see above — if I'm right about comment #6 then this isn't really a problem.

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #0)

But, I'm a little concerned about the situation mentioned in bug 1791442 — in that case I assume we'd fail to shm_open if we don't use the other app's Snap instance name as seen in the environment, but we don't want to use it for profile selection, so there would need to be more complexity there. Alternately, the code in WaylandBuffer.cpp could be modified to use memfd_create and avoid this problem on any vaguely-recent system the way IPC does.

How so? In that case Firefox is not running as a Snap, so would it really fail to create shared memory? I would expect the sandbox policy not to be in effect then.

My concern was that we'd inherit the other Snap's sandbox policy — if these restrictions aren't inherited by child processes then they'd be easy to evade. But if the other program wasn't sandboxed to begin with, and it sounds like that's the case, then this isn't a problem.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1791442

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1825181 [Linux] Use base::SharedMemory as backend for wl_shm pool r?jld

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fadc9036ed2f
[Linux] Use base::SharedMemory as backend for wl_shm pool r=jld

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fadc9036ed2f

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:stransky, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox114 to wontfix.

For more information, please visit BugBot documentation.