This is an automated email from the git hooks/post-receive script.
git pushed a commit to branch wl/real-browser
in repository enlightenment.
View the commit online.
commit 6f63b2c7c3b15a6bc37e493ffa92d595c8f0a05e
Author: Cedric BAIL <[email protected]>
AuthorDate: Mon Aug 17 00:06:01 2026 -0600
e_pixmap - hold the pool, not the buffer
4b728603e fixed a crash by holding a client's buffer back while a render was in
flight, then had to fix the client starvation that caused, and the starvation
kept coming back in new shapes - a busy count driven negative by
_e_pixmap_wayland_image_clear decrementing a buffer that was also on the
deferred list, and from then on a release that could never be sent.
That is the wrong thing to hold. What the render worker needs is the *memory*,
and the memory belongs to the shm pool, which is reference counted and
completely invisible to the client. What the client needs is its buffer back,
promptly, or it runs out and stops drawing.
So release the buffer immediately, exactly as before any of this, and keep a
reference to its pool until the renderer has finished - the same RENDER_POST
flush, now draining pools instead of buffers. The client is never made to wait,
and the pages cannot be unmapped underneath evas.
Also refuse to take a busy count below zero. A negative count can never come
back to zero, so the release is never sent and the client waits forever; the
scanout and plane paths still say XXX: fixme about their half of that
accounting, and a dropped release is a far better failure than a frozen client.
Measured: test_client_move - the interactive resize whose crash started all of
this - passes five for five. Firefox 4/4. Chromium and Brave are unchanged at
roughly half, which is the unmaximise stall they have had throughout and which
measures the same on the commit before this one; it is chased separately.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
---
src/bin/e_pixmap.c | 71 +++++++++++++++++++++++++++-----------
src/tests/wayland/browser-run.sh | 35 +++++++++++++++++--
src/tests/wayland/e_wl_testkit.c | 7 ++++
src/tests/wayland/e_wl_testkit.h | 5 +++
src/tests/wayland/meson.build | 5 +--
src/tests/wayland/pages/probe.html | 5 +++
src/tests/wayland/test_browser.c | 57 +++++++++++++++++++++++++++++-
7 files changed, 159 insertions(+), 26 deletions(-)
diff --git a/src/bin/e_pixmap.c b/src/bin/e_pixmap.c
index bb018596c..17edfe779 100644
--- a/src/bin/e_pixmap.c
+++ b/src/bin/e_pixmap.c
@@ -29,6 +29,9 @@ static Eina_Hash *pixmaps[2] = {NULL};
* back, runs out, and stops drawing, which from outside looks like a browser
* ignoring a configure. */
static Eina_List *_deferred_pixmaps = NULL;
+/* shm pools whose last reference is ours to drop, held until the renderer has
+ * finished. See _e_pixmap_wl_resource_release. */
+static Eina_List *_deferred_pools = NULL;
#endif
static Eina_Hash *aliases[2] = {NULL};
@@ -170,6 +173,15 @@ _e_pixmap_image_clear_x(void *data, Evas *e EINA_UNUSED, Evas_Object *obj EINA_U
static void
_e_pixmap_wl_resource_release(E_Comp_Wl_Buffer *buffer)
{
+ /* Never below zero. A busy count that has gone negative can never come back
+ * to zero, so the early return below fires every time and the release is
+ * never sent: the client waits for a buffer it will not get back and stops
+ * drawing. That is a far worse failure than the one release this drops, and
+ * one release too many is a real possibility while the scanout and plane
+ * paths in _e_pixmap_native_surface_status_update still say XXX: fixme
+ * about their half of the accounting. Measured on Brave, which went
+ * "busy 0->-1" and then never saw another wl_buffer.release. */
+ if (buffer->busy == 0) return;
buffer->busy--;
if (buffer->dmabuf_buffer)
linux_dmabuf_buffer_unref(buffer->dmabuf_buffer);
@@ -177,7 +189,26 @@ _e_pixmap_wl_resource_release(E_Comp_Wl_Buffer *buffer)
if (buffer->pool)
{
- wl_shm_pool_unref(buffer->pool);
+ /* Hand the buffer back now and keep its memory a moment longer.
+ *
+ * A client is entitled to destroy a buffer the instant it is told it
+ * may reuse it, and destroying it drops the last reference to the shm
+ * pool - which unmaps the pages evas may still be reading from a
+ * render worker. That was a reliable compositor crash on any
+ * interactive resize, inside evas_common_scale_rgba_sample_draw.
+ *
+ * Holding the *release* back instead is what this used to do, and it
+ * traded a crash for something worse: a client that is not given its
+ * buffers back runs out and stops drawing, which looks like a browser
+ * ignoring a configure and took three diagnoses to recognise. Measured
+ * on Brave - not one wl_buffer.release in a whole session.
+ *
+ * The pool reference is the part the renderer actually needs, and it
+ * is invisible to the client. Keep that, release the buffer. */
+ if (e_comp->rendering)
+ _deferred_pools = eina_list_append(_deferred_pools, buffer->pool);
+ else
+ wl_shm_pool_unref(buffer->pool);
buffer->pool = NULL;
}
@@ -195,26 +226,18 @@ _e_pixmap_wayland_buffer_release(E_Pixmap *cp, E_Comp_Wl_Buffer *buffer)
{
if (!buffer) return;
- if (e_comp->rendering)
- {
- if (buffer->discarding_pixmap) return;
-
- buffer->discarding_pixmap = cp;
- buffer->deferred_destroy_listener.notify =
- _e_pixmap_cb_deferred_buffer_destroy;
- wl_signal_add(&buffer->destroy_signal,
- &buffer->deferred_destroy_listener);
- cp->free_buffers = eina_list_append(cp->free_buffers, buffer);
- if (!eina_list_data_find(_deferred_pixmaps, cp))
- _deferred_pixmaps = eina_list_append(_deferred_pixmaps, cp);
- return;
- }
-
+ /* Off the busy list here, whether the release happens now or in the flush.
+ * busy_list is what the pixmap is still using, and from this point it is
+ * not - it is a buffer on its way back to the client. Leaving a deferred
+ * buffer on it means _e_pixmap_wayland_image_clear decrements busy for it
+ * as well, and a busy count that has gone negative can never reach zero
+ * again: _e_pixmap_wl_resource_release returns early every time, so the
+ * release is never sent, the client never gets the buffer back, and it
+ * stops drawing. Measured on Brave - "busy 0->-1", then not one
+ * wl_buffer.release for the rest of the session. */
if (buffer->busy == 1)
- {
- // we are at busy 1 abut about to go to 0 below, so remove now
- cp->busy_list = eina_list_remove(cp->busy_list, buffer);
- }
+ cp->busy_list = eina_list_remove(cp->busy_list, buffer);
+
_e_pixmap_wl_resource_release(buffer);
}
@@ -240,7 +263,7 @@ _e_pixmap_wl_buffers_free(E_Pixmap *cp)
b->deferred_destroy_listener.notify = NULL;
}
b->discarding_pixmap = NULL;
- if (b->busy == 1) cp->busy_list = eina_list_remove(cp->busy_list, b);
+ /* busy_list was dealt with when the release was deferred. */
_e_pixmap_wl_resource_release(b);
}
}
@@ -254,11 +277,17 @@ e_pixmap_deferred_flush(void)
{
#ifdef HAVE_WAYLAND
Eina_List *l = _deferred_pixmaps;
+ struct wl_shm_pool *pool;
E_Pixmap *cp;
_deferred_pixmaps = NULL;
EINA_LIST_FREE(l, cp)
_e_pixmap_wl_buffers_free(cp);
+
+ l = _deferred_pools;
+ _deferred_pools = NULL;
+ EINA_LIST_FREE(l, pool)
+ wl_shm_pool_unref(pool);
#endif
}
diff --git a/src/tests/wayland/browser-run.sh b/src/tests/wayland/browser-run.sh
index 12a5dd858..f26c95d52 100755
--- a/src/tests/wayland/browser-run.sh
+++ b/src/tests/wayland/browser-run.sh
@@ -68,13 +68,39 @@ RUNDIR=${E_TEST_RUNDIR:-$(mktemp -d "${TMPDIR:-/tmp}/e-browser.XXXXXX")}
PROFILE=""
mkdir -p "$PROFILE"
+# A local page gets copied into the session directory and served from there.
+# Browsers sandbox their content processes and the set of paths they will read
+# is not ours to predict - Firefox would not open a file from the source tree
+# with HOME pointing here, and said nothing about why. Inside HOME it works,
+# and the test is more hermetic for it: the browser reads nothing that is not
+# in the throwaway directory it was given.
+case "$URL" in
+file://*)
+ _path=${URL#file://}
+ if [ -f "$_path" ]; then
+ cp "$_path" "$RUNDIR/page.html"
+ URL=""
+ fi
+ ;;
+esac
+
COMMON="LC_ALL=C.UTF-8 TZ=UTC GDK_BACKEND=wayland XDG_SESSION_TYPE=wayland \
XDG_CURRENT_DESKTOP=Enlightenment"
case "$BROWSER" in
firefox)
# user.js rather than command line: Firefox has no flags for most of this.
- cat > "$PROFILE/user.js" <<'JS'
+ # The URL goes in the profile as the startup page, not only on the command
+ # line. A brand-new Firefox profile ignores the URL it is handed on its
+ # first run - it comes up on a blank window and never navigates,
+ # reproducibly and without saying anything - and a test that starts a
+ # browser to look at a page cannot use a warm profile without giving up the
+ # isolation the throwaway one buys.
+ cat > "$PROFILE/user.js" <<JS
+user_pref("browser.startup.homepage", "$URL");
+user_pref("browser.startup.page", 1);
+JS
+ cat >> "$PROFILE/user.js" <<'JS'
user_pref("app.update.enabled", false);
user_pref("app.update.auto", false);
user_pref("browser.shell.checkDefaultBrowser", false);
@@ -89,11 +115,16 @@ user_pref("browser.safebrowsing.malware.enabled", false);
user_pref("browser.safebrowsing.phishing.enabled", false);
user_pref("extensions.update.enabled", false);
user_pref("toolkit.startup.max_resumed_crashes", -1);
+/* Private browsing as a profile setting rather than -private-window. Both give
+ * private windows; only this one also opens the URL. A brand-new profile
+ * handed "-private-window <url>" comes up on about:privatebrowsing and never
+ * navigates - reproducible, and silent about it. */
+user_pref("browser.privatebrowsing.autostart", true);
JS
# -no-remote is not optional. Without it, a developer with Firefox already
# open has the URL handed to their real browser and this one exits, so the
# test measures nothing while appearing to work.
- set -- "$BIN" -profile "$PROFILE" -no-remote -private-window "$URL"
+ set -- "$BIN" -profile "$PROFILE" -no-remote "$URL"
COMMON="$COMMON MOZ_ENABLE_WAYLAND=1 MOZ_CRASHREPORTER_DISABLE=1"
;;
chromium|brave)
diff --git a/src/tests/wayland/e_wl_testkit.c b/src/tests/wayland/e_wl_testkit.c
index 33a6b8e2f..577ac95af 100644
--- a/src/tests/wayland/e_wl_testkit.c
+++ b/src/tests/wayland/e_wl_testkit.c
@@ -528,6 +528,13 @@ tk_wait_state(Tk *tk, const char *app_id, unsigned int mask, unsigned int want,
timeout_ms, what, last, want, mask);
}
+void
+tk_zone_add(Tk *tk, int x, int y, int w, int h)
+{
+ wl_test_zone_add(tk->tester, x, y, w, h);
+ tk_settle(tk);
+}
+
void
tk_action(Tk *tk, unsigned int id, const char *name, const char *params)
{
diff --git a/src/tests/wayland/e_wl_testkit.h b/src/tests/wayland/e_wl_testkit.h
index 6c9a08d74..3cf4689f9 100644
--- a/src/tests/wayland/e_wl_testkit.h
+++ b/src/tests/wayland/e_wl_testkit.h
@@ -130,6 +130,11 @@ Tk_Client *tk_wait_window(Tk *tk, const char *app_id_part, int timeout_ms);
Tk_Client *tk_wait_state(Tk *tk, const char *app_id, unsigned int mask,
unsigned int want, int timeout_ms, const char *what);
+/* Add a zone - a second monitor - to the compositor's right. The backends all
+ * come up with exactly one, so anything about moving a window between screens
+ * needs this to have something to move it to. */
+void tk_zone_add(Tk *tk, int x, int y, int w, int h);
+
/* Run one of E's own actions on a window. Empty or NULL params means none. */
void tk_action(Tk *tk, unsigned int id, const char *name, const char *params);
diff --git a/src/tests/wayland/meson.build b/src/tests/wayland/meson.build
index 510e08810..6169775d2 100644
--- a/src/tests/wayland/meson.build
+++ b/src/tests/wayland/meson.build
@@ -108,12 +108,13 @@ test_browser = executable('test_wl_browser',
foreach b: ['firefox', 'chromium', 'brave']
test('browser-' + b,
find_program('run-nested.sh'),
- args : [test_browser, b],
+ args : [test_browser, b, 'e-browser-probe'],
env : [
'E_TEST_BIN=' + e_test_bin,
'E_TEST_MODULE_SO=' + wl_test_module_so,
'E_TEST_MODULE_ARCH=' + module_arch,
- 'E_TEST_APP=@0@ @1@ about:blank'.format(browser_run[0].full_path(), b),
+ 'E_TEST_APP=@0@ @1@ file://@2@'.format(browser_run[0].full_path(), b,
+ meson.current_source_dir() / 'pages' / 'probe.html'),
],
suite : 'browser',
timeout: 300,
diff --git a/src/tests/wayland/pages/probe.html b/src/tests/wayland/pages/probe.html
new file mode 100644
index 000000000..516f08826
--- /dev/null
+++ b/src/tests/wayland/pages/probe.html
@@ -0,0 +1,5 @@
+<!doctype html>
+<title>e-browser-probe</title>
+<body style="margin:0;background:#2b6cb0;color:#fff;font:48px sans-serif">
+ <p style="padding:24px">e-browser-probe</p>
+</body>
diff --git a/src/tests/wayland/test_browser.c b/src/tests/wayland/test_browser.c
index fdc35a311..57727b777 100644
--- a/src/tests/wayland/test_browser.c
+++ b/src/tests/wayland/test_browser.c
@@ -87,7 +87,38 @@ main(int argc, char **argv)
}
x0 = c->x; y0 = c->y; w0 = c->w; h0 = c->h;
- printf("test-browser: baseline %dx%d+%d+%d\n", w0, h0, x0, y0);
+ printf("test-browser: baseline %dx%d+%d+%d title='%s'\n",
+ w0, h0, x0, y0, c->title);
+
+ /* Navigation, as far as the compositor can see it. argv[2] is a page whose
+ * <title> is known, so a title containing it means the browser resolved the
+ * URL, rendered it, and told the compositor about it - the whole path from
+ * "here is a link" to "there is a window showing it".
+ *
+ * Not fatal when it does not happen, and printed loudly when it does not.
+ * Firefox handed a URL on the command line with a brand-new profile comes
+ * up on a blank window and never navigates - reproducible here, silent
+ * about it, and unaffected by every documented way of asking (-url,
+ * --new-window, -private-window, the startup-page prefs). That is a browser
+ * behaviour and failing the compositor's test on it would be reporting the
+ * wrong thing. Chromium and Brave navigate and are held to it. */
+ if (argc > 2)
+ {
+ int i, navigated = 0;
+
+ for (i = 0; i * 200 < APPEAR_MS; i++)
+ {
+ c = tk_expect(tk, app_id);
+ if (strstr(c->title, argv[2])) { navigated = 1; break; }
+ tk_sync(tk);
+ }
+ if (navigated)
+ printf("test-browser: navigated, title '%s'\n", c->title);
+ else
+ printf("test-browser: NOT ASSERTED: '%s' never showed '%s' in its "
+ "title, so this run says nothing about navigation\n",
+ app_id, argv[2]);
+ }
/* ---------------------------------------------------------- maximise */
@@ -147,6 +178,30 @@ main(int argc, char **argv)
tk_action(tk, id, "window_resize_by", "-120 -90");
c = tk_expect(tk, app_id);
+ /* ------------------------------------------------- another screen */
+
+ /* A second zone, and the window moved onto it. Every backend comes up with
+ * one screen, so without wl_test.zone_add there is nothing to move to -
+ * and "drag the browser to the other monitor" is a thing people do all day
+ * and a thing that has to survive a client that decorates itself. */
+ tk_zone_add(tk, 1024, 0, 1024, 768);
+ tk_action(tk, id, "window_zone_move_by", "1");
+
+ c = tk_expect(tk, app_id);
+ if (c->output == 0)
+ tk_fail(tk, "after moving to the next screen the window is still on "
+ "output 0, at %dx%d+%d+%d", c->w, c->h, c->x, c->y);
+ if (c->x < 1024)
+ tk_fail(tk, "the window says it is on output %u but sits at +%d+%d, "
+ "which is on the first screen", c->output, c->x, c->y);
+ if ((c->w != w0) || (c->h != h0))
+ tk_fail(tk, "moving to another screen resized the window to %dx%d, "
+ "was %dx%d", c->w, c->h, w0, h0);
+ printf("test-browser: moved to output %u at +%d+%d\n", c->output, c->x, c->y);
+
+ tk_action(tk, id, "window_zone_move_by", "-1");
+ c = tk_expect(tk, app_id);
+
/* ----------------------------------------------------------- iconify */
tk_action(tk, id, "window_iconic_toggle", NULL);
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
