Publishing from composer loses parts of file names

VERIFIED FIXED in mozilla1.4alpha

Status

SeaMonkey
Composer
--
critical
VERIFIED FIXED
16 years ago
14 years ago

People

(Reporter: Carl Kumaradas, Assigned: Adam Lock)

Tracking

({dataloss})

Trunk
mozilla1.4alpha
dataloss

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2]publish)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
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.
at least doesn't happen locally

Comment 2

16 years ago
--> brade
Assignee: syd → brade

Comment 3

16 years ago
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

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

Marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 5

16 years ago
Composer isn't truncating, nsIWebBrowserPersist is.

*** This bug has been marked as a duplicate of 134890 ***
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → DUPLICATE

Comment 6

16 years ago
verified.
Status: RESOLVED → VERIFIED

Comment 7

16 years ago
Adam Lock wants to handle this separately
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---

Comment 8

16 years ago
Updating owner and status etc. 
Assignee: brade → adamlock
Status: REOPENED → NEW
Keywords: dataloss, nsbeta1
Whiteboard: [adt2]publish
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 9

16 years ago
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

16 years ago
That sounds good to me!
(Assignee)

Comment 11

16 years ago
Created attachment 81178 [details] [diff] [review]
Work in progress

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

Comment 12

16 years ago
I don't know that I agree with the "dataloss" keyword.  How is renaming a file
dataloss?
OS: Linux → All
Hardware: PC → All

Comment 13

16 years ago
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.

Updated

16 years ago
Keywords: nsbeta1 → nsbeta1+

Comment 14

16 years ago
Setting target milestone to 'Future' since 'mozilla1.0' came and went.
Target Milestone: mozilla1.0 → Future

Updated

16 years ago
Target Milestone: Future → ---

Comment 15

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

Comment 16

16 years ago
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.
Severity: normal → critical
(Assignee)

Comment 17

16 years ago
Created attachment 112162 [details] [diff] [review]
Updated work in progress
Attachment #81178 - Attachment is obsolete: true

Comment 18

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

r=brade
Attachment #112162 - Flags: review+

Updated

15 years ago
Attachment #112162 - Flags: superreview?(jaggernaut)

Comment 19

15 years ago
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 '.'".
Attachment #112162 - Flags: superreview?(jaggernaut) → superreview-

Comment 20

15 years ago
Looks like we're getting some traction here again.  Adam, do you think a 1.4a
target might be possible here?
(Assignee)

Comment 21

15 years ago
I'll target 1.4a and submit a new patch
Target Milestone: --- → mozilla1.4alpha
(Assignee)

Comment 22

15 years ago
Created attachment 117875 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #112162 - Attachment is obsolete: true
(Assignee)

Comment 23

15 years ago
Created attachment 118057 [details] [diff] [review]
Patch

This is the real patch this time, notes still apply
Attachment #117875 - Attachment is obsolete: true

Updated

15 years ago
Attachment #118057 - Flags: superreview?(jaggernaut)
Attachment #118057 - Flags: review+
(Assignee)

Comment 24

15 years ago
Created attachment 118064 [details] [diff] [review]
Patch for editor

Patch for the editor upload

Comment 25

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

r=brade
Attachment #118064 - Flags: superreview?(jaggernaut)
Attachment #118064 - Flags: review+

Comment 26

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

sr=jag
Attachment #118064 - Flags: superreview?(jaggernaut) → superreview+

Comment 27

15 years ago
Comment on attachment 118057 [details] [diff] [review]
Patch

sr=jag
Attachment #118057 - Flags: superreview?(jaggernaut) → superreview+
(Assignee)

Comment 28

15 years ago
Thanks all, fix checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago15 years ago
Resolution: --- → FIXED

Comment 29

15 years ago
Updated qa contact to petersen@netscape.com
QA Contact: sujay → petersen

Comment 30

15 years ago
Verified on the 2003-06-09-08 win32 and Macho trunks builds.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.