Skip to content

nemo-places-sidebar: fetch free-space info asynchronously (#3764) - #3833

Open
leigh123linux wants to merge 1 commit into
linuxmint:masterfrom
leigh123linux:mtp_freeze
Open

nemo-places-sidebar: fetch free-space info asynchronously (#3764)#3833
leigh123linux wants to merge 1 commit into
linuxmint:masterfrom
leigh123linux:mtp_freeze

Conversation

@leigh123linux

Copy link
Copy Markdown
Member

Free-space info for Home, File System, mounted volumes, and
bookmarks was fetched with a single blocking GIO call per row from
update_places() on the main thread. A gvfs mount that never answers
(e.g. an Android phone connected via MTP with "No data transfer")
froze the whole sidebar - and with it, every open Nemo window -
indefinitely.

Rows are now added with no free-space info, and an async
g_file_query_filesystem_info_async() call fills it in later if the
backend answers before the row is gone. Bookmarks pointing into a
mount are covered the same way, since they were exposed to the same
hang; the default XDG bookmarks (Pictures, Documents, etc.) are left
alone, as they're always plain subdirectories of Home rather than
distinct mounts.

mtp:// and gphoto2:// mounts are excluded from this outright rather
than queried and relied upon to error or cancel cleanly. Testing
against a real device showed that a stuck query on these backends
doesn't actually get freed by cancellation - gdb still shows a
thread parked inside the MTP backend indefinitely - so retrying or
tracking such a request is pointless and, if repeated, risks pinning
GIO's shared worker-thread pool on a backend that's never coming
back. No thread or timeout is needed for the general case: the
async call itself never blocks the caller regardless of whether or
when a well-behaved backend responds.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The async callback can dereference disposed sidebar state (notably sidebar->store) if a backend completes successfully after disposal/cancellation, risking a crash.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR prevents Nemo’s places sidebar (and thus open Nemo windows) from freezing when querying filesystem free-space info for certain mounts by moving those queries off the main thread and populating the UI first, then filling in disk usage later.

Changes:

  • Replace synchronous g_file_query_filesystem_info() “per row” calls with g_file_query_filesystem_info_async() and a row-reference based callback update.
  • Track and cancel in-flight free-space queries via a shared GCancellable across sidebar refreshes / disposal.
  • Skip free-space queries entirely for known-problematic backends (mtp://, gphoto2://).
File summaries
File Description
src/nemo-places-sidebar.c Adds async free-space querying with cancellation and unsafe-backend exclusion to avoid UI hangs.
Review details

Suppressed comments (1)

src/nemo-places-sidebar.c:534

  • query_disk_full_async() stores a raw sidebar pointer in DiskFullQueryData. If the sidebar gets disposed while an async query is still in flight and a backend ignores cancellation (or completes after dispose), the callback can dereference freed memory. Take a strong reference when enqueuing the async request.
    data = g_new0 (DiskFullQueryData, 1);
    data->sidebar = sidebar;
    data->base_tooltip = g_strdup (base_tooltip);
    data->show_df_when_available = show_df_when_available;
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/nemo-places-sidebar.c
Comment thread src/nemo-places-sidebar.c
Comment on lines +474 to +481
if (gtk_tree_row_reference_valid (data->row_ref)) {
NemoPlacesSidebar *sidebar = data->sidebar;
GtkTreePath *path = gtk_tree_row_reference_get_path (data->row_ref);
GtkTreeIter iter;

if (gtk_tree_model_get_iter (GTK_TREE_MODEL (sidebar->store), &iter, path)) {
guint64 k_used, k_total, k_free;
gint df_percent = -1;

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The async query user_data does not take a reference to sidebar despite unref’ing it later, which can lead to refcount underflow and use-after-free during/after dispose.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/nemo-places-sidebar.c

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new async query user-data holds a strong sidebar reference that can prevent teardown if a backend never completes, and the new tooltip assembly should escape strings since tooltips are rendered as markup.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread src/nemo-places-sidebar.c
Comment on lines +436 to +440
typedef struct {
NemoPlacesSidebar *sidebar;
GtkTreeRowReference *row_ref;
gchar *base_tooltip;
gboolean show_df_when_available;
Comment thread src/nemo-places-sidebar.c
Comment on lines +499 to +500
free_line = g_strdup_printf (_("Free space: %s"), size_string);
tooltip = g_strdup_printf ("%s\n%s", data->base_tooltip, free_line);
Comment thread src/nemo-places-sidebar.c
int df_percent,
gboolean show_df_percent,
GFile *df_file,
gboolean show_df_when_available,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is show_df_when_available even necessary? It's only ever TRUE if df_file != NULL isn't it? So we wouldn't be running query_disk_full_async() on a location we didn't want show it in the first place...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Home is the one place they diverge. df_file is always non-NULL for Home (we always want to query its free space, for the tooltip), but show_df_when_available there is home_on_different_fs(mount_uri) — independently FALSE when Home is on the same filesystem as root:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants