103468 - Mozilla creates .url files with zero byte at end, confusing IE (Internet shortcut)

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P1)

Description

[Olivier Mengué]

See also bug #103466 for related information/bug about Internet Shortcuts.

Mozilla always put an \0 as the last byte of Internet Shortcuts.

Microsoft Internet Explorer 6 doesn't seem to be able to load (double-click in
Windows Explorer) the shortcut when last byte of the file is \0 AND the file
length is a multiple of 2.
Of course, the file size depend on the length of the URL bookmarked.

It is probably a Windows UNICODE-related problem as the "type" command or the
notepad also have problems with such files.

I'm attaching below two Internet Shortcuts :
- one that works with IE6/type/notepad (Mozilla 0.9.4 Release Notes (2).URL)
- one that doesn't work (Mozilla 0.9.4 Release Notes.URL)

Comment 1

[Olivier Mengué]

Attachment: A Mozilla-generated Internet Shorcut that works

Comment 2

[Olivier Mengué]

Attachment: A Mozilla-generated Internet Shorcut that doesn't work

Comment 3

[Olivier Mengué]

I forgot to say that I verified that removing the last byte of the file (the
'\0') in the Internet Shortcut solved the problem (IE/notepad/type can load the
shortcut).

Comment 4

[Jesse Ruderman]

-> bookmarks

Comment 5

[Ashley Bischoff (blog at handcoding.com)]

No dupes found. Marking NEW.

-> XP Apps: Drag and Drop

Reporter: In future, please use the Bugzilla Helper for reporting bugs:
http://www.mozilla.org/quality/help/bugzilla-helper.html
(among other things, it automagically adds your Build ID)

Comment 6

[Olivier Mengué]

Attachment: An Internet Shortcut created with Mozilla 0.9.6

Comment 7

[Olivier Mengué]

Verified in 0.9.6 (build 2001112009) that the bug (the '\0' byte) is still there.
Added as an attachment an Internet shortcut created with Mozilla 0.9.6.

However due to the patch for bug #103466 that added a carriage return at the end
of the URL, the bug is now not blocking with IE.

But please, remove this little byte!

Comment 8

[Olivier Mengué]

Still in Mozilla 0.9.7.

Comment 9

[Blake Ross]

Attachment: patch that removes the \0

Comment 10

[Blake Ross]

ok, so there's a patch, but this code was obviously very deliberate.  cc'ing
pinkerton for explanation...?

Comment 11

[Mike Pinkerton (not reading bugmail)]

uh, the code that the patch removes does not introduce the null. what it does is
truncate the string so that it's length is <= MAX_PATH. Removing that code would
be bad. 

the right fix would be to trim the null off the result of the sprintf (which is
where the null actually gets introduced). I'm not sure how removing those lines
fixes the problem.

Comment 12

[Olivier Mengué]

The bug is still in Mozilla 1.0RC1 (build 2002041711).

Comment 13

[Terri Preston]

qa contact -> pmac

Comment 14

[Andrew Hagen]

There have been recent check-ins in this area.

Worksforme Build ID 2002120708 pre-1.3 alpha, Windows 98.

Reporter, please download and install the latest-trunk build of Mozilla. It
should now work.

Comment 15

[Sebastian Lisken]

The bug still exists in Mozilla 1.6a under Windows 2000. Why?

Comment 16

[Brodie]

Attachment: patch to fix the null byte

Problem was that:
1. PR_snprintf adds a null to the end of the formatted string
2. need to allocate a buffer big enough for the null byte that snprintf adds
3. the entire contents of the buffer are written to disk

Fix is: 
1. only allocate a buffer big enough for the data and no null
2. use memcpy to format the string (easy since it is just a prefix and suffix)

Comment 17

[Brodie]

Attachment: patch #2

Another patch to fix this bug and more. Bugs addressed with this patch are all
wrapped up in one:
* this bug
* problems where MBCS

Comment 18

[Brodie]

Attachment: testcases

Accidently click 'submit' before I was ready.

Fixes are:
* zero byte removed from shortcut file
* code simplified and memory allocations removed
* long titles when converted to MBCS will not be cut in the middle of a
multi-byte character but only at character borders
* the test for an invalid filename includes all of the characters in MSDN
listed as invalid, as well as a test for invalid filenames like "nul"
* all possible error conditions (I think) are handled gracefully

This file is a number of testcases that I created. Contents are:
1. test for invalid characters in name
2. test for invalid filename
3. test for simple MBCS filename (includes backslash as trail-byte)
4. test for very long MBCS filename which exercises the "title too long" case

Comment 19

[Brodie]

Attachment: final patch

I found another couple of possible issues:
* now using the localized string for "untitled page" when a title can't be
generated
* handles a page with no title (fixes bug 104933)
* fixes the problem case of a filename starting with a forbidden string (e.g.
"nul.txt", which in the process of testing IE behaviour ended up with a file on
the desktop called "nul.txt" which I can't delete).
* split the code into separate functions to make it easier to read

I'm definitely finished hacking on this now. 
It'd be nice to get a code review.

Comment 20

[Brodie]

Attachment: updated testcases

Comment 21

[Brodie]

Attachment: patch 4

compress whitespace within the title and trim the ends.
fix a GlobalFree without GlobalUnlock (not strictly necessary but cleaner)

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  static struct { char* name; size_t nameLen; } forbiddenNames[] = {
+    { "CON",  0 }, { "PRN",  0 }, { "AUX",  0 }, { "NUL",  0 }, { "CLOCK$", 0 }, 
+    { "COM1", 0 }, { "COM2", 0 }, { "COM3", 0 }, { "COM4", 0 }, { "COM5", 0 }, 
+    { "COM6", 0 }, { "COM7", 0 }, { "COM8", 0 }, { "COM9", 0 }, { "LPT1", 0 }, 
+    { "LPT2", 0 }, { "LPT3", 0 }, { "LPT4", 0 }, { "LPT5", 0 }, { "LPT6", 0 }, 
+    { "LPT7", 0 }, { "LPT8", 0 }, { "LPT9", 0 }
+  };

I think you should not store the length. This is not performance critical code,
so we should do the simplest thing, which is to just use strlen to get the
length every time.

Other than that, looks fantastic. If you respin a patch I'll give it sr+. If ere
 is not responding (I haven't seen him in IRC, so he may be incommunicado) I'll
stretch out and give this r+ myself.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 149178 [details] [diff] [review]
patch 4

minusing in anticipation of a new patch

Comment 24

[Brodie]

Attachment: patch 5

Patch with requested changes. Also using existing definition from nsCRT.h of
illegal characters.

Comment 25

[Ere Maijala (slow)]

Comment on attachment 149548 [details] [diff] [review]
patch 5

r=ere

Comment 26

[Brodie]

I don't have CVS write permissions, so would one of you please check this in 
for me? This will fix both this bug and bug 104933.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Taking for checkin

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in. Thank you very much!!

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in. Thank you very much!!

Comment 30

[Walter K.]

+      strcpy(fileGroupDesc->fgd[0].cFileName, "Untitled.URL"); // fallback
Should this be localizable?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It is. This value is only used if we failed to get the localized string.

Comment 32

[:deanis74]

Did this get rid of the fix that was originally checked in for bug 78509?  I
can't see that old code in the patch anywhere.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If I understand correctly, this patch just removes all colons from the title
before choosing the file name.

Comment 34

[Brodie]

This patch supercedes the patch in bug 78509. Look at the function
MangleTextToValidFilename() for full behaviour. It gets rid of all of the
invalid characters as listed in that bug (FILE_PATH_SEPARATOR 
FILE_ILLEGAL_CHARACTERS) and checks for invalid filenames (nul, lpt1.txt, etc). 

The only thing it doesn't do is strip ascii chars < 32. If these chars should
also be removed then create a new bug and I'll supply a patch.

Note that this resolves bug 104933 and bug 78509 too.

Comment 35

[Brodie]

Attachment: remove control chars < 32

If required, this patch is an addition to the existing checked in one and will
remove all control characters < 32 from the title. I'm not sure that we need
this however, as at least on Windows XP, the OS appears to do this for us.

Comment 36

[Brodie]

Attachment: testcases

Updated testcases including one for for control characters. test8.html includes
all control chars < 32 in the title. When viewing, ensure that you manually set
the char encoding back to Windows-1252 for correct display. When this file is
dragged from the browser to the desktop on Windows XP, both IE and Moz (1.7RC2)
will correctly create a file using the title with all control chars stripped,
or changed to spaces (for \t\r\n). It seems that the OS is handling this. I
don't know if non-XP versions also do the same processing.

Comment 37

[:deanis74]

Thanks Brodie.  I searched through the patch repeatedly and couldn't find the
lines that removed the invalid characters.  I'm happy now.

Comment 38

[Hidehiro Kozawa]

*** Bug 159773 has been marked as a duplicate of this bug. ***

Comment 39

[Hidehiro Kozawa]

Shift-Jis(Japanese Charactor) contains 0x5c.
Patch of this bug also fix Bug 159773.
If 1.7 becomes stable branck, I want to check in this patch 1.7 branch too.

Comment 40

[Brodie]

Too late for 1.7, it's already released. 
Perhaps 1.7.1?

Comment 41

[Hidehiro Kozawa]

yes 1.7.1 if that will be release.

Comment 42

[Hiro]

Is this patch reflected in AVIARY_1_0_20040515_BRANCH?

Comment 43

[Brodie]

No. Firefox is bug 247227.

Comment 44

[Brodie]

Nominating for fixing in the 1.7 branch for 1.7.1
patch is "patch 5", attachment 149548 [details] [diff] [review]

Comment 45

[OstGote!]

Any chance that this will be going into 1.7.5? Is there a reason now for
approval1.7.5- for the patch?

Comment 46

[Brodie]

The comment I received from Roc is that this won't be accepted for 1.7.5. The
only branch in which this fix exists is the trunk, so at the moment only Mozilla
1.8a1+ have this fixed. When Firefox 1.1 is released, because it is based on the
trunk it will also get this fix.