106708 - DnD Data Object format incorrect

Status: RESOLVED FIXED

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

Description

[Robin Dunn]

Links dragged from Mozilla (from the address bar or from the body text) are not
in the right format to be compatible with other Widnows Apps.  In an App that
accepts CFSTR_SHELLURL format data objects it appears that Mozilla is giving it
a Unicode string.  MSDN clearly states that CFSTR_SHELLURL is the same format as
CF_TEXT and CF_TEXT should contain ANSI strings:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/Shell/Shell_basics/clipboard.asp

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/ipc/clipbrd_0cxf.asp

Comment 1

[Asa Dotzler [:asa]]

->drag and drop component. probably already reported

Comment 2

[Adrian Buckley]

This has definitely already been reported - searches for "Drag" and "Unicode"
brought up the following candidates:

bug 40608 - text/unicode used in drag and drop not a valid IANA media type
bug 63177 - Drag and Drop to Drop Baskets broken with Gozilla and Net Vampire
bug 91328 - URL Drag/Drop does not function correctly

and possibly:

bug 128384 - Can't drag link from Mozilla to IE

Can someone at Mozilla look at combining these bugs into one "superbug", and
raise the priority accordingly?

Comment 3

[Michael Lefevre]

already reported, but it seems this is more general than the others - except
possibly bug 40608, which I'm not sure about.

confirming, adding what I think are dependencies (or maybe dupes) bug 128384,
bug 63177, bug 91328, bug 113457, bug 138830.

Comment 4

[Andrew Hagen]

Is QA wanted anymore for this bug? If not, we can remove the keyword.

Comment 5

[Adrian Buckley]

Attachment: Proposed patches

The clipboard appears to be generating the UniformResourceLocator
automatically, using the internal text/x-moz-url object to do so (which is held
in UNICODE format).

This patch generates a non-unicode UniformResourceLocator manually instead.

I'm no expert on C++, the Clipboard or Drag-and-Drop, so this is a
cut-and-paste job using code from other functions within nsDataObj.cpp (ie code
that other people have written). As a result, the code will need checking for
memory leaks, code formatting errors etc. for which I apologise :-)

The patch may also not be for the latest source code - pulling the latest code
using CVS isn't an option with my current setup (download source at work, and
compile at home) ;-)

Comment 6

[:deanis74]

Actually, MSDN states that CFSTR_SHELLURL is deprecated and it looks like the
now-supported CFSTR_INETURL handles Unicode.

"CFSTR_INETURL
This format identifier replaces CFSTR_SHELLURL (deprecated). If you want your
application to manipulate clipboard URLs, use CFSTR_INETURL instead of
CFSTR_SHELLURL (deprecated). This format gives the best clipboard representation
of a single URL. If UNICODE is not defined, the application retrieves the
CF_TEXT/CFSTR_SHELLURL version of the URL. If UNICODE is defined, the
application retrieves the CF_UNICODE version of the URL"

So is it as easy as replacing all occurrences of CFSTR_SHELLURL with CFSTR_INETURL?

Comment 7

[Adrian Buckley]

I've just tried using CFSTR_INETURL, and got an undeclared identifier error -
CFSTR_INETURL is not defined in the shlobj.h library that came with Visual C++ 6.0.

Is it defined in the .NET libraries?

The MSDN page is too short on details on CFSTR_INETURL for my liking: When did
SHELL_URL become deprecated? How do older Operating Systems and applications
handle CFSTR_INETURL?

http://msdn.microsoft.com/workshop/author/datatransfer/overview.asp may be relevant.
This page says "Internet Explorer 4.0 has limited data transfer functionality"
and "Internet Explorer 5 greatly improved and enhanced this functionality".

Does this equate to CFSTR_SHELLURL and CFSTR_INETURL respectively?

Comment 8

[:deanis74]

Yeah, I tried that at home last night and couldn't find it either.  Not only
that, but there's zero information on the web about CFSTR_INETURL.  I think this
is probably the better way to go, then.  All in all, the logic makes sense to
me.  I'm no clipboard guru either - it's too confusing for anyone to be - but
I've played around in nsDataObj.cpp previously and this patch jives with how we
handle things.

+  const int totalLen = strlen(urlStr)+1;

Instead of 1 use sizeof(char).

+    char* contents = NS_REINTERPRET_CAST(char*, ::GlobalLock(hGlobalMemory));
+    PR_snprintf( contents, totalLen, UrlFormatStr, urlStr );

Isn't there a better way to copy the data over to |contents|?  It looks like
this was copied from  GetFileContentsInternetShortcut, but there the format
string is more complicated than just "%s".  Why not just copy the memory
directly using memcpy?

     memcpy(contents, urlStr, 

+nsDataObj :: SetupUniformResourceLocator ( FORMATETC& aFE, STGMEDIUM& aSTG )

I'm not crazy about a function named "Setup".

+nsDataObj :: GetUniformResourceLocator ( FORMATETC& aFE, STGMEDIUM& aSTG )
...
+      res = SetupUniformResourceLocator ( aFE, aSTG );
...
+  if ( NS_FAILED(ExtractShortcutURL(url)) )

Style nit - remove the spaces around "::", "(", and ")".

Can someone double-check the PR string stuff to make sure nothing's leaking?  I
don't trust myself around strings.

Comment 9

[Adrian Buckley]

Attachment: Modified patch per comment 8

Submitting a second patch following Dean's suggestions and constructive
criticism.

I did warn you that C++ wasn't my first language - PERL, PHP, JAVASCRIPT,
DELPHI - I've (ab)used them all ;-)
Once I compiled something that "worked" (i.e. did more than send a Unicode h!),
I opened a celebratory beer, uninstalled NS4.7 and submitted the patch KNOWING
that there were bound to be probably style inconsistencies, bugs and flaws that
would make C++ gurus ROFL!


Dean: Regarding CFSTR_INETURL

I found a magazine coverdisk at work today with a copy of the VS .NET BETA2 on
it, and found some interesting bits of code in shlobj.h:

#define CFSTR_SHELLURL			    TEXT("UniformResourceLocator")
#define CFSTR_INETURLA			    CFSTR_SHELLURL
#define CFSTR_INETURLW			    TEXT("UniformResourceLocatorW")

#ifdef UNICODE
#define CFSTR_INETURL		CFSTR_INETURLW
#else
#define CFSTR_INETURL		CFSTR_INETURLA
#endif

If I understand the above correctly, for non-unicode platforms CFSTR_INETURL
equates to CFSTR_SHELLURL - so just changing  CFSTR_SHELLURL to CFSTR_INETURL
would accomplish precisely _nothing_ on Win9x platforms :-)

Comment 10

[timeless]

Comment on attachment 90835 [details] [diff] [review]
Modified patch per comment 8

>@@ -215,6 +216,8 @@
>               return GetFileDescriptor ( *pFE, *pSTM );
>             else if ( format == fileFlavor )
>               return GetFileContents ( *pFE, *pSTM );
>+            else if ( format == UniformResourceLocator )
>+	      return GetUniformResourceLocator( *pFE, *pSTM );
there's a tab on this line. between mozilla's html and editor views it's very
easy to spot -- i'm growing to love mozilla for code reviews.  Tabs are bad. In
mozilla, you should *never* need them.	(it is true that some core makefiles
use them, but most of the makefiles you'll see in mozilla are just assigning
variables since the rules are hidden out of site, and for variables whitespace
is just a delimeter.
>             else {
>               PRNTDEBUG2("***** nsDataObj::GetData - Unknown format %x\n", format);
> 					    return GetText(*df, *pFE, *pSTM);
not your fault but..
"else after return". what this means is that the code doesn't make sense
instead you should do:
if (condition)
  return something;
/*noelse, because the only way to reach here is !cond*/
if (cons2)
  return thing2;
/*noelse, because the only way to reach here is !cond*/
...

>@@ -944,3 +947,55 @@
> 
>   return NS_OK;
> }
>+
>+HRESULT 
>+nsDataObj::GetUniformResourceLocator(FORMATETC& aFE, STGMEDIUM& aSTG)
>+{
>+  HRESULT res = S_OK;
>+  if (IsInternetShortcut())  
>+  {
>+      res = ExtractUniformResourceLocator(aFE, aSTG);
>+  }
indentation above is 4, indentation below is 2, please be consistent and match
the file (which appears to use 2).
>+  else
>+    NS_WARNING ("Not yet implemented\n");
there's a not yet implemented macro lying around somewhere, but this is fine.
Although i'm not sure I like the \n.
>+  return res;
>+}
>+
i don't see a need for two newlines ^v
>+
>+HRESULT
>+nsDataObj::ExtractUniformResourceLocator(FORMATETC& aFE, STGMEDIUM& aSTG)

dean seems to like the code wrt w32api and i've nitted wrt moz style.

Comment 11

[timeless]

oh right, wrt your w9x comment.

microsoft is encouraging everyone to use:
The Microsoft® Layer for Unicode™ on Windows® 95/98/ME Systems

and we're planning to convert, you should keep that in mind.

Comment 12

[Adrian Buckley]

Attachment: modified patch per comment 10

Removed unnecessary nested-IFs: C++ functions exit immediately after executing
a RETURN statement. Also edited for nits as requested in comment 10.

Comment 13

[Adrian Buckley]

Attachment: lost a line

The last patch is a line of code missing - this patch actually works!

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 93555 [details] [diff] [review]
lost a line

r=pink. it would be nice to avoid the alloc/copy when creating the cstring, but
i won't hold it based on that. could jag help out perhaps?

Comment 15

[jag (Peter Annema)]

Comment on attachment 93555 [details] [diff] [review]
lost a line

>+HRESULT
>+nsDataObj::ExtractUniformResourceLocator(FORMATETC& aFE, STGMEDIUM& aSTG)
>+{
>+  HRESULT result = S_OK;
>+  
>+  nsAutoString url;
>+  if (NS_FAILED(ExtractShortcutURL(url)))
>+    return E_OUTOFMEMORY;

Replace

>+  char* urlStr = ToNewCString(url);
>+  if ( !urlStr )
>+    return E_OUTOFMEMORY;

with:

  NS_LossyConvertUCS2toASCII urlC(url);

This has an internal buffer of 64 characters, allocated on the stack in this
case. It'll fall back to heap allocation when the url is larger than that.

>+
>+  // setup format structure
>+  FORMATETC fmetc = { 0, NULL, DVASPECT_CONTENT, 0, TYMED_HGLOBAL };
>+  fmetc.cfFormat = RegisterClipboardFormat ( CFSTR_SHELLURL );
>+
>+  // create a global memory area and build up the file contents w/in it
>+  const int totalLen = strlen(urlStr)+sizeof(char);

You can just say + 1 here, sizeof(char) is defined as 1.

>+
>+  HGLOBAL hGlobalMemory = ::GlobalAlloc(GMEM_ZEROINIT|GMEM_SHARE, totalLen);
>+
>+  // Copy text to Global Memory Area
>+  if ( hGlobalMemory ) {
>+    char* contents = NS_REINTERPRET_CAST(char*, ::GlobalLock(hGlobalMemory));
>+    memcpy(contents, urlStr, totalLen);

And replace urlStr here with urlC.get()

>+    ::GlobalUnlock(hGlobalMemory);
>+    aSTG.hGlobal = hGlobalMemory;
>+    aSTG.tymed = TYMED_HGLOBAL;
>+  }
>+ 

And because of the above you won't have to worry about freeing it here.
Btw, that comment was incorrect, the memory was created by itself.

>+  // Now, delete the memory that was created by ExtractShortcutURL
>+  nsMemory::Free(urlStr);
>+
>+  return result;
> }

sr=jag with those changes made.

Comment 16

[Adrian Buckley]

Attachment: Modified patch per comment 15

>+  const int totalLen = strlen(urlStr)+sizeof(char);
>You can just say + 1 here, sizeof(char) is defined as 1.

In comment 8 I was asked to change FROM +1 TO sizeof(char) - oh well ;-)

Comment 17

[Terri Preston]

qa contact -> pmac

Comment 18

[Asa Dotzler [:asa]]

Adrian, is this ready to land? It's on the list of bugs that drivers is
interested in for 1.2beta (which is happening rsn)

Comment 19

[:deanis74]

Comment on attachment 99495 [details] [diff] [review]
Modified patch per comment 15

> In comment 8 I was asked to change FROM +1 TO sizeof(char) - oh well ;-)

My review still stands.  I always use sizeof(char) , just in case it changes
some day.  But either is fine with me.

Comment 20

[Adrian Buckley]

This patch still needs super-review - unless the super-review stands from the
previously attempted patch.

This is my first patch - assuming it gets super-review how do I get it checked in?

Comment 21

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

+  FORMATETC fmetc = { 0, NULL, DVASPECT_CONTENT, 0, TYMED_HGLOBAL };
+  fmetc.cfFormat = RegisterClipboardFormat ( CFSTR_SHELLURL );

shouldn't this just be

+  FORMATETC fmetc = { UniformResourceLocator, NULL, DVASPECT_CONTENT, 0,
TYMED_HGLOBAL };

?

I'll give sr on the assumption that this is done.

Comment 22

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

Comment on attachment 99495 [details] [diff] [review]
Modified patch per comment 15

sr=roc+moz

Comment 23

[Adrian Buckley]

Robert,

+ FORMATETC fmetc = { UniformResourceLocator, NULL, DVASPECT_CONTENT, 0,
TYMED_HGLOBAL };
Failed to compile:
d:/MOZILLA/mozilla/widget/src/windows/nsDataObj.cpp(971) : error C2065: 'Uniform
ResourceLocator' : undeclared identifier

It looks like:

+  FORMATETC fmetc = { 0, NULL, DVASPECT_CONTENT, 0, TYMED_HGLOBAL };
+  fmetc.cfFormat = RegisterClipboardFormat ( CFSTR_SHELLURL );

was right after all.

Comment 24

[Christopher Blizzard (:blizzard)]

Is this ready to land or what?  It's on the 1.2 list.

Comment 25

[Adrian Buckley]

Robert O'Callahan in comments 21/22 says he gives sr on the assumption that I
change the patch - but changing the patch causes build errors.

This is a bit of a Catch-22 situation!

My patch IS consistent with the rest of nsDataObj.cpp however (lines 474/5 and
572/3).

Comment 26

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

Sorry about that. I wasn't CCed on the bug so I never saw your comment until now.

+  FORMATETC fmetc = { 0, NULL, DVASPECT_CONTENT, 0, TYMED_HGLOBAL };
+  fmetc.cfFormat = RegisterClipboardFormat ( CFSTR_SHELLURL );

Change this to

+  static CLIPFORMAT UniformResourceLocator = ::RegisterClipboardFormat(
CFSTR_SHELLURL );
+  FORMATETC fmetc = { UniformResourceLocator, NULL, DVASPECT_CONTENT, 0,
TYMED_HGLOBAL };

Comment 27

[:deanis74]

Why not just...

FORMATETC fmetc = { ::RegisterClipboardFormat(CFSTR_SHELLURL), NULL, 
DVASPECT_CONTENT, 0, TYMED_HGLOBAL };

Comment 28

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

Because then we call ::RegisterClipboardFormat every time through the code,
instead of just the first time.

Comment 29

[:deanis74]

Duh... 'static'.  My bad.

Comment 30

[Adrian Buckley]

Attachment: The final patch?

Modified patch as requested in comment 26.

Comment 31

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

Comment on attachment 104950 [details] [diff] [review]
The final patch?

sr=roc+moz
bringing forward previous r=

Comment 32

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

Now send email to [e-mail] to get approval for checkin for 1.2.
Explain why you think this is important for 1.2 and why there is a low risk of
regressions.

Thanks!!!

Comment 33

[Christopher Blizzard (:blizzard)]

Comment on attachment 104950 [details] [diff] [review]
The final patch?

a=blizzard on behalf of drivers for 1.2 final.

Please try to get this in before the morning tree closure on Nov 5th, 2002
otherwise it's going to have to wait until after the branch is cut.

Comment 34

[Adrian Buckley]

Checked in to trunk.