281343 - Cannot save web pages using very long filenames

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Son Le]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041109 Firefox/1.0 (MOOX M1)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041109 Firefox/1.0 (MOOX M1)

Follow on from bug 241864, comment #16

On Windows, the MAX_PATH for a filepath (drive letter + directory name + file
name) cannot be longer than 248 chars. nsWebBrowserPersist (and possibly other
places) fails to check against this limit and fails silently.


Reproducible: Always

Steps to Reproduce:
1. Open any webpage
2. Select Save Page As...
3. Enter the following filename
1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678912345678
4. Click on save

Actual Results:  
Nothing.

Expected Results:  
Page saved or error message indicating save has failed.

Comment 1

[Son Le]

Don't know which area this bug belongs, there was no embedding/components
selection. :)

Anyway, the problem is here:

http://lxr.mozilla.org/mozilla/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#1586

Possible solutions are to truncate the filepath or fail and show an error message.

Comment 2

[Son Le]

Bug 129911 is also related...

Comment 3

[Son Le]

Attachment: proposed patch v0

Display error message if save fails.

Comment 4

[Son Le]

Christian, can you please have a look at this bug and patch for me?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 174127 [details] [diff] [review]
proposed patch v0

you can not hardcode english messages.

Also, I am not sure that an unparented prompt is really a good thing.

Can't you just call SendErrorStatusChange, with an appropriate error code
(possibly a new one)?

Comment 6

[Son Le]

Attachment: proposed patch v1

Use SendErrorStatusChange() instead, which works much better. Had to alter
nsLocalFileWin.cpp which is in xpcom (and added a missing break or two as
well).

Not sure about the wording of the error messages. Does it need to be run by
anyone first? Translators?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 174369 [details] [diff] [review]
proposed patch v1

looks great to me, thanks!
I'm not sure that the text for the AlreadyExistsError is so good, since users
may not even know about a _files directory... I guess it's a rare case, so it
probably won't matter much.

r=biesi. please get review from darin or dougt for the xpcom change.

Comment 8

[Son Le]

Comment on attachment 174369 [details] [diff] [review]
proposed patch v1

dougt, asking for sr for xpcom change.

:bi, originally just had "file already exists" but found it was a bit
misleading as users might go looking for the wrong file. Thought the _files
might give them a hint.

Comment 9

[Son Le]

Attachment: new nsWebBrowserPersist.properties location

updated to match with new nsWebBrowserPersist.properties location

Comment 10

[Son Le]

carried forward cbiesinger review+

Comment 11

[Son Le]

:bi, can you please check this in for me?

thanx in advance.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in for 1.8b2, thanks for the patch!

Checking in dom/locales/en-US/chrome/nsWebBrowserPersist.properties;
/cvsroot/mozilla/dom/locales/en-US/chrome/nsWebBrowserPersist.properties,v  <--
 nsWebBrowserPersist.propertiesnew revision: 1.2; previous revision: 1.1
done
Checking in embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp;
/cvsroot/mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp,v
 <--  nsWebBrowserPersist.cpp
new revision: 1.106; previous revision: 1.105
done
Checking in xpcom/io/nsLocalFileWin.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v  <--  nsLocalFileWin.cpp
new revision: 1.136; previous revision: 1.135
done

Comment 13

[:deanis74]

+fileAlreadyExistsError=%S could not be saved, because a file already exists
with the same name as the '_files' directory.\n\nTry saving to a different location.
+fileNameTooLongError=%S could not be saved, because the file name was too
long.\n\nTry saving with a shorter file name.

nit: Didn't need the commas before "because".