Last Comment Bug 660586 - Selecting a large number of files in <input type="file"> hangs browser
: Selecting a large number of files in <input type="file"> hangs browser
Status: VERIFIED FIXED
: hang
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla7
Assigned To: Mounir Lamouri (:mounir)
:
: Markus Stange [:mstange]
Mentors:
data:text/html,<!DOCTYPE html><input ...
Depends on: 662164
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-29 22:04 PDT by Scott Hughes
Modified: 2011-09-01 06:55 PDT (History)
3 users (show)
mounir: in‑testsuite-
mounir: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected


Attachments
Patch v1 (1.75 KB, patch)
2011-05-31 03:14 PDT, Mounir Lamouri (:mounir)
jaas: review-
Details | Diff | Splinter Review
Patch v2 (1.84 KB, patch)
2011-05-31 08:01 PDT, Mounir Lamouri (:mounir)
jaas: review+
asa: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description Scott Hughes 2011-05-29 22:04:54 PDT
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_7) AppleWebKit/534.24 (KHTML, like Gecko) Chrome/11.0.696.71 Safari/534.24
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Selecting ~1000 files in a multiple file input causes Firefox Mac to hang temporarily. After ~1 minute, the UI becomes responsive again.

Reproducible: Always

Steps to Reproduce:
1. Load the test case URL.
2. Click the "Browse" button on the file input.
3. Select a large number (e.g. 1000) of files in the file dialog.
4. Click the file dialog's "Open" button.

Actual Results:  
Firefox hangs for a period of time proportional to the number of files selected.

Expected Results:  
Firefox should not hang. Chrome and Safari do not hang with the above steps.

This appears to only affect Firefox Mac. Windows and Ubuntu do not hang with the above steps.
Comment 1 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-05-29 23:10:28 PDT
I'm not seeing any such behaviour on Fedora 12 nightlies.
Comment 2 Mounir Lamouri (:mounir) 2011-05-30 06:41:51 PDT
I've been able to reproduce this on MacOS X 10.6 with a Nightly.

I will try with my Gentoo and check were we are spending our CPU cycles.
Comment 3 Mounir Lamouri (:mounir) 2011-05-30 06:42:24 PDT
By the way, thank you very much for this report Scott ;)
Comment 4 Mounir Lamouri (:mounir) 2011-05-30 07:20:32 PDT
This hang is MacOS X specific and appears here:
https://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsFilePicker.mm#403

On GTK, I got a very quick hang but I believe it happens when copying the file names in the <input type=file>.
Comment 5 Mounir Lamouri (:mounir) 2011-05-30 08:07:13 PDT
I have a fix. I'm looking at something and will attach here for review.
Comment 6 Mounir Lamouri (:mounir) 2011-05-31 03:14:10 PDT
Created attachment 536262 [details] [diff] [review]
Patch v1
Comment 7 Josh Aas 2011-05-31 07:47:51 PDT
Comment on attachment 536262 [details] [diff] [review]
Patch v1

>+  for (NSURL* url in [thePanel URLs]) {

We don't use Obj-C 2.0 anywhere yet, which means no "in" loop form. Please just write this the old way. We'll have to look into whether we want to allow 2.0 features at some point.
Comment 8 Mounir Lamouri (:mounir) 2011-05-31 08:01:25 PDT
Created attachment 536303 [details] [diff] [review]
Patch v2

That's a shame because Fast Enumeration seems nice. I didn't knew it was in Obj-C 2.0. Though, I didn't knew there was a 2.0 version of Obj-C ;)
Comment 9 Josh Aas 2011-06-02 14:28:37 PDT
Comment on attachment 536303 [details] [diff] [review]
Patch v2

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

::: widget/src/cocoa/nsFilePicker.mm
@@ +404,5 @@
>    
>    if (result == NSFileHandlingPanelCancelButton)
>      return retVal;
> +
> +  // Converts data from a NSArray af NSURL to the returned format.

Spelling.

@@ +406,5 @@
>      return retVal;
> +
> +  // Converts data from a NSArray af NSURL to the returned format.
> +  // We should be careful to not create the array for each iteration in the loop to prevent
> +  // hangs when a lot of files are selected.

This comment is misleading because it isn't obvious what array creation you're talking about. I assume you mean to explain that getting URLs from the panel causes it to create a new array - which is an implementation detail if it's true, not something you'd be able to know from reading the code.
Comment 10 Mounir Lamouri (:mounir) 2011-06-02 15:32:51 PDT
I'm going to push that with this comment instead:
// We should be careful to not call [thePanel URLs] more than once given that it
// creates a new array each time.
Comment 11 Mounir Lamouri (:mounir) 2011-06-03 03:11:44 PDT
Pushed in m-c:
http://hg.mozilla.org/mozilla-central/rev/b9924776a728
Comment 12 Mounir Lamouri (:mounir) 2011-06-03 03:12:45 PDT
Comment on attachment 536303 [details] [diff] [review]
Patch v2

Asking for approval in Aurora/Firefox 6 given that the fix is very simple and safe and might fix hangs.
Comment 13 Mounir Lamouri (:mounir) 2011-06-03 03:14:54 PDT
Scott, thank you very much for filing this bug :) It will be fixed in Firefox 7 (maybe in Firefox 6 depending on the answer to my request). You will be able to test the fix if you use a Nightly build tomorrow, they are available here: https://nightly.mozilla.org/
Comment 14 Scott Hughes 2011-06-05 17:21:31 PDT
You're welcome. Thanks for such a fast response. :)

I've verified this is fixed on the 2011-06-05 nightly.
Comment 15 Asa Dotzler [:asa] 2011-06-09 14:40:57 PDT
Comment on attachment 536303 [details] [diff] [review]
Patch v2

Not the kind of important regression fix or finishing off a feature that would otherwise be backed out so we'll wait on this to reach users in Firefox 7.
Comment 16 Vlad [QA] 2011-09-01 06:55:32 PDT
Setting resolution to Verified Fixed on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0 beta 3

Firefox isn't hanging when a large number of files is loaded.

Note You need to log in before you can comment on or make changes to this bug.