138832 - Publishing from composer loses parts of file names

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Carl Kumaradas]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0rc1) Gecko/00200203
BuildID:    20020419

I add images with names line 'l.a50.gif' or 's.j50.gif'.  When 
publishing the page, the names are changed to 'l.gif' and 's.gif'
in the HTML code and on the remote destination directory.

Reproducible: Always
Steps to Reproduce:
1. Open composer
2. Create any web page with images having a name with more than one period.
3. Publish the page and look at the image file names on the published directory.

Comment 1

[Kai Lahmann (is there, where MNG is)]

at least doesn't happen locally

Comment 2

[sujay]

--> brade

Comment 3

[TucsonTester1]

I'm also seeing this on Win 98 with build 2002042106.  Composer is truncating
image name at first (.).  This appears in the publishing page dialog while
attempting to upload page.  both the original image filename as well as the
truncated name show as being published. The truncated name will publish, as will
the page itself, but the original name "hangs" with the spinning icon.  

Comment 4

[michael wendell]

I am seeing the same thing on Win 2k using the 04-21 1.0.0 branch build.

Marking NEW.

Comment 5

[Charles Manske]

Composer isn't truncating, nsIWebBrowserPersist is.

*** This bug has been marked as a duplicate of 134890 ***

Comment 6

[sujay]

verified.

Comment 7

[Charles Manske]

Adam Lock wants to handle this separately

Comment 8

[Charles Manske]

Updating owner and status etc. 

Comment 9

[Adam Lock]

Perhaps the simplest fix for this is to add a
PERSIST_FLAGS_DONT_CHANGE_FILENAMES flag to the persist object and if set skip
all name mangling & appending extensions. Then l.a50.gif or whatever is
encountered, it will be uploaded exactly as written.

Is this a reasonable approach?

Comment 10

[Charles Manske]

That sounds good to me!

Comment 11

[Adam Lock]

Attachment: Work in progress

Patch demonstrates how I intend to fix this. I haven't had a chance to test it
yet though!

Comment 12

[Kathleen :Brade]

I don't know that I agree with the "dataloss" keyword.  How is renaming a file
dataloss?

Comment 13

[Kathleen :Brade]

Do we really need a flag?  Is there some reason we can't have a "." inside a
filename (in addition to the extension)?  
I think if we removed the check for '.' we'd be fine.
In fact, I'm not sure we need to check for #, &, ?, or the others since
"GetFileName" isn't including the query or ref portion of url.

Comment 14

[Juan José Mata]

Setting target milestone to 'Future' since 'mozilla1.0' came and went.

Comment 15

[Samir Gehani]

Composer triage team: Adam, are you planning on landing this soon?

Comment 16

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 17

[Adam Lock]

Attachment: Updated work in progress

Comment 18

[Kathleen :Brade]

Comment on attachment 112162 [details] [diff] [review]
Updated work in progress

r=brade

Comment 19

[jag (Peter Annema)]

Comment on attachment 112162 [details] [diff] [review]
Updated work in progress

>Index: mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp,v
>retrieving revision 1.68
>diff -u -r1.68 nsWebBrowserPersist.cpp
>--- mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp	18 Jan 2003 02:13:36 -0000	1.68
>+++ mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp	21 Jan 2003 17:26:02 -0000
>@@ -1812,6 +1812,10 @@

      nsCOMPtr<nsIURL> url(do_QueryInterface(aURI));
      if (url)
>     {
>         nsCAutoString nameFromURL;
>         url->GetFileName(nameFromURL);
>+        if (mPersistFlags & PERSIST_FLAGS_DONT_CHANGE_FILENAMES)
>+        {
>+            goto end;
>+        }

This doesn't do what you want it to. |fileName| is still empty at this point,
so |aFilename| will also be empty.

Instead of |goto end;| you could do:

  url->GetFileName(nameFromURL);
  if (mPersistFlags & PERSIST_FLAGS_DONT_CHANGE_FILENAMES)
  {
    aFilename = nameFromURL;
    return NS_OK;
  }

Btw, the logic in this snippet is contradicting (and seems to be the cause of
comment 0):

1822		 for (;*p && *p != ';' && *p != '?' && *p != '#' && *p != '.'
1823		      ;p++)
1824		 {
1825		     if (nsCRT::IsAsciiAlpha(*p) || nsCRT::IsAsciiDigit(*p)
1826			 || *p == '.' || *p == '-' ||  *p == '_' || (*p == '
'))

The first check filters out '.', the second says "append if it's '.'".

Comment 20

[Juan José Mata]

Looks like we're getting some traction here again.  Adam, do you think a 1.4a
target might be possible here?

Comment 21

[Adam Lock]

I'll target 1.4a and submit a new patch

Comment 22

[Adam Lock]

Attachment: Patch

Patch fixes the raised issue.

One thing to say about the dot truncation stuff is that it is necessary in
order to get the file to load properly from a local file. For example, a
subframe might be called foo.cgi which loads fine from the server because the
server feeds the proper text/html mime type, however when loaded locally the
.cgi extension is meaningless and Mozilla won't know what to do with it.

In order to ensure the file is loaded properly from local store, the persist
object strips the original extension off and then appends the proper local
extension based upon the mime type supplied by the server. 

In the case of double dots, it might seem that it should just chop the last
extension and preserve the others, however it would still potentially change
the name, for example lopping off a .jpeg and replacing it with .jpg or
whatever, not to mention possibly changing filenames in other ways such as
truncating them below 31 chars. I am also wary that this could lead to
exploits, for example if a page contained a linked file called foo.exe.txt and
we lop off the .txt.

For composer upload none of behaviour is desirable, so the flag can be supplied
to prevent the persist object from interfering with the names in any way.

Comment 23

[Adam Lock]

Attachment: Patch

This is the real patch this time, notes still apply

Comment 24

[Adam Lock]

Attachment: Patch for editor

Patch for the editor upload

Comment 25

[Kathleen :Brade]

Comment on attachment 118064 [details] [diff] [review]
Patch for editor

r=brade

Comment 26

[jag (Peter Annema)]

Comment on attachment 118064 [details] [diff] [review]
Patch for editor

sr=jag

Comment 27

[jag (Peter Annema)]

Comment on attachment 118057 [details] [diff] [review]
Patch

sr=jag

Comment 28

[Adam Lock]

Thanks all, fix checked in

Comment 29

[Paul Wyskoczka]

Updated qa contact to [e-mail]

Comment 30

[Chris Petersen]

Verified on the 2003-06-09-08 win32 and Macho trunks builds.