280947 - Fix for bug 279945 breaks dragging of dynamic images

Status: VERIFIED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The fix for bug 279945 disallowed dragging of images which don't have an image
extension.  I think that code should be removed and replaced with code that
changes the extension on the filename we save to appropriately.

Specifically, if we have an imgIRequest (if we don't, it's not an image) and if
the extension on the URI is not an image extension at
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsContentAreaDragDrop.cpp#1108
or so we should get the type off the imgIRequest, then call
nsIMIMEService.getPrimaryExtension on it to get the extension we want to use and
append that to the URI.

Comment 1

[Daniel Veditz [:dveditz]]

Is dragging of images anything more than a convenience? This doesn't stop anyone
from doing a Save As for the "broken" images.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Dragging is a LOT more prevalent than using the context menu on Mac.  So on Mac,
this is a serious usability regression.

Further, Firefox has some known bugs with Save As that keep it from saving a lot
of dynamically generated pages (including images).  Those are exactly the ones
worst affected by the fix for bug 279945.

Finally, the fix for bug 279945 blocked dragging on all platforms, whereas the
security hole was not present on platforms where the extension doesn't determine
whether the file is executable (say Linux).

So in brief, we introduced usability issues on Linux and Mac to fix a Windows
security problem.  ;)

In any case, I'm not sure what the point of comment 1 is.  Are you arguing that
we shouldn't fix this the "right" way?

Comment 3

[Simon Fraser [no longer active]]

Boris is right; for some users, drag and drop is their primary means of saving
images. We should not break this.

Comment 4

[Johnny Stenback (:jst)]

Attachment: "fix" the file extension in the image URI when d&d'ing an image...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm.  So what happens if the image URI is data:?   Or some other non-nsIURL
type?  Should we perhaps just append the extension to the spec in that case, or
something? 

Do we really want to set mImage if mImageSourceString is not set?

Also, what's the purpose of the nodeToSerialize change?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Btw, I'm assuming that this was tested on Windows, right?

Comment 7

[Daniel Veditz [:dveditz]]

Probably want this on branches that got (or might get) the fix for bug 279945,
adding nomination flags

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Plussing to ensure this bug doesn't exist on the branches.

Comment 9

[Johnny Stenback (:jst)]

(In reply to comment #5)
> Hmm.  So what happens if the image URI is data:?   Or some other non-nsIURL
> type?  Should we perhaps just append the extension to the spec in that case, or
> something? 

Currently we're unable to drop such an image into the file system (see
nsContentAreaDragDrop::SaveURIToFileInDirectory(), it bails if the uri is not an
nsIURL), and I see no reason to change that now.

> Do we really want to set mImage if mImageSourceString is not set?

Probably not, I moved the setting of mImage to the same place where
mImageSourceString is now set.

> Also, what's the purpose of the nodeToSerialize change?

Cut n' paste fun. Ignore those.

New patch coming up.

Comment 10

[Boris Zbarsky [:bzbarsky]]

One other thing.  Even if we're failing to drop them into the filesystem, we may
be able to drop data: and such images into other apps or other Mozilla frames
(worth checking).  If so, we should not break that.  In other words, if the URI
is not a URL, I think we should just set mImageSourceString to the URI and set
mImage.  Bug 279945 won't reappear because we can't drop those into the
filesystem, but dragging elsewhere will work...

Comment 11

[Johnny Stenback (:jst)]

Attachment: Fix bz's issues.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

+              mImageSourceString = NS_ConvertUTF8toUTF16(spec);

CopyUTF8toUTF16?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase for dragging data: images -- drag the green square

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173629 [details] [diff] [review]
Fix bz's issues.

r=bzbarsky with biesi's nit....  Note that dragging of data: images is already
broken, even without this patch.  :(  Please file a followup bug to get it
fixed?

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 173629 [details] [diff] [review]
Fix bz's issues.

sr=dveditz

Comment 16

[Johnny Stenback (:jst)]

Fix landed, data: URL bug on file as bug 281431.

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 173629 [details] [diff] [review]
Fix bz's issues.

a=asa for branches checkins.

Comment 18

[Johnny Stenback (:jst)]

Ahem, turns out that this fix wasn't right after all. We now properly update the
file extension in the URL, but we do that too early and we end up trying to
download the image data from the URL with the changed extension, and most likely
get no data. Fixing this won't be trivial as we need to either change this later
on in the drag n' drop process, or pass along not only the source of the image,
but also the target filename that we want, which may be different due to the
file extension differences.

Comment 19

[Daniel Veditz [:dveditz]]

What's the status of this one? comment 18 implies we need a new patch, does that
mean the currently reviewed and approved one should be obsoleted?

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 173629 [details] [diff] [review]
Fix bz's issues.

Yeah, this one ain't good for the branch, and we need this fixed on the trunk
too... I may get to looking at this tomorrow...

Comment 21

[Johnny Stenback (:jst)]

Attachment: Additional patch needed to download the right file with the fixed extension

Comment 22

[Johnny Stenback (:jst)]

Attachment: Same thing for the 1.0.1 branch

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174494 [details] [diff] [review]
Additional patch needed to download the right file with the fixed extension

>Index: content/base/src/nsContentAreaDragDrop.cpp

>@@ -1130,38 +1120,51 @@ nsTransferableFactory::Produce(nsITransf

>+              // make the filename safe for the filesystem
>+              fileName.ReplaceChar('/', '_');
>+              fileName.ReplaceChar('\\', '_');
>+              fileName.ReplaceChar(':', '_');

I realize you just moved the existing code, but said code is wrong.  If nothing
else, it misses a whole bunch of characters that are not valid in filenames on
Windows.  The way nsExternalHelperAppService.cpp handles this is:

  mSuggestedFileName.ReplaceChar(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS,
'-');

(needs nsCRT.h for the FILE_* macros).	Do the same here (with fileName, of
course).

r=bzbarsky with that.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 174494 [details] [diff] [review]
Additional patch needed to download the right file with the fixed extension

sr=dveditz with the change requested by bz
a=dveditz for branches

Comment 25

[Johnny Stenback (:jst)]

Fixed on the branches.

Comment 26

[Johnny Stenback (:jst)]

Fixed.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 174494 [details] [diff] [review]
Additional patch needed to download the right file with the fixed extension

too late for 1.8b1; please land on trunk (which is now open)

Comment 28

[sairuh (rarely reading bugmail)]

tested with 2005021806-1.0.1 firefox builds on Mac 10.3.8 and WinXP:

1. went to http://www.mikx.de/firedragging/
2. dragged the firefox image to the desktop

results: the file foximg.gif was saved to the desktop, and I was able to view it
using the default image viewer for the respective system.

question: is this correct behavior (I'm guessing yes, but want to be sure.)

Comment 29

[Boris Zbarsky [:bzbarsky]]

Yes, that's the right behavior (extension is forced to correspond to the MIME type).

Comment 30

[sairuh (rarely reading bugmail)]

(In reply to comment #29)
> Yes, that's the right behavior (extension is forced to correspond to the MIME
type).

I just checked this on 2005021807-1.0.1 firefox on linux fc2, and when I d'n'd
the image onto the desktop, the filename remained foximg.bat. :-\  when I
double-clicked the image, it opened another tab in firefox (set as the default
browser), displaying the image, with "foximg.bat (GIF Image, 88x31 pixels)" in
the titlebar.

is this an acceptable result, or does that mean this hasn't been properly fixed?
 (or perhaps another bug?)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Hmm... Odd.  Could you do a log of the "HelperAppService" module for that
operation (just like
http://www.mozilla.org/projects/netlib/http/http-debugging.html says but set
NSPR_LOG_MODULES=HelperAppService:5) and email it to me?

Comment 32

[Johnny Stenback (:jst)]

Not sure what the deal is, but linux apparently never saves images to the system
when dragged, it just saves a shortcut to the URL of the image. That's why we
still see foximg.bat on linux, it's just a link to the image on the server.

Comment 33

[sairuh (rarely reading bugmail)]

so comment 30 would be expected behavior on linux? if that's the case, I can
verify this...

Comment 34

[Boris Zbarsky [:bzbarsky]]

Yes, that sounds like the right behavior on Linux.

Comment 35

[sairuh (rarely reading bugmail)]

thanks!