Closed Bug 660586 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla7
Tracking Status
firefox5 --- affected
firefox6 --- affected

People

(Reporter: shughes, Assigned: mounir)

References

()

Details

(Keywords: hang)

Attachments

(1 file, 1 obsolete file)

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.
Component: File Handling → DOM: Core & HTML
QA Contact: file-handling → general
I'm not seeing any such behaviour on Fedora 12 nightlies.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Keywords: hang
By the way, thank you very much for this report Scott ;)
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>.
Component: DOM: Core & HTML → Widget: Cocoa
QA Contact: general → cocoa
Hardware: x86 → All
I have a fix. I'm looking at something and will attach here for review.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #536262 - Flags: review?
Attachment #536262 - Flags: review? → review?(joshmoz)
Whiteboard: [needs review]
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.
Attachment #536262 - Flags: review?(joshmoz) → review-
Attached patch Patch v2Splinter Review
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 ;)
Attachment #536262 - Attachment is obsolete: true
Attachment #536303 - Flags: review?(joshmoz)
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.
Attachment #536303 - Flags: review?(joshmoz) → review+
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.
Flags: in-testsuite-
Flags: in-litmus?
Whiteboard: [needs review]
Whiteboard: [fixed in cedar]
Pushed in m-c:
http://hg.mozilla.org/mozilla-central/rev/b9924776a728
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → mozilla7
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.
Attachment #536303 - Flags: approval-mozilla-aurora?
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/
Depends on: 662164
You're welcome. Thanks for such a fast response. :)

I've verified this is fixed on the 2011-06-05 nightly.
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.
Attachment #536303 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.