Dragging item from archiver onto pcmanfm's side pane freezes the archiver

[CasperVector]

• Open pcmanfm with the side pane enabled, open a compressed file with engrampa, and drag an item from the compressed file across the side pane.
• As soon as the mouse pointer reaches the side pane, even before the item is dropped, an error dialog shows up; meanwhile, the shape of the pointer changes from the normal "hand" (if originally outside of the file list area in pcmanfm) or the "hand with plus sign" (if originally inside the file list) to the "hand with chain sign".
• The desktop becomes unresponsive to mouse clicks and most keystrokes, until engrampa is killed, after which the pointer shape returns normal. Similar symptom also happens with xarchiver, but not with thunar.
• The issue can be worked around by reverting the changes in 1d7738c; actually, disabling the if(!dd->waiting_data) { ... } branch when either dest or dest_path is zero prevents the symptom from occuring. However, I am totally unfamiliar with the drag-and-drop protocols, and therefore unsure whether this fixes the root cause of this issue and does not cause undesirable side effects.

[CasperVector]

A patch which seems most unintrusive to me:

diff -ur libfm-1.3.2/src/gtk/fm-dnd-dest.c libfm-1.3.2/src/gtk/fm-dnd-dest.c
--- libfm-1.3.2/src/gtk/fm-dnd-dest.c	2022-10-31 01:05:49.931057670 +0800
+++ libfm-1.3.2/src/gtk/fm-dnd-dest.c	2022-10-31 01:07:27.667579011 +0800
@@ -979,7 +979,7 @@
         if (dd->context != drag_context)
             clear_src_cache(dd);
         action = 0;
-        if(!dd->waiting_data) /* we're still waiting for "drag-data-received" signal */
+        if(dest_path && !dd->waiting_data) /* we're still waiting for "drag-data-received" signal */
         {
             /* retrieve the source files */
             gtk_drag_get_data(dd->widget, drag_context, target, time(NULL));

[CasperVector]

Sorry, incomplete check...

diff -ur libfm-1.3.2/src/gtk/fm-dnd-dest.c libfm-1.3.2/src/gtk/fm-dnd-dest.c
--- libfm-1.3.2/src/gtk/fm-dnd-dest.c	2022-10-31 01:05:49.931057670 +0800
+++ libfm-1.3.2/src/gtk/fm-dnd-dest.c	2022-10-31 01:07:27.667579011 +0800
@@ -979,7 +979,7 @@
         if (dd->context != drag_context)
             clear_src_cache(dd);
         action = 0;
-        if(!dd->waiting_data) /* we're still waiting for "drag-data-received" signal */
+        if(dest && dest_path && !dd->waiting_data) /* we're still waiting for "drag-data-received" signal */
         {
             /* retrieve the source files */
             gtk_drag_get_data(dd->widget, drag_context, target, time(NULL));

[dirkf]

Did you build and test this? It doesn't seem to have a PR yet.

[cwendling]

@CasperVector it probably will work, but doesn't that just effectively revert 1d7738c? I'm not sure the reason behind that commit, and it's from 2012, but it sill might be interesting to see why that was done like so.

[fulalas]

@CasperVector, I just tried your patch, but unfortunately it didn't fix the issue for me. Once I drag a file from a extractor application like engrampa over pcmanfm panel, it shows this error just like it happens without your patch:

I also tried to combine with this other patch mentioned here but it didn't change anything :(

[CasperVector]

@CasperVector it probably will work, but doesn't that just effectively revert 1d7738c? I'm not sure the reason behind that commit, and it's from 2012, but it sill might be interesting to see why that was done like so.

Sorry, I did not research (and now probably cannot afford to; I have been extremely busy with my day job for nearly an entire year) the commit you mentioned. But I can confirm my patch works on Devuan Chimaera and Devuan Daedalus (and probably the corresponding Debian releases), both against libfm 1.3.2.

[CasperVector]

@CasperVector, I just tried your patch, but unfortunately it didn't fix the issue for me. Once I drag a file from a extractor application like engrampa over pcmanfm panel, it shows this error just like it happens without your patch:

I also tried to combine with this other patch mentioned here but it didn't change anything :(

I think the error popup is expected when you (drag and) drop an entry in a compressed file onto the pcmanfm's side pane; what my patch does is to prevent the desktop freeze when the entry is dragged across the pane. I do not know which you mean, but as is said in the previous message, the patch is confirmed to work in the environments I use.

[dirkf]

... the error popup is expected ...

That's what I thought.

[fulalas]

what my patch does is to prevent the desktop freeze

Now I see!

the error popup is expected when you (drag and) drop

Sure, but it happens just by hovering (no drop) over the panel, which clearly is not the expected behavior.

[CasperVector]

Sure, but it happens just by hovering (no drop) over the panel, which clearly is not the expected behavior.

That is exactly what my patch attempts to prevent. So both my patch and the other patch you mentioned probably need some refinement, preferably by someone capable (and having the time) of debugging your case (either locally or remotely).

[fulalas]

This is intriguing. Are you building against GTK2 or GTK3?

[CasperVector]

I think it should be GTK3; my Devuan Chimaera (previous) and Devuan Daedalus (current) enviroments do not use GTK2.

[fulalas]

@CasperVector, I think I found the cause of the issue. Try this patch -- don't need to use your patch nor this one.

--- src/gtk/fm-places-view.c    2024-10-14 15:43:41.175374415 +0200
+++ src/gtk/fm-places-view.c.patched    2024-10-14 15:43:34.074374079 +0200
@@ -251,6 +251,15 @@
     gboolean ret = FALSE;
     GdkDragAction action = 0;
 
+    GdkWindow* source_window = gdk_drag_context_get_source_window(drag_context);
+    GtkWidget* source_widget = NULL;
+    if (source_window != NULL) {
+        gdk_window_get_user_data(source_window, (gpointer*)&source_widget);
+
+        if (source_widget == NULL)
+            return FALSE;
+    }
+
     target = gtk_drag_dest_find_target(dest_widget, drag_context, NULL);
     if(target == GDK_NONE)
         return FALSE;

Explanation: engrampa window (and potentially other extractors) has source_widget == NULL so it causes the error. What my patch does is preventing any action when source_widget == NULL.

After applying my patch, I tested drag and dropping from engrampa to the file section of pcmanfm and it's still extracting, so no regression. Also, when drag and dropping a folder from pcmanfm to the side panel, it's still creating a bookmark, so again no regression here.

Let me know if you find any issues.

Thanks!

[CasperVector]

I can confirm your latest patch also prevents the freeze, and (just like in your test) does not suffer from the two regressions. Even better, it also eliminates the error popup in your screenshot, and corresponds to a clear explanation of the root cause. Many thanks!

[fulalas]

Cool!

[dirkf]

In the library source file gtkdragdest.c, gtk_drag_dest_find_target() gets the source widget (if any) using gtk_drag_get_source_widget(). That seems to be a documented part of the API.

What happens if the patch is made like this:

     gboolean ret = FALSE;
     GdkDragAction action = 0;
 
+    GtkWidget* source_widget = gtk_drag_get_source_widget (drag_context);
+    if (source_widget == NULL) {
+        return FALSE;
+    }
+
     target = gtk_drag_dest_find_target(dest_widget, drag_context, NULL);
     if(target == GDK_NONE)
         return FALSE;

One possibility is that the GTK call hangs because that's where it hangs in the bug. Otherwise it's a few lines shorter (and could be 1 line shorter by collapsing the declaration: if (NULL == gtk_drag_get_source_widget (drag_context)) { ....

[fulalas]

@dirkf, even better! I confirm it works!

[CasperVector]

I can confirm the same. Thanks!

[dirkf]

So this is a solution that works for PCManFM without affecting other packages: time for a PR?

The originally suggested patch seems to be good practice too, even if it doesn't solve the issue, so maybe include both commits?

[fulalas]

So this is a solution that works for PCManFM without affecting other packages: time for a PR?

Well, it might affect packages that depend on libfm, although very unlikely to cause any issue due to the nature of the changes we made.

The originally suggested patch seems to be good practice too, even if it doesn't solve the issue, so maybe include both commits?

I don't see any need for this other patch made by zhuyaliang. I wouldn't include it.

But yes, I'm happy to suggest your code in a PR. Maybe just removing the curly brackets to keep consistency across the code. :)

[dirkf]

That was just the Gnome style that I pulled in. Some style guides mandate {} even for a single statement block, to avoid confusion, eg if additional statements are added later, and, reluctantly, I've come to agree. But unless it's dangerously bad, maintainers should match the existing style.

I also don't much like val == NULL (vs !val) since NULL in C should be a null pointer constant (0, or ((void*)0)) that always compares falsy in a conditional expression. In other languages with multiple null/falsy values, comparing to any one of them is a "code smell", but maybe the Gnomes have/had to support some non-conforming C compiler with a truthy NULL; the principle above applies.

Unfortunately we still haven't discovered the dangerous bug, whereby GTK could hang the UI when this patch is absent. Probably we won't, now, but, if calling gtk_drag_dest_find_target() is what provoked the hang, there are these possibilities:

• gtk_drag_get_source_widget() hangs: but we know it doesn't (:-)
• gtk_drag_dest_get_target_list() hangs
• gdk_drag_context_list_targets() hangs
• one of the object lists returned by the above two being compared in the nested loop must somehow have become self-linked (there are no checks against this).

Since GTK must be many times more exercised than PCManFM, I wonder if the latter might be incorrectly setting some data (the possible targets in the Places widget, say) so as to pass to or cause a circular list in GTK. Some years ago the Windows user APIs were overhauled to check user-supplied data more strictly; I think I would want my library code to validate a user-supplied linked list before processing it or handing it out to other API users.