(Hidpi) Crash w segfault on confirmation in file conflict dialog #1630
Comments
|
I got this backtrace by turning off caja in session>required_components and running caja under gdb in a new session: |
|
This just started, so we have something invoking (or something IN) gtk_widget_get_scale_factor () not liking a change in some underlying lib. I have locally compiled GTK 3.24.34 and glib 2.72.1, just updated to builds of these today from older versions with no change in this behavior. Using a 4K monitor, tested both with gtk_window_scaling=2 and gtk_window_scaling=1 with no change in behavior. Will update if laptop with 1080x1920 monitor does not also do this |
|
Just confirmed this does NOT happen on my laptop with a 1080x1920 monitor and window-scaling=1 |
lukefromdc
changed the title
|
Quick glance at the code suggests |
|
I don't know how to switch to frame 1
|
|
It's worth noting that this crash/segfault occurs only on "replace" not "cancel" or "skip"
Two things have to be happening at that point: the dialog is being destroyed, and the file is
being copied.
I also found that just like on the laptop (never gets this crash) on the desktop, changing window scaling
or screen resolution has no effect on the crash, the physical monitor of course remains a 4K monitor.
I am wondering if some kind of race condition in the destruction of the dialog is to blame here
|
|
Also I just found that Nemo also has this crash, and Nautius (which uses different confirmation dialog) does not.
|
|
In testing I found that I could halt the crashes by entirely emptying the function
|
|
Looks like at this point in execution, both
If I hardcode the scale to 2, the backtrace changes to indicating a If I disable everything using one of these variables, the backtrace Some data must in these variables though(even if it is garbage), as null checking with |
(gdb) frame 1
(gdb) print fcd
(gdb) print fcd->details
(gdb) print fcd->details->dest_imageBut I have a feeling Does something like this help? diff --git a/libcaja-private/caja-file-conflict-dialog.c b/libcaja-private/caja-file-conflict-dialog.c
index 9d86d15c..ff0386a3 100644
--- a/libcaja-private/caja-file-conflict-dialog.c
+++ b/libcaja-private/caja-file-conflict-dialog.c
@@ -370,10 +370,10 @@ file_list_ready_cb (GList *files,
caja_file_monitor_add (src, fcd, CAJA_FILE_ATTRIBUTES_FOR_ICON);
caja_file_monitor_add (dest, fcd, CAJA_FILE_ATTRIBUTES_FOR_ICON);
- details->src_handler_id = g_signal_connect (src, "changed",
- G_CALLBACK (file_icons_changed), fcd);
- details->dest_handler_id = g_signal_connect (dest, "changed",
- G_CALLBACK (file_icons_changed), fcd);
+ details->src_handler_id = g_signal_connect_object (src, "changed",
+ G_CALLBACK (file_icons_changed), fcd, 0);
+ details->dest_handler_id = g_signal_connect_object (dest, "changed",
+ G_CALLBACK (file_icons_changed), fcd, 0);
}
static void |
|
I will have to try that tomorrow, on the road now, omly the desktop his this issue, and it hss to be run while we have sunlight as we are off the grid and rely on solar panels.
|
|
Got this on your test: |
|
Still a segfault with your suggested change which in turn makes |
Apparently |
|
First, I think a good approach would be to have two callbacks instead of one, splitting the current callback function into two, one for the source and another for the destination. current cb function: static void
file_icons_changed (CajaFile *file,
CajaFileConflictDialog *fcd)
{
cairo_surface_t *surface;
surface = caja_file_get_icon_surface (fcd->details->destination,
CAJA_ICON_SIZE_LARGE,
FALSE,
gtk_widget_get_scale_factor (fcd->details->dest_image),
CAJA_FILE_ICON_FLAGS_USE_THUMBNAILS);
gtk_image_set_from_surface (GTK_IMAGE (fcd->details->dest_image), surface);
cairo_surface_destroy (surface);
surface = caja_file_get_icon_surface (fcd->details->source,
CAJA_ICON_SIZE_LARGE,
FALSE,
gtk_widget_get_scale_factor (fcd->details->src_image),
CAJA_FILE_ICON_FLAGS_USE_THUMBNAILS);
gtk_image_set_from_surface (GTK_IMAGE (fcd->details->src_image), surface);
cairo_surface_destroy (surface);
} |
|
@rbuj that would probably make sense as there's no point in updating both if only one changed, but that shouldn't be the root issue here I think. |
diff --git a/libcaja-private/caja-file-conflict-dialog.c b/libcaja-private/caja-file-conflict-dialog.c
index 9d86d15c..233f47ae 100644
--- a/libcaja-private/caja-file-conflict-dialog.c
+++ b/libcaja-private/caja-file-conflict-dialog.c
@@ -68,27 +68,18 @@ G_DEFINE_TYPE_WITH_PRIVATE (CajaFileConflictDialog,
GTK_TYPE_DIALOG);
static void
-file_icons_changed (CajaFile *file,
- CajaFileConflictDialog *fcd)
+file_icons_changed (CajaFile *file,
+ GtkWidget *widget)
{
cairo_surface_t *surface;
- surface = caja_file_get_icon_surface (fcd->details->destination,
+ surface = caja_file_get_icon_surface (file,
CAJA_ICON_SIZE_LARGE,
FALSE,
- gtk_widget_get_scale_factor (fcd->details->dest_image),
+ gtk_widget_get_scale_factor (widget),
CAJA_FILE_ICON_FLAGS_USE_THUMBNAILS);
- gtk_image_set_from_surface (GTK_IMAGE (fcd->details->dest_image), surface);
- cairo_surface_destroy (surface);
-
- surface = caja_file_get_icon_surface (fcd->details->source,
- CAJA_ICON_SIZE_LARGE,
- FALSE,
- gtk_widget_get_scale_factor (fcd->details->src_image),
- CAJA_FILE_ICON_FLAGS_USE_THUMBNAILS);
-
- gtk_image_set_from_surface (GTK_IMAGE (fcd->details->src_image), surface);
+ gtk_image_set_from_surface (GTK_IMAGE (widget), surface);
cairo_surface_destroy (surface);
}
@@ -371,9 +362,11 @@ file_list_ready_cb (GList *files,
caja_file_monitor_add (dest, fcd, CAJA_FILE_ATTRIBUTES_FOR_ICON);
details->src_handler_id = g_signal_connect (src, "changed",
- G_CALLBACK (file_icons_changed), fcd);
+ G_CALLBACK (file_icons_changed),
+ fcd->details->src_image);
details->dest_handler_id = g_signal_connect (dest, "changed",
- G_CALLBACK (file_icons_changed), fcd);
+ G_CALLBACK (file_icons_changed),
+ fcd->details->dest_image);
}
static void |
|
PR #1632 |
|
@lukefromdc I'm nor sure if that could account for the issue, but Valgrind pointed out to me an issue, it looks like the scale in the icon cache key is no properly populated. Could you give this a shot? diff --git a/libcaja-private/caja-icon-info.c b/libcaja-private/caja-icon-info.c
index cf690a6e..ee52e4bd 100644
--- a/libcaja-private/caja-icon-info.c
+++ b/libcaja-private/caja-icon-info.c
@@ -307,6 +307,7 @@ icon_key_new (GIcon *icon,
key = g_slice_new (IconKey);
key->icon = g_object_ref (icon);
+ key->scale = scale;
key->size = size;
return key; |
|
Still got a crash with the last patch posted here applied. The again hardcoded scale to 2 to try and get past it, got another crash. This time the backtrace showed |
|
Still beats me… maybe you could try and run under Valgrind's memcheck to see if it could give us the initial source of invalid pointer? Simply |
|
Got this result, again with that last patch applied: |
|
OK, it really looks like those widgets are destroyed and the callbacks still hit. Could you try using Also, there's a pretty scary libmate-desktop issue as well showing up here, maybe you could try and build with debugging symbols to get more info? |
|
Getting out of my skill range here |
|
could you paste your current diff? |
|
|
and what is the HEAD commit? (e.g. on what is this a diff) |
|
Current master, again you are getting past my skillset |
|
I just pulled a copy of the file out of my last build (from master as of today) and applied the patch |
|
|
|
#1632 also had no effect |
|
yeah #1632 is basically the diff you pasted… this shouldn't really help (but might avoid the crash in a sad way thanks to the checks -- not a real solution), but let's see: diff --git a/libcaja-private/caja-file-conflict-dialog.c b/libcaja-private/caja-file-conflict-dialog.c
index 9d86d15c..574c926a 100644
--- a/libcaja-private/caja-file-conflict-dialog.c
+++ b/libcaja-private/caja-file-conflict-dialog.c
@@ -68,27 +68,18 @@ G_DEFINE_TYPE_WITH_PRIVATE (CajaFileConflictDialog,
GTK_TYPE_DIALOG);
static void
-file_icons_changed (CajaFile *file,
- CajaFileConflictDialog *fcd)
+file_icons_changed (CajaFile *file,
+ GtkWidget *widget)
{
cairo_surface_t *surface;
- surface = caja_file_get_icon_surface (fcd->details->destination,
+ surface = caja_file_get_icon_surface (file,
CAJA_ICON_SIZE_LARGE,
FALSE,
- gtk_widget_get_scale_factor (fcd->details->dest_image),
+ gtk_widget_get_scale_factor (widget),
CAJA_FILE_ICON_FLAGS_USE_THUMBNAILS);
- gtk_image_set_from_surface (GTK_IMAGE (fcd->details->dest_image), surface);
- cairo_surface_destroy (surface);
-
- surface = caja_file_get_icon_surface (fcd->details->source,
- CAJA_ICON_SIZE_LARGE,
- FALSE,
- gtk_widget_get_scale_factor (fcd->details->src_image),
- CAJA_FILE_ICON_FLAGS_USE_THUMBNAILS);
-
- gtk_image_set_from_surface (GTK_IMAGE (fcd->details->src_image), surface);
+ gtk_image_set_from_surface (GTK_IMAGE (widget), surface);
cairo_surface_destroy (surface);
}
@@ -370,10 +361,12 @@ file_list_ready_cb (GList *files,
caja_file_monitor_add (src, fcd, CAJA_FILE_ATTRIBUTES_FOR_ICON);
caja_file_monitor_add (dest, fcd, CAJA_FILE_ATTRIBUTES_FOR_ICON);
- details->src_handler_id = g_signal_connect (src, "changed",
- G_CALLBACK (file_icons_changed), fcd);
- details->dest_handler_id = g_signal_connect (dest, "changed",
- G_CALLBACK (file_icons_changed), fcd);
+ details->src_handler_id = g_signal_connect_object (src, "changed",
+ G_CALLBACK (file_icons_changed),
+ fcd->details->src_image, 0);
+ details->dest_handler_id = g_signal_connect_object (dest, "changed",
+ G_CALLBACK (file_icons_changed),
+ fcd->details->dest_image, 0);
}
static void |
|
That last patch did stop the crash in my tests |
|
wow, okay, interesting. Could you run under Valrgind to see if there's still a problem, just less serious? |
|
Got this under valgrind: |
|
OK, so that looks good. Weird you don't get things like |
|
I closed #1632 so I don't want to lock the fix |
|
@lukefromdc Could you try the diff below and tell me what happens? I'd really like to actually understand what's going on, although I'll end up (also?) submitting a version of the above that worked for you. diff --git a/libcaja-private/caja-file-conflict-dialog.c b/libcaja-private/caja-file-conflict-dialog.c
index 9d86d15c..4228a900 100644
--- a/libcaja-private/caja-file-conflict-dialog.c
+++ b/libcaja-private/caja-file-conflict-dialog.c
@@ -679,33 +679,45 @@ caja_file_conflict_dialog_init (CajaFileConflictDialog *fcd)
}
static void
-do_finalize (GObject *self)
+do_dispose (GObject *self)
{
CajaFileConflictDialogPrivate *details =
CAJA_FILE_CONFLICT_DIALOG (self)->details;
- g_free (details->conflict_name);
-
if (details->handle != NULL)
{
caja_file_list_cancel_call_when_ready (details->handle);
+ details->handle = NULL;
}
if (details->src_handler_id)
{
g_signal_handler_disconnect (details->source, details->src_handler_id);
caja_file_monitor_remove (details->source, self);
+ details->src_handler_id = 0;
}
if (details->dest_handler_id)
{
g_signal_handler_disconnect (details->destination, details->dest_handler_id);
caja_file_monitor_remove (details->destination, self);
+ details->dest_handler_id = 0;
}
- caja_file_unref (details->source);
- caja_file_unref (details->destination);
- caja_file_unref (details->dest_dir);
+ g_clear_pointer (&details->source, caja_file_unref);
+ g_clear_pointer (&details->destination, caja_file_unref);
+ g_clear_pointer (&details->dest_dir, caja_file_unref);
+
+ G_OBJECT_CLASS (caja_file_conflict_dialog_parent_class)->dispose (self);
+}
+
+static void
+do_finalize (GObject *self)
+{
+ CajaFileConflictDialogPrivate *details =
+ CAJA_FILE_CONFLICT_DIALOG (self)->details;
+
+ g_free (details->conflict_name);
G_OBJECT_CLASS (caja_file_conflict_dialog_parent_class)->finalize (self);
}
@@ -713,6 +725,7 @@ do_finalize (GObject *self)
static void
caja_file_conflict_dialog_class_init (CajaFileConflictDialogClass *klass)
{
+ G_OBJECT_CLASS (klass)->dispose = do_dispose;
G_OBJECT_CLASS (klass)->finalize = do_finalize;
}
|
|
This stopped the crash, didn't get any known file conflict dialog issues running this
On 6/2/2022 at 9:29 AM, "Colomban Wendling" ***@***.***> wrote:
***@***.*** Could you try the diff below and tell me what happens?
… I'd really like to actually understand what's going on, although
I'll end up (also?) submitting a version of the above that worked
for you.
```diff
diff --git a/libcaja-private/caja-file-conflict-dialog.c b/libcaja-
private/caja-file-conflict-dialog.c
index 9d86d15c..4228a900 100644
--- a/libcaja-private/caja-file-conflict-dialog.c
+++ b/libcaja-private/caja-file-conflict-dialog.c
@@ -679,33 +679,45 @@ caja_file_conflict_dialog_init
(CajaFileConflictDialog *fcd)
}
static void
-do_finalize (GObject *self)
+do_dispose (GObject *self)
{
CajaFileConflictDialogPrivate *details =
CAJA_FILE_CONFLICT_DIALOG (self)->details;
- g_free (details->conflict_name);
-
if (details->handle != NULL)
{
caja_file_list_cancel_call_when_ready (details->handle);
+ details->handle = NULL;
}
if (details->src_handler_id)
{
g_signal_handler_disconnect (details->source, details-
>src_handler_id);
caja_file_monitor_remove (details->source, self);
+ details->src_handler_id = 0;
}
if (details->dest_handler_id)
{
g_signal_handler_disconnect (details->destination,
details->dest_handler_id);
caja_file_monitor_remove (details->destination, self);
+ details->dest_handler_id = 0;
}
- caja_file_unref (details->source);
- caja_file_unref (details->destination);
- caja_file_unref (details->dest_dir);
+ g_clear_pointer (&details->source, caja_file_unref);
+ g_clear_pointer (&details->destination, caja_file_unref);
+ g_clear_pointer (&details->dest_dir, caja_file_unref);
+
+ G_OBJECT_CLASS (caja_file_conflict_dialog_parent_class)-
>dispose (self);
+}
+
+static void
+do_finalize (GObject *self)
+{
+ CajaFileConflictDialogPrivate *details =
+ CAJA_FILE_CONFLICT_DIALOG (self)->details;
+
+ g_free (details->conflict_name);
G_OBJECT_CLASS (caja_file_conflict_dialog_parent_class)-
>finalize (self);
}
@@ -713,6 +725,7 @@ do_finalize (GObject *self)
static void
caja_file_conflict_dialog_class_init (CajaFileConflictDialogClass
*klass)
{
+ G_OBJECT_CLASS (klass)->dispose = do_dispose;
G_OBJECT_CLASS (klass)->finalize = do_finalize;
}
```
--
Reply to this email directly or view it on GitHub:
#1630 (comment)-
1144648715
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
cwendling
added a commit
that referenced
this issue
Jun 7, 2022
cwendling
added a commit
that referenced
this issue
Jun 7, 2022
Instead of manually keeping tabs on the signals so we can disconnect them before the data parameter gets destroyed, let GObject automatically track lifetime of the data, which it can do as that data is a GObject itself. This does not change behavior in the normal case, but makes sure the callback simply cannot get called with invalid/freed parameters, even if we did screw anything up (which we used to). This actually would have solved #1630 as well with using the target widgets as data parameters as the signal would have been disconnected as soon as the widget got destroyed, no matter whether we got finalized ourselves or not. The signal IDs were also use as guards to whether the monitor was set up for the related files, but we can just as well use the state of the file list ready handle which should only be NULL when we actually have monitors set up. Even if it wasn't the case, worse case scenario would be removing a non-existent monitor, which is perfectly OK anyway.
lukefromdc
pushed a commit
that referenced
this issue
Jun 8, 2022
lukefromdc
pushed a commit
that referenced
this issue
Jun 8, 2022
Instead of manually keeping tabs on the signals so we can disconnect them before the data parameter gets destroyed, let GObject automatically track lifetime of the data, which it can do as that data is a GObject itself. This does not change behavior in the normal case, but makes sure the callback simply cannot get called with invalid/freed parameters, even if we did screw anything up (which we used to). This actually would have solved #1630 as well with using the target widgets as data parameters as the signal would have been disconnected as soon as the widget got destroyed, no matter whether we got finalized ourselves or not. The signal IDs were also use as guards to whether the monitor was set up for the related files, but we can just as well use the state of the file list ready handle which should only be NULL when we actually have monitors set up. Even if it wasn't the case, worse case scenario would be removing a non-existent monitor, which is perfectly OK anyway.
|
You should cherry-pick/packport fixes to 1.26 branch, in result i can make a new release, |
lukefromdc
pushed a commit
that referenced
this issue
Jun 19, 2022
lukefromdc
pushed a commit
that referenced
this issue
Jun 19, 2022
Instead of manually keeping tabs on the signals so we can disconnect them before the data parameter gets destroyed, let GObject automatically track lifetime of the data, which it can do as that data is a GObject itself. This does not change behavior in the normal case, but makes sure the callback simply cannot get called with invalid/freed parameters, even if we did screw anything up (which we used to). This actually would have solved #1630 as well with using the target widgets as data parameters as the signal would have been disconnected as soon as the widget got destroyed, no matter whether we got finalized ourselves or not. The signal IDs were also use as guards to whether the monitor was set up for the related files, but we can just as well use the state of the file list ready handle which should only be NULL when we actually have monitors set up. Even if it wasn't the case, worse case scenario would be removing a non-existent monitor, which is perfectly OK anyway.
|
Cherrypicked all three commit after local test build and run went fine. |
raveit65
pushed a commit
that referenced
this issue
Jul 21, 2022
raveit65
pushed a commit
that referenced
this issue
Jul 21, 2022
Instead of manually keeping tabs on the signals so we can disconnect them before the data parameter gets destroyed, let GObject automatically track lifetime of the data, which it can do as that data is a GObject itself. This does not change behavior in the normal case, but makes sure the callback simply cannot get called with invalid/freed parameters, even if we did screw anything up (which we used to). This actually would have solved #1630 as well with using the target widgets as data parameters as the signal would have been disconnected as soon as the widget got destroyed, no matter whether we got finalized ourselves or not. The signal IDs were also use as guards to whether the monitor was set up for the related files, but we can just as well use the state of the file list ready handle which should only be NULL when we actually have monitors set up. Even if it wasn't the case, worse case scenario would be removing a non-existent monitor, which is perfectly OK anyway.
|
Also note that on a LONG file replacement jog such as a bunch of video files, the crash may occur partially
through or even near the end of the file transfer job, the progress bar appears normally as file conflict dialog
disappears. The backtrace showed the file conflict dialog as the problem though
|
|
@lukefromdc do you mean the issue is actually not resolved? If so, would you have a backtrace of the case you mention, e.g. long after the confirmation dialog closed? Or any backtrace on top of those fixes for that matter. |
|
No, this was before the issue was fixed. I haven't had that crash once since the fix was merged
|
|
Also note that may previous comment here that was posted long ago seem to have just shown up today
|
|
ah OK, weird, but cool then :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Expected behaviour
When replacing a file by moving or copying from another directory with another file of the same name, thus invoking the file conflict dialog,, clicking "replace" should replace the file and caja should keep running normally
Actual behaviour
Clicking "replace" causes caja to crash instantly, then restart as normal in a MATE session. On opening the file, it is clear the file replacement has succeeded.
Steps to reproduce the behaviour
Copy any short file (short is all I tested) to another directory, then move or copy it back to the original directory
MATE general version
1.26
Package version
locally compiled git master current as of March 22 (no change since then according to git pull)
Linux Distribution
Debian Unstable updated on or after May 15 2022
Link to bugreport of your Distribution (requirement)
None as this is a locally compiled package
The text was updated successfully, but these errors were encountered: