660586 - Selecting a large number of files in <input type="file"> hangs browser

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Scott Hughes]

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]]

I'm not seeing any such behaviour on Fedora 12 nightlies.

Comment 2

[Mounir Lamouri (:mounir)]

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)]

By the way, thank you very much for this report Scott ;)

Comment 4

[Mounir Lamouri (:mounir)]

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)]

I have a fix. I'm looking at something and will attach here for review.

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 7

[Josh Aas]

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)]

Attachment: 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]

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)]

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)]

Pushed in m-c:
http://hg.mozilla.org/mozilla-central/rev/b9924776a728

Comment 12

[Mounir Lamouri (:mounir)]

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)]

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]

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]]

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]]

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.