573321 - Preferred effect is not determined properly which sometimes causes "ASSERTION: Expected word size" when dragging

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jesse Ruderman]

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.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch for wrong drop preference handling

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 2

[[e-mail]]

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.]

Comment 3

[Brian R. Bondy [:bbondy]]

> 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.

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch for wrong drop preference handling v2

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.

Comment 5

[[e-mail]]

(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 6

[[e-mail]]

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.

Comment 7

[Brian R. Bondy [:bbondy]]

> 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.

Comment 8

[Brian R. Bondy [:bbondy]]

> 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.

Comment 9

[Brian R. Bondy [:bbondy]]

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

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Patch for wrong drop preference handling v3

Changed unset effects from 0 to DROPEFFECT_NONE

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Patch for wrong drop preference handling v4

Fixed alignment spacing.

Comment 12

[Brian R. Bondy [:bbondy]]

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.

Comment 13

[Brian R. Bondy [:bbondy]]

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

Comment 14

[Brian R. Bondy [:bbondy]]

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

Comment 15

[Brian R. Bondy [:bbondy]]

Review ping

Comment 16

[[e-mail]]

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.

Comment 17

[Brian R. Bondy [:bbondy]]

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.

Comment 18

[Brian R. Bondy [:bbondy]]

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

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/fd7827c6e05b