134883 - Image urls changed during publishing must be restored if publishing fails

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Charles Manske]

During publishing, image (and other associated file links) are changed to be 
relative to the new source location. Using Composer publishing, this may include
a subdiretory that's not the same as the documents. But if publishing fails, the
document url is unchanged, but the image urls are still relatie to the new 
destination.

Comment 1

[Charles Manske]

I *really* think this issue is important and must be fixed before RTM!
If we have any kind of error and don't detect it (i.e., we switch the document
to the remote url even though we had a failure), the document is now toast.
And the original image links are all lost.

Comment 2

[Charles Manske]

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

Comment 3

[Charles Manske]

Bug 137641 is a simple example of how we get into trouble by not "undoing" the 
relative image urls after failing to publish:
1. Create a new Composer file and publish the page via FTP so you are now editing 
are remote page. Or simply edit an existing page on a site that uses FTP to 
publish to.
2. Add an image. E.g.: I added one that resulted in the absolute url:
file:///M:/Temp/1.gif
3. Publish the page using Publish As and use a non-existent subdirectory for 
image. E.g.: I used "bad".
Publishing will fail, but if double click on image to view it's properties,
you'll see the URL is now "bad/1.gif" as a result of making the URL relative 
to the page source, even though "bad" doesn't exist. Note that there's no image
in the preview window because the image cannot be accessed. Note also that the 
image still appears to be correct in the page because nothing has triggered 
reloading of page, so user is fooled into thinking image link is correct.

4. Now if you try to publish again, but this time use an existing image 
subdirectory, publishing will fail because it will never be able to find 
"bad/1.gif". It will change the image url each time, though! (E.g., it changed
to "Test/1.gif" after my testing when I used "Test" as the subdirectory to
publish to.)

Thus if we don't restore the original image URLs after a publishing failure,
the document is always at risk of being corrupted with bad image links and 
very likely will never be publishable again.

Comment 4

[Charles Manske]

Attachment: patch v1

This is not the most efficient way to solve this problem, but it is the safest
and smallest code change we can make. We simply save a backup of the HTML
source 
output as a JS string, and use that to rebuild the document when publishing has

failed. This is exactly the same code used to build the HTML Source window in
normal editing, and how it is reconverted into the DOM document.
The performance/memory cost, obviously, is storing a possibley very large 
(but temporary) string for the source HTML.

Comment 5

[Charles Manske]

reassigning to me

Comment 6

[Charles Manske]

Attachment: patch v2

Same as v1, just removed lines that turn on debug flags.

Comment 7

[Charles Manske]

I tested this with a 204kbyte file that had 2 images: one at the start and one
at the end of the page. First I tested publishing successfully. Then I tested
FTP publishing to a non-existent subdirectory and the appropriate error alert
dialogs appeared and the publishing dialog showed "Not all files were published"
Using "Cancel" in Publish Progress dialog canceled the publishing and restored
the image SRC urls correctly.

Comment 8

[Charles Manske]

Attachment: patch v3

Added try/catch per review.
Added optimization to not save backup if not uploading images

Comment 9

[Kathleen :Brade]

Comment on attachment 83575 [details] [diff] [review]
patch v3

assuming this patch doesn't introduce any JS warnings for mail or Composer and
assuming it doesn't have any serious performance issues, r=brade

Comment 10

[Charles Manske]

Attachment: patch v4

Update per kin's review: Clear the transaction cache (undo stack) after 
completely rebuilding the document when we had a publishing error.
This is to avoid eating a lot of memory since all nodes in new doc are cached
whe we rebuild the document.

Comment 11

[Charles Manske]

Testing:
1. Create a new Composer page.
2. Insert an image from a local drive. (It will have an absolute "file:" URL)
3. Use "File | Publish As" command to publish and enter a subdirectory to publish 
to that doesn't exist. Click "Publish"
4. You get the error alert messages such as "550 ... No such file or directory"
because of the bad directory. Dismiss all of those messages and click on "Cancel"
button in Publish Progress dialog.
5. In the Edit menu, the "Undo" command should be disabled since the undo stack
was cleared.
6. Double-click on the image or view page in HTML Source to see that the SRC url
for the image you inserted should still be the absolute "file:" url that you 
picked before.

Comment 12

[Charles Manske]

Comment on attachment 83960 [details] [diff] [review]
patch v4

Please ignore:
+const gShowDebugOutputStateChange = true;
That won't be checked in

Comment 13

[Kathleen :Brade]

Comment on attachment 83960 [details] [diff] [review]
patch v4

please remove the blank spaces on the line after
+	   editor.resetModificationCount();

I wish we had constants in our JS to covert the output flags (256).

r=brade

Comment 14

[Christine Wozniak]

Adding custrtm-; no impact on customization.

Comment 15

[Joe Hewitt (gone)]

Comment on attachment 83960 [details] [diff] [review]
patch v4

sr=hewitt

Comment 16

[Charles Manske]

checked into trunk.
Nominating for checkin for 1.0.1 branch.
This is very important, please look at comments #1 and #3

Comment 17

[scottputterman]

sujay, could you verify this on the trunk?

Comment 18

[Charles Manske]

The best test for verification is clearly defined comment #11.

Comment 19

[Charles Manske]

To test the non-error case:
1. Create a new Composer page.
2. Insert an image from a local drive. (It will have an absolute "file:" URL)
3. Publish to an existing site and select a subdirectory that does exist on the
site. All files should publish successfully.
4. Double-click on the image and it should now show a relative SRC URL,
e.g., "images\myimage.gif", not the absolute "file://..." url you started with.

Comment 20

[Charles Manske]

Note that the error case of a bad directory must be testing using FTP.
HTTP will automatically create the subdirectory and you won't see an error.

Comment 21

[sujay]

verified in 6/5 trunk build.

Comment 22

[scottputterman]

adding adt1.0.1+.  Please get drivers approval before checking into the branch.

Comment 23

[Judson Valeski]

please checkin to the 1.0.1 branch. once there remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 24

[Judson Valeski]

Comment on attachment 83960 [details] [diff] [review]
patch v4

assuming the debugoutput=true piece is not hitting the branch, here's the
approval.

Comment 25

[Charles Manske]

checked into mozilla1.0.1 branch

Comment 26

[Charles Manske]

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

Comment 27

[Charles Manske]

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

Comment 28

[sujay]

verified in 7/17 branch

removing fixed1.0.1 keyword