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

RESOLVED FIXED in mozilla10

Status

()

Core
Widget: Win32
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: bbondy)

Tracking

({assertion})

Trunk
mozilla10
x86
Windows 7
assertion
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → netzen
(Assignee)

Updated

6 years ago
Summary: Dragging file to Firefox triggers "ASSERTION: Expected word size" → Preferred effect is not determined properly which sometimes causes "ASSERTION: Expected word size" when dragging
(Assignee)

Comment 1

6 years ago
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.
Attachment #552927 - Flags: review?(neil)
(Assignee)

Updated

6 years ago
Depends on: 633160

Comment 2

6 years ago
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.]
(Assignee)

Comment 3

6 years ago
> 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.
(Assignee)

Comment 4

6 years ago
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.
Attachment #552927 - Attachment is obsolete: true
Attachment #552972 - Flags: review?(neil)
Attachment #552927 - Flags: review?(neil)

Comment 5

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

6 years ago
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.
(Assignee)

Comment 7

6 years ago
> 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.
(Assignee)

Comment 8

6 years ago
> 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.
(Assignee)

Comment 9

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

Comment 10

6 years ago
Created attachment 553036 [details] [diff] [review]
Patch for wrong drop preference handling v3

Changed unset effects from 0 to DROPEFFECT_NONE
Attachment #552972 - Attachment is obsolete: true
Attachment #553036 - Flags: review?(neil)
Attachment #552972 - Flags: review?(neil)
(Assignee)

Comment 11

6 years ago
Created attachment 553037 [details] [diff] [review]
Patch for wrong drop preference handling v4

Fixed alignment spacing.
Attachment #553036 - Attachment is obsolete: true
Attachment #553037 - Flags: review?(neil)
Attachment #553036 - Flags: review?(neil)
(Assignee)

Updated

6 years ago
Blocks: 679196
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a34d0baace5c
(Assignee)

Comment 14

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

Comment 15

6 years ago
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.
Attachment #553037 - Flags: review?(neil) → review+
(Assignee)

Comment 17

6 years ago
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.
(Assignee)

Comment 18

6 years ago
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/fd7827c6e05b
https://hg.mozilla.org/mozilla-central/rev/fd7827c6e05b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.