396592 - Drag and drop images from the content area fail

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

Vista only, I checked XP and it's working fine. Dragging an image from the content area to the desktop or some other folder you have permissions in fails to copy the image over. The file copy dialog will show, but the file never shows up.

Comment 1

[Jim Mathies [:jimm]]

Appears to be caused by our half hearted implementation of IStream in widget/src/windows/nsDataObj.cpp.

Comment 2

[Jim Mathies [:jimm]]

Yep, that's it. Stat and Seek need to return valid information.

Comment 3

[Jim Mathies [:jimm]]

Attachment: vista drag image patch v.1

Comment 4

[Ere Maijala (slow)]

Comment on attachment 281555 [details] [diff] [review]
vista drag image patch v.1

Sorry, I can't test Vista yet (I'll try to get it installed in the next week or two...).

Comment 5

[Ere Maijala (slow)]

Comment on attachment 281555 [details] [diff] [review]
vista drag image patch v.1

+    PRUint32 nMaxNameLength = (wideFileName.Length()*2) + 2;
+    void * retBuf = CoTaskMemAlloc(nMaxNameLength); // freed by caller
+    if (!retBuf) 
+      return STG_E_INSUFFICIENTMEMORY;
+
+    ZeroMemory(retBuf, nMaxNameLength);
+    wcsncpy(statstg->pwcsName, wideFileName.get(), nMaxNameLength);

This is wrong, retBuf is allocated but wcsncpy's destination is statstg->pwcsName. Why ZeroMemory as you're allocating just what's needed?

+  if (!mChannel)
+    return E_NOTIMPL;
.
.
.
+  if (mChannel)
+    mChannel->GetContentLength(&nLength);

Which one did you mean? Return error if !mChannel or return what's possible. I'd go for the latter. 

Also, wouldn't E_FAIL be the correct return value instead of E_NOTIMPL in the error cases?

Comment 6

[Jim Mathies [:jimm]]

>This is wrong, retBuf is allocated but wcsncpy's destination is
>statstg->pwcsName. Why ZeroMemory as you're allocating just what's needed?

I'm adding a character on the end, and zeroing out the entire buffer to null terminate it. There's really no harm in that afaict. I'll fix the buffer issue and repost here in a sec.

Comment 7

[Jim Mathies [:jimm]]

Attachment: vista drag image patch v.2

This addresses the missing buffer asignment. I've also changed the not implemented returns to E_FAIL.

Comment 8

[Ere Maijala (slow)]

Comment on attachment 284002 [details] [diff] [review]
vista drag image patch v.2

r=emaijala

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (nMove.LowPart == 0 && 
+      nNewPos && 
+      (dwOrigin == STREAM_SEEK_SET || dwOrigin == STREAM_SEEK_CUR)) { 
+    nNewPos->LowPart = NULL;
+    nNewPos->HighPart = NULL;
+    return S_OK;
+  }

Shouldn't we test nMove.HighPart == 0 as well?

Shouldn't we return S_OK even when nNewPos is null? (or should we return some other error in that case?)

And shouldn't those NULLs be 0s?

+    wcsncpy((wchar_t *)retBuf, wideFileName.get(), wideFileName.Length());

Can't we just use memcpy here?

+  SystemTimeToFileTime((const SYSTEMTIME*)&st, (LPFILETIME)&statstg->ctime);
+  SystemTimeToFileTime((const SYSTEMTIME*)&st, (LPFILETIME)&statstg->atime);

Can't these just be "*statstg->ctime = *statstg->atime = *statstg->mtime;"?

Comment 10

[Jim Mathies [:jimm]]

Attachment: vista drag image patch v.3

>Shouldn't we test nMove.HighPart

updated.

>Shouldn't we return S_OK even when nNewPos is null?

msdn suggested we use STG_E_INVALIDPOINTER.

>Can't we just use memcpy here?

updated.

>"*statstg->ctime = *statstg->atime = *statstg->mtime;"?

updated (without the dreferences which weren't needed).

Comment 11

[Jim Mathies [:jimm]]

Attachment: vista drag image patch v.4

sorry, minor touch up, removed the unneeded nNewPos check in the if statement.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Restoring awol blocker flag.

Comment 13

[Reed Loden [:reed]]

Checking in widget/src/windows/nsDataObj.cpp;
/cvsroot/mozilla/widget/src/windows/nsDataObj.cpp,v  <--  nsDataObj.cpp
new revision: 1.85; previous revision: 1.84
done

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

I tested:

* email addresses (shortcut created was a mailto:)
* links (shortcut created was a hyperlink)
* images (the raw image was copied to the desktop, to where I dragged it)

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102407 Minefield/3.0a9pre