467405 - FIFO pipe in directory hangs file selector

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Scott]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4

Firefox hangs when file selector navigates to a _directory_ where the FIFO pipe is held. 

Reproducible: Always

Steps to Reproduce:
mkdir testdir
cd testdir
mknod backpipe p

---

1. In firefox: open a file selection dialog from a website (attach file in gmail, etc.)
2. navigate to directory where pipe is stored (not necessary to navigate to the pipe, just the containing directory)
3. file selector hangs. 
Actual Results:  
hang

Expected Results:  
show files

64bit centos 5.1

Comment 1

[Tyler Downer [He/Him]]

This bug was reported using Firefox 3.0 or older, which is no longer supported. The bug has also not been changed in over 500 days and is still in UNCO.
Reporter, please retest this bug in Firefox 3.6.10 or later using a fresh profile, http://support.mozilla.com/en-US/kb/managing+profiles. If you still see this problem, please update the bug. If you no longer see the bug, please set the resolution to RESOLVED, WORKSFORME.

This is a mass search of unconfirmed bugs that have no activity on them, so if you feel a bug was marked in error, just remove the CLOSEME comment in the whiteboard within the next month.

Comment 2

[Tyler Downer [He/Him]]

No reply, INCOMPLETE. Please retest with Firefox 3.6.12 or later and a new profile (http://support.mozilla.com/kb/Managing+profiles). If you continue to see this issue with the newest firefox and a new profile, then please comment on this bug.

Comment 3

[[e-mail]]

This bug exists and I have fix for it. Could we please reopen this bug and push my patch? 

diff -r f14dcd1c8c0b widget/gtk/nsFilePicker.cpp
--- a/widget/gtk/nsFilePicker.cpp	Fri Dec 12 17:18:42 2014 -0800
+++ b/widget/gtk/nsFilePicker.cpp	Sun Dec 14 14:36:05 2014 +0100
@@ -4,6 +4,9 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/Types.h"
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
 
 #include <gtk/gtk.h>
 
@@ -70,7 +73,7 @@
 {
   GtkImage *preview_widget = GTK_IMAGE(preview_widget_voidptr);
   char *image_filename = gtk_file_chooser_get_preview_filename(file_chooser);
-
+  struct stat st_buf;
   if (!image_filename) {
     gtk_file_chooser_set_preview_widget_active(file_chooser, FALSE);
     return;
@@ -78,6 +81,21 @@
 
   gint preview_width = 0;
   gint preview_height = 0;
+  /* check type of file */
+  if (stat(image_filename, &st_buf)) {
+    g_free(image_filename);
+    gtk_file_chooser_set_preview_widget_active(file_chooser, FALSE);
+    return; /*stat failed */
+  }
+  /* if file is named pipe, Open is blocking which may lead to UI
+   *  nonresponsiveness; if file is directory/socket, it also isn't 
+   *  likely to get preview */
+  if (!S_ISREG(st_buf.st_mode)) {
+    g_free(image_filename);
+    gtk_file_chooser_set_preview_widget_active(file_chooser, FALSE);
+    return;
+  }
+  
   GdkPixbufFormat *preview_format = gdk_pixbuf_get_file_info(image_filename,
                                                              &preview_width,
                                                              &preview_height);

Comment 4

[[e-mail]]

Attachment: Patch solving this problem

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 8536247 [details] [diff] [review]
Patch solving this problem

+  /* check type of file */
+  if (stat(image_filename, &st_buf)) {
+    g_free(image_filename);
+    gtk_file_chooser_set_preview_widget_active(file_chooser, FALSE);
+    return; /*stat failed */
+  }
+  /* if file is named pipe, Open is blocking which may lead to UI
+   *  nonresponsiveness; if file is directory/socket, it also isn't 
+   *  likely to get preview */
+  if (!S_ISREG(st_buf.st_mode)) {
+    g_free(image_filename);
+    gtk_file_chooser_set_preview_widget_active(file_chooser, FALSE);
+    return;
+  }

Thanks for the fix! My only comment is that it'd be a bit less code to maintain if we were to combine the two if checks since the code inside the checks is identical.

Thanks again, r=jst with that, but also requesting review from the gtk widget owner (which I am not).

Comment 6

[Johnny Stenback (:jst)]

Since you wrote the patch, you get to own the bug :) (and let me know if you need any help!). Once you get review from roc or karlt (who happens to be on vacation until Christmas), please set the checkin flag to ? :jst and we'll make sure to get this landed!

Comment 7

[Johnny Stenback (:jst)]

(roc should be around before Christmas, in a couple of days IIRC, karlt is the one who's on vacation until Christmas)

Comment 8

[[e-mail]]

Attachment: hang.pch

shorter version of patch. as per jst's suggestion I've combined two checks into one.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8537391 [details] [diff] [review]
hang.pch

Review of attachment 8537391 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk/nsFilePicker.cpp
@@ +90,5 @@
> +    g_free(image_filename);
> +    gtk_file_chooser_set_preview_widget_active(file_chooser, FALSE);
> +    return; /* stat failed or file is not regular */ 
> +  }
> +  

remove trailing whitespace

Comment 10

[[e-mail]]

Thank you sir for review. used emacs' whitespace-mode to make sure, no trailing whitespaces are left. Attaching corrected version of patch.

Comment 11

[[e-mail]]

Attachment: hang.pch2

Comment 12

[[e-mail]]

How do we exactly commit? I have ofc. ran test cases and  I have added checkin needed flag.

What else do I have to do?

Thanks again jst,roc for being so helpful.

Comment 13

[Carsten Book [:Tomcat]]

can we get a try run for this change ? Thanks !

Comment 14

[[e-mail]]

yes, why not?

Comment 15

[Johnny Stenback (:jst)]

https://tbpl.mozilla.org/?tree=Try&rev=6514ef5a9700

Comment 16

[Johnny Stenback (:jst)]

Builds on try, but I realized that I didn't enable any tests :( However, this change is in gtk widget code which I seriously doubt we have a single test for, cause it's effectively untestable in our harnesses.

Given that, I decided to land this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6c7aa17862f

Thanks again [e-mail]!

Comment 17

[Carsten Book [:Tomcat]]

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