232587 - Trying to overwrite a file that does not contain a dot in its filename causes a hang

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[PikeUK]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040129 Firebird/0.8.0+

The increment algorithm in the validateLeafName() function in nsHelperAppDlg.js
(and the equivalent code in contentAreaUtils.js) does not work if the filename
does not contain a dot. This bug causes Firebird to hang if you save an
extension-less file to a folder that already contains a file of the same name.

http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in#254

Any file which does not already have a number appended will fail this test:

  var parts = /.+-(\d+)(\..*)?$/.exec(aLocalFile.leafName);
  if (parts) { ...

And will fallback to the else case, unfortunately without a dot in the filename
the replace will fail:

  aLocalFile.leafName.replace(/\./, "-1$&");

This means the filename will never increment and thus the condition in the while
can never be met and so FB goes into an infinite loop.

Reproducible: Always
Steps to Reproduce:
1. Make "Save all files to this folder" is selected in Options->Downloads
2. Visit the URL - http://www.pikey.me.uk/mozilla/test/dlbug.html
3. Right click the link and press Save Link to Disk
4. Repeat step 3

Actual Results:  
Firebird hangs

Expected Results:  
File should have been downloaded as file-1 in download folder

Originally reported in Bug 231371 Comment 8.

I have no idea if this counts as critical since it's so rare and I'm not sure if
this testcase will still work if I just attach both files to this bug (a html
file with a link to an extension-less file)?

Comment 1

[PikeUK]

For what it's worth the following changes seem to fix this bug for me.

In the relevant while loop in contentAreaUtils.js and nsHelperAppDlg.js change
the first replace to:

file.leafName = file.leafName.replace(/((\d+)\.)|((\d+)$)/,
    function (str, dot, dotNum, noDot, noDotNum, pos, s) {
        return (parseInt(str) + 1) + (dot ? "." : "");
    });

And the second to:

file.leafName = file.leafName.replace(/\.|$/, "-1$&");

Comment 2

[Mike Connor [:mconnor]]

Pike, does the suggest fix cause any side effects that we should be aware of? 
If not, can you attach a patch for review?

Comment 3

[Mike Connor [:mconnor]]

we can and should fix this for 0.9, especially with a proto-fix already discussed.

Comment 4

[PikeUK]

(In reply to comment #2)
> Pike, does the suggest fix cause any side effects that we should be aware of? 
> If not, can you attach a patch for review?

Not sure, didn't notice any problems but I didn't do much testing, I'll try
testing it a bit more later this week.

Also after I added comment 1 I noticed there is a createUnique method in nsIFile:

http://lxr.mozilla.org/mozilla/source/xpcom/io/nsIFile.idl#273

It seems to work when replacing the current code with a call to that function
but I'm not sure if it would be appropriate in this situation (for a start it
needs an argument containing the file permissions, I didn't get around to
finding out how FF determines those).

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: pike's suggested change in patch form

seems to work for me.

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

patch r=me. 

checked in branch and trunk.