Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100619 Minefield/3.7a6pre Dragging an HTML file to Firefox triggers: ###!!! ASSERTION: Expected word size: 'tempDataLen == 2', file c:/Users/jruderman/mozilla-central/widget/src/windows/nsNativeDragTarget.cpp, line 267 I'm using Windows 7 (64-bit) but my Firefox is 32-bit.

The problem was that CFSTR_PREFERREDDROPEFFECT was being checked with strlen but it should contain a 4 byte DWORD always, not a string. As per the MSDN doc entitled: "Shell Clipboard Formats" CFSTR_PREFERREDDROPEFFECT should return a DWORD. Reference: http://msdn.microsoft.com/en-us/library/bb776902(v=vs.85).aspx The strlen call in the else block below was therefore setting the data length depndent on what the preferred method of drop was. if ( NS_SUCCEEDED(GetGlobalData(stm.hGlobal, aData, &allocLen)) ) { if ( fe.cfFormat == CF_HTML ) { *aLen = allocLen; } else { *aLen = nsCRT::strlen(reinterpret_cast<PRUnichar*>(*aData)) * sizeof(PRUnichar); } I also cleaned up the preference handling code when we determine the drop type. It was wrongly only checking for move preference.

Comment on attachment 552927 [details] [diff] [review] Patch for wrong drop preference handling I've never seen this assertion. Does anyone have any more specific STR? > // Only a single effect should be specified outgoing for the parameter pdwEffect. This is starting to get repetitive. This bug might not be the best place to fix this, but I was thinking along the following lines: 1. Local variable desiredEffect which is initially one of DROPEFFECT_LINK, DROPEFFECT_MOVE, DROPEFFECT_COPY or DROPEFFECT_NONE depending on modifiers 2. if (!(desiredEffect &= mEffectsAllowed)) { desiredEffect = mEffectsPreferred & mEffectsAllowed; if (!desiredEffect) desiredEffect = mEffectsAllowed; } 3. Check desiredEffect for MOVE, COPY or LINK in order as before. > nsresult loadResult = nsClipboard::GetNativeDataOffClipboard( > pIDataSource, 0, ::RegisterClipboardFormat(CFSTR_PREFERREDDROPEFFECT), nsnull, &tempOutData, &tempDataLen); [Is it me or are we supposed to NS_Free tempOutData when we're done?] >+ // We have no preference if we can't obtain it >+ mEffectsPreference = 0; Do we get one drag target per drag or is it shared? [Also, unnecessary trailing space.]

> I've never seen this assertion. Does anyone have any more specific STR? There are 2 side effects of this bug: 1) Improper preferred effects handling 2) Assertions hit what seems to be randomly when dragging files onto Firefox. It was introduced in Bug 296528. Here's a better explanation of why it happens, the steps to reproduce will follow: Windows will fill the data returned from CFSTR_PREFERREDDROPEFFECT with some combinations of drop effects. Windows returns a DWORD to us, so size 4. Recall: #define DROPEFFECT_NONE 0 #define DROPEFFECT_COPY 1 #define DROPEFFECT_MOVE 2 #define DROPEFFECT_LINK 4 #define DROPEFFECT_SCROLL 0x80000000 If you stuff a 4 byte DWORD with any combination of values above, and interpret its memory as a char* and call strlen on it, in all combinations it would return a size of 1. Why? Because 1|2|4 == 7 < 255 and 0x80000000 has some zeros before it's digit so strlen sees those as end of string. This is what the bug was. The size of 1 from strlen was multiplied by sizeof(PRUnichar) giving the usual return value of 2. So the assertion from Bug 296528 assumed it was always 2 that was being returned, but in effect it depends on the drop effect. So it follows that 2 ways to reproduce is for windows to fill with either DROPEFFECT_NONE or DROPEFFECT_SCROLL which would return a value of 0 from strlen. In those cases the call succeeds but the length is not 2. Assertion hit. So to reproduce simply take a file from windows explorer and drag it around in and out of your window. If it doesn't work try another file. You should be able to reproduce it pretty easily. > This [in reference to the condition blocks] is starting to get repetitive. This bug might not be the best place to fix this, I was thinking this myself as well but I think the more verbose version is more readable. I don't mind changing it though, and I do like the way yours reads, so I will submit a new patch with that way. > [Is it me or are we supposed to NS_Free tempOutData when we're done?] It seems that we call nsMemory::Alloc to get this memory but I don't see an associated nsMemory::Free, so I think it should be fixed. Don't mochitests check for memory leaks though? Wouldn't a mochitest somewhere fail because of this? Let me know, I'll file a separate bug for that since it's not caused by this task. > Do we get one drag target per drag or is it shared? It is shared within each window. I.e. each nsWindow has one.

Review comments implemented. Regarding the potential mem leak from Bug 296528, let me know about my questions regarding detection and I'll file a new bug about that. > I think the more verbose version is more readable Never mind, I like yours much better once commented.

(In reply to Brian R. Bondy from comment #3) > > I've never seen this assertion. Does anyone have any more specific STR? > So to reproduce simply take a file from windows explorer and drag it around > in and out of your window. If it doesn't work try another file. You should > be able to reproduce it pretty easily. I only asked because I couldn't reproduce it at all. Certainly I can try dragging files, folders, shares and drives in to my window and I get a number of drop effects (particularly when dropped into bookmarks) but no assertion. > > [Is it me or are we supposed to NS_Free tempOutData when we're done?] > It seems that we call nsMemory::Alloc to get this memory but I don't see an > associated nsMemory::Free, so I think it should be fixed. > Don't mochitests check for memory leaks though? Wouldn't a mochitest > somewhere fail because of this? It's very unlikely that a mochitest would test dragging from another application that used a preferred drop effect. > Let me know, I'll file a separate bug for that since it's not caused by this > task. Sure, I wasn't expecting you to fix that in this bug.

Comment on attachment 552972 [details] [diff] [review] Patch for wrong drop preference handling v2 By the way, you use 0 in a couple of places but it would probably be slightly more readable if you used DROPEFFECT_NONE instead.

> It's very unlikely that a mochitest would test dragging from another application Ya thought of that when I was driving today, perhaps we can come up for a test case too that tests it in the context of that bug. > ...but no assertion I'm not sure how windows picks when to set the preferred effect, it is not consistent for me. I know it sets on FTP always, and sometimes on windows explorer. The one on FTP though is the one of size 2 which didn't cause an assertion. When I am reproducing it I am just dragging a single .ico or .txt file from Win7 x64 from C: windows explorer. I think any file type would be able to reproduce though. Maybe try from a UNC path.

> you use 0 in a couple of places it would probably be slightly more readable if you used DROPEFFECT_NONE instead I do this to signify the absence of a bit being set. Since it is being checked with bitwise operators people reading the code would have need to know that DROPEFFECT_NONE is set to 0 to fully understand the code. I can change if you want though, please advise.

I guess it just depends how you think about it, I'll do the DROPEFFECT_NONE for kicks and attach a new patch shortly.

Changed unset effects from 0 to DROPEFFECT_NONE

Fixed alignment spacing.

Posted "Bug 679196 - Mem leak when preferred effect is set for drag and drop" about the mem leak. Let me know about the rest of the review whenever you get a chance.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a34d0baace5c

This is ready to be pushed if it gets an r+. Assertions are gone with this patch and passes tests when pushed to try.

Review ping

Comment on attachment 553037 [details] [diff] [review] Patch for wrong drop preference handling v4 Sorry for the delay, I thought I was looking for an application that would give me a preferred drop effect but as you said I can just drag from FTP.

I re-pushed to try because the whole patch needed rebasing: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=517f45e4dd2e I'll push to inbound once that succeeds.

Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/fd7827c6e05b