429247 - Double-clicking on empty white space in the Download Manager activates the currently selected item

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Nochum Sossonko [:Natch]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041506 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041506 Minefield/3.0pre

In the download manager the first item is always selected by default and can't be deselected in any way. This is very different from the behavior of all other windows, which deselect the item when you click on white space. What's more interesting is that in the dm when you click on white space it's exactly as if you clicked on the presently selected item. Very confusing.

Consider this scenario:
A user downloads a questionable file, but he's a careful guy and wants to check the file out before actually running it. He goes into the dm and dbl-clicks some whitespace which causes the file to run. Now, suppose he has the "Never show this again" checked off on the security dialog, or better yet suppose it's some sort of macro in a regular (non-exe or otherwise executable file which triggers the safety message) file, the user may have just uknowingly installed a virus on his computer!

I'm marking this as a security problem, as just stated in the previous scenario.

Reproducible: Always

Steps to Reproduce:
1.Download any file.
2.Open the DM.
3.Dbl-click on empty white space

Comment 1

[:Gavin Sharp [email: [e-mail]]]

This isn't a security sensitive bug report that needs to be kept hidden.

Comment 2

[Nochum Sossonko [:Natch]]

I changed the summary as I realized that previously (i.e. firefox 2) the behavior was the same in that one item is constantly selected and clicking in the white space doesn't change this, however the dbl-clicking behavior is still strange.

In reply to comment #1:

Thank you, I don't think the bug needs to be hidden, although I do still see a security implication here.

Comment 4

[Nochum Sossonko [:Natch]]

Attachment: tiny fix for this problem

Probably not absolutely necessary, but it's so small figured I'd try.

Comment 5

[Nochum Sossonko [:Natch]]

function onDownloadDblClick(aEvent)
{
  // Only do the default action for double primary clicks
  if (aEvent.button == 0 && aEvent.target.selected)
    doDefaultForSelected();
}

that's the only code i changed (maybe you can get a diff for me, i don't have permission?).

It just checks if the clicked item is selected (tried it myself, worked like a charm)

Comment 6

[Shawn Wilsher :sdwilsh]

I don't understand what you mean by "don't have permission".  We support anonymous checkout from CVS, and hg doesn't require a username to clone.

Comment 7

[Nochum Sossonko [:Natch]]

Attachment: Real patch

Sorry about the confusion, didn't do it right last time, here's the revised edition.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 321975 [details] [diff] [review]
Real patch

r=sdwilsh, but I'd love for you to get a test on this.

It's going to be similar to http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/tests/browser/browser_bug_411172.js, but you'll want to synthesize a mouse event instead of a keyboard one.  I'll be happy to help with any questions on the test too!

Comment 9

[Nochum Sossonko [:Natch]]

Attachment: browser_bug_429247 diff

Just diffed it against your file for comparison, didn't change that much.

Am I headed in the right direction, this is the first time I'm doing this, so not sure... ;)

BTW do I need sr/a for any of this?

Thanks alot for your help!

Comment 10

[Nochum Sossonko [:Natch]]

I'm not sure how I would go about checking if the function ran, the functions in EventUtils only let you check for proper event completion and would probably pass being that the event is fired & handled, so I need to check if the default action was performed.... any ideas?

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)Yes, and no.  I'd mouse stuff looks right (untested).  However, I guess I gave you a bad sample file (Edward - we really need to fix the rests of our tests).  My apologies on that.

What's wrong with that sample file is the setTimeout at the end.  It actually makes bad things happen when we run our tests through valgrind to find errors (tests time out), so we've been trying to eliminate that.  So, you'll want to copy the window opening and test function running stuff from this test file:
http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/tests/browser/browser_bug_416303.js

This won't need sr, and it's going to land in mozilla-central, so I don't think it'll need a.  I'll let you know if that changes though.

thanks for doing this!

(In reply to comment #10)
You need to check that the popup for the context menu is hidden.  I think the menupopup element has the necessary method to test this.

Comment 12

[Nochum Sossonko [:Natch]]

the menupopup doesn't show on dblclick, so i don't think it matters, so,

I changed the file to reflect the window opening and test functions in comment #11, however I had to keep alot of the structure of the previous file being that it has to check for download methods (i.e. doDefaultForSelected etc.) so it must have downloads, I hope I got that right, I was thinking of attaching a function to gPerformAllCallBack, but that wouldn't help because we're only running a command on one item! So I'm kinda stuck here unless I can redefine doDefaultForSelected and have it return true (or false/int whatever) when called... is that OK/possible?

Also should i change all the vars to lets? Is that the preferred syntax (obviously they're different, but alot of the js in the file could use it).

Comment 13

[Nochum Sossonko [:Natch]]

Attachment: Better

Here is an updated version of the bug test, no diff (what's preferred here, I can't diff a file against a non-existing one)!

This reflects alot of what was mentioned in comment #12.

Comment 14

[Shawn Wilsher :sdwilsh]

Sorry - I was thinking this was a different bug with that last comment.  We already have a test that does what you are looking for with regards to the functions:
http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/tests/browser/browser_multi_select.js

I much prefer let over var too :)

With that said, I think there are some syntax errors in your test.  Are you running it by chance?  If not, instructions on how to do so are here:
http://developer.mozilla.org/en/docs/Browser_chrome_tests#Running_individual_tests

As for diffing, you can use a utility called cvsdo to add the file to generate a proper diff.  However, if you want to keep contributing to Mozilla, you'll probably want to get on trunk development and checkout mozilla-central which uses mercurial.

Comment 15

[Nochum Sossonko [:Natch]]

Attachment: This won't seem to work!

Here's an error-free version on the testcase, but I don't think it works... i.e. I tried it and it didn't report correctly, I need some help here with this.

I stuck in my own call to initMouseEvent because I thought maybe I need to send a dblclick event which can't be done with EventUtils, but it still won't work dunno why.

About the previous patch... I tend to commit to quickly... it's a sickness.;)

(btw thanks for the previous comment, a life saver)

Comment 16

[Nochum Sossonko [:Natch]]

Attachment: Found the problem this one works

Alright this one works, though if it's not optimal I could do more work on it.

(i.e. if you want to incorporate more tests or make this one more rigorous)

Again thanks alot for all your help!

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 323038 [details] [diff] [review]
Found the problem this one works

I'll take a look at this a bit later today.

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 323038 [details] [diff] [review]
Found the problem this one works

EventUtils.synthesizeMouse should do the trick, and you'll want to pass in clickCount: 2 for aEvent (plus whatever else it'd need).  This way you don't have to manage the privileges or anything else.

Comment 19

[Nochum Sossonko [:Natch]]

(In reply to comment #18)
> (From update of attachment 323038 [details] [diff] [review])
> EventUtils.synthesizeMouse should do the trick, and you'll want to pass in
> clickCount: 2 for aEvent (plus whatever else it'd need).  This way you don't
> have to manage the privileges or anything else.

I've been trying to use that and sendMouseEvent but neither seemed to work, I keep on getting a "timed out" failure.

Here's how I tried them:

EventUtils.synthesizeMouse(downloadView, 0, 0, {type: "click", 
                           button: 0, clickCount: 2}, win);
                                     
EventUtils.sendMouseEvent({type: "click", button: 0, clickCount: 2},
                          "downloadView", win);

Comment 20

[Ed Lee :Mardak]

(In reply to comment #19)
> EventUtils.sendMouseEvent({type: "click", button: 0, clickCount: 2},
>                           "downloadView", win);
I don't think EventUtils.sendMouseEvent processes clickCount.
http://mxr.mozilla.org/seamonkey/source/testing/mochitest/tests/SimpleTest/EventUtils.js#18

> EventUtils.synthesizeMouse(downloadView, 0, 0, {type: "click", 
>                            button: 0, clickCount: 2}, win);
I don't think EventUtils.synthesizeMouse doesn't handles type: "click" either.
http://mxr.mozilla.org/seamonkey/source/dom/public/idl/base/nsIDOMWindowUtils.idl#94

Try this:

EventUtils.synthesizeMouse(downloadView, 0, 0, { clickCount: 2 }, win);

Comment 21

[Shawn Wilsher :sdwilsh]

Also, from irc:
<Enn> sdwilsh: he shouldn't be specifying the type. synthesizeMouse expects events that the native widget layer would send and it sends only 'mousedown' and 'mouseup'
<Enn> 'click' is a dom event fired as a result later on

Comment 22

[Nochum Sossonko [:Natch]]

Attachment: Now using the standard functions

Here it is with the comments addressed, only now i have to find the last download and add 75 to it so that I hit whitespace, since I can't explicitly set the target...

This one works fine.

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> Here it is with the comments addressed, only now i have to find the last
> download and add 75 to it so that I hit whitespace, since I can't explicitly
> set the target...
I'm not terribly concerned with testing more than one download selected, so you can get away with only adding one download if that helps.  I'm also slightly concerned about not having white space to hit if we have too many downloads.

Why 75 out of curiosity?

Comment 24

[Nochum Sossonko [:Natch]]

>I'm not terribly concerned with testing more than one download selected, so you
>can get away with only adding one download if that helps.  I'm also slightly
Either way I'd have to move beyond the last download and hit the whitespace so that shouldn't really be a problem.

>concerned about not having white space to hit if we have too many downloads.

>Why 75 out of curiosity?

The downloads are cleared at the beginning of the function (I think) so that shouldn't really be a concern, the only problem would be if someone would change the window size of the download manager, but that problem can happen with this method regardless, that's why the first way was somewhat more optimal as it sets the target explicitly and avoids this guessing situation.

75 I figured was for sure more than the height of the download. 

Comment 25

[Nochum Sossonko [:Natch]]

Alternatively, if I set detail to 2, would that trigger a dblclick event, the sendMouseEvent function does parse detail, and then I could use the target.

Comment 26

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> The downloads are cleared at the beginning of the function (I think) so that
> shouldn't really be a concern, the only problem would be if someone would
> change the window size of the download manager, but that problem can happen
> with this method regardless, that's why the first way was somewhat more optimal
> as it sets the target explicitly and avoids this guessing situation.
> 
> 75 I figured was for sure more than the height of the download. 
Right, I'm just trying to future proof this.  I can't see us making this smaller than one download ever, but it could get smaller.  Only having one download should be sufficient.

With that said, you can be more accurate with your height by getting the height of a download (they should all be the same height).

Comment 27

[Nochum Sossonko [:Natch]]

(In reply to comment #26)
> Right, I'm just trying to future proof this.  I can't see us making this
> smaller than one download ever, but it could get smaller.  Only having one
> download should be sufficient.
> 
> With that said, you can be more accurate with your height by getting the height
> of a download (they should all be the same height).

Alright I'll take care of this (and any other comments) Sunday.

Comment 28

[Shawn Wilsher :sdwilsh]

no rush - mozilla-central is still closed.  Enjoy your weekend, and thanks for doing this again!

Comment 29

[Nochum Sossonko [:Natch]]

Attachment: Addresses comment 27

Here is what I hope will be the final patch, I removed most of the extraneous stuff, now that I'm only inserting and checking one download.

I also changed the comment on top to reflect the correct bug.

Comment 30

[Nochum Sossonko [:Natch]]

Attachment: Just a comment fix

Same as the previous just changed the comment from offsetX to offsetY.

Comment 31

[Shawn Wilsher :sdwilsh]

Comment on attachment 323263 [details] [diff] [review]
Just a comment fix

>+ * Contributor(s):
You should add yourself to this

Can you attach a patch that has both your fix and your test in it too please?  Once I double check that everything works on my machine I'll give this r+, and it can land once mozilla-central opens.

Comment 32

[Nochum Sossonko [:Natch]]

Attachment: All inclusive final

Here is the full patch containing the fix to downloads.js and the new mochitest.

Comment 33

[Shawn Wilsher :sdwilsh]

Comment on attachment 323480 [details] [diff] [review]
All inclusive final

so I don't forget about this...

I'll be taking a look at this tomorrow :)

Comment 34

[Shawn Wilsher :sdwilsh]

Comment on attachment 323480 [details] [diff] [review]
All inclusive final

r=sdwilsh

Comment 35

[Shawn Wilsher :sdwilsh]

Attachment: for checkin

Patch was missing the necessary Makefile addition.

Comment 36

[Shawn Wilsher :sdwilsh]

Attachment: for checkin (for real)

hehe, whoops...

forgot to hg add the test file :)

Comment 37

[Nochum Sossonko [:Natch]]

I suppose you'll check it in when necessary, or should I add checkin-needed to the keywords?

Comment 38

[Shawn Wilsher :sdwilsh]

I'll get to it today or tomorrow.

Comment 39

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central

changeset: 7cf30dce13a1

Thanks for this patch!

Comment 40

[Hasham]

This checks out on OS-X, but NOT on Windows. It still acts as though the selected item was clicked. 

Comment 41

[Hasham]

Ok, this looks to be a problem. It doesn't work on either OS-X and Windows. I had a .wav file in the DL and everytime I double-click in the white space, it still opens it up.

I've tried this on FF3, 3.0.1pre and 3.1pre. Same results for all three.

Comment 42

[Shawn Wilsher :sdwilsh]

er, that seems weird.  The test would be catching that...

Comment 43

[Hasham]

Shawn, I tested with the following rheet: http://ftp.mozilla.org/pub/mozilla.org/mozilla/libraries/bonus-tracks/rheet.wav

Comment 44

[Stephen Donner [:stephend] Not actively reading bugmail]

Yeah, I see this too :-(

Comment 45

[Shawn Wilsher :sdwilsh]

WFM with url in comment 43 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1a1pre) Gecko/2008062003 Minefield/3.1a1pre

Comment 46

[Hasham]

I am on 10.5.2 and XP SP3. Both don't work.

Comment 47

[Hasham]

Hm, looks like 3.1pre is fine on both Mac and Windows. 3.0.1pre and FF3 are the problems.

Comment 48

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #47)
> Hm, looks like 3.1pre is fine on both Mac and Windows. 3.0.1pre and FF3 are the
> problems.

Same here; that's because this wasn't landed on trunk (3.0.1pre), but rather mozilla-central (3.1pre).

Comment 49

[Hasham]

Ah, ok. Is this ever going to land on trunk or should I just mark it verified for mozilla-central?

Comment 50

[Shawn Wilsher :sdwilsh]

You can verify it for mozilla-central.  If we verify it on branch, we usually use keywords.

Comment 51

[Hasham]

Cool. Thanks, Shawn.

Comment 52

[Nochum Sossonko [:Natch]]

BTW the patch was tested on trunk so if anyone wants to push it on the trunk, by all means (if you need a separate patch for that or whatever, Id be glad to do it). Just mentioning because I see the blocking-1.9.0.1? flag.

Comment 53

[Samuel Sidler (old account; do not CC)]

Shawn, do you want this in 1.9.0.x?

Comment 54

[Shawn Wilsher :sdwilsh]

(In reply to comment #53)
> Shawn, do you want this in 1.9.0.x?
It's a low risk patch with a test that fixes a minor annoyance.  I'd like to see it land, but I won't be upset if it doesn't.

Comment 55

[Samuel Sidler (old account; do not CC)]

Works for me. If this patch applies, please request approval on it, otherwise, attach one that applies.

Comment 56

[Shawn Wilsher :sdwilsh]

Comment on attachment 323597 [details] [diff] [review]
for checkin (for real)

I don't see why this wouldn't apply.

Comment 57

[Samuel Sidler (old account; do not CC)]

We're opting not to take this in a maintenance release.