Last Comment Bug 573321 - Preferred effect is not determined properly which sometimes causes "ASSERTION: Expected word size" when dragging
: Preferred effect is not determined properly which sometimes causes "ASSERTION...
Status: RESOLVED FIXED
: assertion
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla10
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on: 633160
Blocks: 296528 679196
  Show dependency treegraph
 
Reported: 2010-06-20 01:51 PDT by Jesse Ruderman
Modified: 2011-10-01 02:21 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for wrong drop preference handling (7.95 KB, patch)
2011-08-13 20:45 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for wrong drop preference handling v2 (9.15 KB, patch)
2011-08-14 06:41 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for wrong drop preference handling v3 (9.25 KB, patch)
2011-08-14 14:52 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for wrong drop preference handling v4 (9.24 KB, patch)
2011-08-14 14:54 PDT, Brian R. Bondy [:bbondy]
neil: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2010-06-20 01:51:36 PDT
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] 2011-08-13 20:45:45 PDT
Created attachment 552927 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-08-14 02:56:04 PDT
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] 2011-08-14 06:19:29 PDT
> 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] 2011-08-14 06:41:57 PDT
Created attachment 552972 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-08-14 14:23:10 PDT
(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 neil@parkwaycc.co.uk 2011-08-14 14:30:49 PDT
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] 2011-08-14 14:31:23 PDT
> 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] 2011-08-14 14:35:21 PDT
> 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] 2011-08-14 14:45:18 PDT
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] 2011-08-14 14:52:04 PDT
Created attachment 553036 [details] [diff] [review]
Patch for wrong drop preference handling v3

Changed unset effects from 0 to DROPEFFECT_NONE
Comment 11 Brian R. Bondy [:bbondy] 2011-08-14 14:54:01 PDT
Created attachment 553037 [details] [diff] [review]
Patch for wrong drop preference handling v4

Fixed alignment spacing.
Comment 12 Brian R. Bondy [:bbondy] 2011-08-15 16:21:14 PDT
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] 2011-09-08 09:29:15 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a34d0baace5c
Comment 14 Brian R. Bondy [:bbondy] 2011-09-20 17:29:53 PDT
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] 2011-09-29 07:31:13 PDT
Review ping
Comment 16 neil@parkwaycc.co.uk 2011-09-29 09:10:33 PDT
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] 2011-09-29 12:30:49 PDT
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] 2011-09-30 06:19:34 PDT
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/fd7827c6e05b
Comment 19 Marco Bonardo [::mak] 2011-10-01 02:21:11 PDT
https://hg.mozilla.org/mozilla-central/rev/fd7827c6e05b

Note You need to log in before you can comment on or make changes to this bug.