135527 - Previously uploaded images can cause publish to wrongly cancel

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[TucsonTester1]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.9+)
Gecko/20020404 Netscape6/6.2.1+
BuildID:    2002040403

Images that were previously uploaded to an ftp site in the main directory will
not upload to a sub-directory if more images are added.  The HTML, however, is
changed to tell the browser to locate the previous image in the sub-directory.

Reproducible: Always
Steps to Reproduce:
1.Install/Launch NS 6.2.1+.
2.Launch a new/blank Composer window.
3.Enter some content including an image.
4.Publish the page to an ftp site, ensuring images are uploaded to same location
as page.
5.Navigate to the page to verify the page uploaded properly.
6.Reopen the page in Composer(ctrl-e).
7.Add another image to the page.
8.Publish the page using file|publish as..., but select "use this site
sub-directory" for the images.
9.Select a pre-existing sub-directory on the ftp site.
10.Navigate to the page.

Actual Results:  The first image will upload to the main directory.  When the
second image is added and the page is uploaded to a sub-directory, the second
image uploads to the sub-directory, but the first image remains in the main
directory.  However, the HTML is changed so that the browser is told to locate
the image in the sub-directory. (i.e. HTML of first image changes from
"src=mypicture.jpg" to "src=images/mypicture.jpg" where images/ is the
sub-directory.)  

Expected Results:  Either the first image remains in the main directory and the
html reflects this or the first image is uploaded to the sub-directory. The
latter seemed to be the case in previous builds.

Comment 1

[Charles Manske]

Sounds like the url-adjusting code in nsIWebBrowserPersist needs to be smarter.
This is tricky to know what is right. If user "republishes" (i.e., user is
editing the remote url) and then changes the "other" directory, do they intend
to move all the files from current dir to the new one? They would be copied, not
moved by current code. So I'd suggest testing if image urls are already relative
to a remote document, we shouldn't include them in the list of files to copy
and also do not change the image url.

Comment 2

[Kathleen :Brade]

I believe this is a regression.
DSLMichael or Sujay--can one of you try this but make sure that the image
doesn't already exist in the images directory?  My belief is that one or both
images may already exist (same name at least) in the sub directory and the
progress dialog doesn't handle that situation correctly (since we don't upload
it if it's already there).

Comment 3

[TucsonTester1]

works for me.
seems to be fixed in this new build 2002040803.

Comment 4

[Kathleen :Brade]

I think this bug is being difficult to pin down because of the number of
variables that are in play (number and size of images, src location of image,
order of upload, network traffice, size of html, etc.).  

Looking at the code, I see where publishing images to the location where we
already are doesn't do any uploading (since the image src location is the
destination location) so a binding aborted error is being sent to the listener
and the publish listener thinks this is a hard error and cancels the publishing
of all files.  If the rest of the files have finished, you won't notice any
problems.  However, if some of the files were still uploading during the
cancellation, then detection of a problem might show up as partial files or
files entirely missing.

Proposed patch coming...

Comment 5

[Kathleen :Brade]

Attachment: patch (v1)

Comment 6

[Akkana Peck]

Comment on attachment 78547 [details] [diff] [review]
patch (v1)

r=akkana (actually same patch as in bug 135771, same comment about dump()
applies).

Comment 7

[kinmoz]

Comment on attachment 78547 [details] [diff] [review]
patch (v1)

sr=[e-mail]

Comment 8

[Kathleen :Brade]

users are having their publishing transaction canceled on them because we aren't
handling this case correctly; nominate this for adt branch approval

Comment 9

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin into 1.0. Pls check this in to
the trunk and 1.0 branch today.

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 78547 [details] [diff] [review]
patch (v1)

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 11

[Kathleen :Brade]

fix checked into trunk and 1.0 branch
Please verify in both locations.

Comment 12

[TucsonTester1]

verified on 4-13 1.0.0 and 4-15 trunk.

Comment 13

[michael wendell]

Adding verified1.0.0 keyword.