1494588 - Drag&drop of multiple attachments creates duplicates

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Marco De Vitis]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36

Steps to reproduce:

In TB 60.0b11 on macOS 10.13.6, upon receiving a message with multiple (4) attachments, I opened the "attachments area" below the message preview to show all attached files, then I selected them all, and finally dragged them and dropped them into a Finder window.


Actual results:

In the destination folder 16 files were created, instead of 4.
Each attachment was saved 4 times, its duplicates are named appending "-1", "-2", "-3" to the original file name.


Expected results:

Only 4 files with the original attachments names should have been created.

Comment 1

[Wayne Mery (:wsmwk)]

Does it also happen with THunderbird started in safe mode?

Comment 2

[Marco De Vitis]

Yes the same happens also in safe mode.

And for the record, it does not happen only with a specific message: it happens with any message having multiple attachments.

Comment 3

[Wayne Mery (:wsmwk)]

> In TB 60.0b11 on macOS 10.13.6

In what version did this last work?

Comment 4

[Marco De Vitis]

I'm not able to answer this question because it's just a few months since I started using TB on Mac daily.
It is surely happening since a few betas, but I do not know about older versions.

Comment 5

[Wayne Mery (:wsmwk)]

Needs love from those who know Mac

Comment 6

[:Dossy Shiobara]

Can confirm this is reproducible for me in TB 60.2.1 on OSX 10.13.6.

Comment 7

[Wayne Mery (:wsmwk)]

Thanks. Perhaps then this is a regression / worked in version 52. 

Not sure which component to put this.

Comment 8

[Jorg K (CEST = GMT+2)]

Works OK on Windows. Hmm, you could look at this for us? Is Markus still active ([e-mail])? Arshad is a Mac person, but he's got other things to do.

Alice, do you also find regressions on Mac?

Comment 9

[Wayne Mery (:wsmwk)]

(I won't have time to look at this, plus I don't have a Mac)

Comment 10

[Alice0775 White]

I don't have a Mac too...

Comment 11

[Magnus Melin [:mkmelin]]

Didn't somebody report it's not working at all on mac anymore?
xref bug 270292 to find the related code for debugging.

Comment 12

[Nomis101]

I've tested this issue with different versions of Thunderbird on macOS Mojave. Seems the issue was introduced with TB 53.

60.0 - issue
58.0b3 - issue
57.0b2 - issue
56.0b4 - issue
55.0b2 - issue
54.0b3 - issue
53.0b2 - issue
52.9.1 - no issue, working

Comment 13

[Jorg K (CEST = GMT+2)]

Well, that broke ages ago and no beta user ever complained :-(((

Markus, do you still help out in TB with Mac issues? Also NI'ing a few other people known to have Macs.

Comment 14

[Tim Nguyen :ntim]

I can reproduce this, seems like you get n^2 files for n attachments you drag.

Comment 16

[Tim Nguyen :ntim]

I tried to find a regression range using `mozregression --app thunderbird`, but all the builds around 50-52 crash on MacOS.

Comment 17

[Alice0775 White]

(In reply to Alice0775 White from comment #15)
> I can reproduce the issue on Tb60.2.1 Linux Mint.

sorry, this is wrong. pls ignore the comment#15

Comment 18

[:Dossy Shiobara]

(In reply to Nomis101 from comment #12)
> I've tested this issue with different versions of Thunderbird on macOS
> Mojave. Seems the issue was introduced with TB 53.
...
> 53.0b2 - issue
> 52.9.1 - no issue, working

Is there an easy way to get a diff between 52.9.1 and 53.0b2?  I was hoping to be able to compare two versions in DXR but my DXR-fu is weak.  :-/

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #16)
> ... but all the builds around 50-52 crash on MacOS.
If comment #12 is correct, it would be best to look at thhe mozilla53 cycle (2016-11-14 - 2017-01-23).

To my knowledge, there is no DXR diff, you colud only look at there changelog of the respective Mac files that handle drag&drop, like this one:
https://hg.mozilla.org/mozilla-central/log/tip/widget/cocoa/nsDragService.mm

Comment 21

[gsh474246]

Working with MAC High Sierra. This error started with Thunderbird 60.xxx. Before copying multiple attachments to a folder did not cause duplicates.

Comment 22

[Jorg K (CEST = GMT+2)]

Alex, you have a Mac, can you please check this.

Comment 23

[Alessandro Castellani [:aleca]]

I can confirm the issue.
I tested this on both v60.7.2 and trunk on macos Mojave.

Dragging and dropping 3 attachments at once, with 3 difference sizes and file types, resulted in a total of 9 attachments downloaded on the desktop. So it seems the number of times a single file is downloaded is equal to the number of attachments selected.

I see if I have some capacity this week since I'm fixing a couple of regressions in the acctachment list area.

Comment 24

[oliver]

I can also confirm this on MacOS 10.14.6 (Mojave) and Thunderbird 68.4.1 and 68.4.2. If you drag multiple attachment files to a Finder window it will always create n^2 files instead of n.

Can someone please fix this, this is really annoying.

Comment 25

[Jorg K (CEST = GMT+2)]

Magnus?

Comment 26

[Magnus Melin [:mkmelin]]

Khushil is working on dnd currently for bug 1226362. Please have a look at this at the same time.

Comment 27

[Khushil Mistry [:khushil324]]

Attachment: Bug-1494588_dnd-of-attachments-creates-duplicates-osx-0.patch

This is kind of a hack. This issue is coming from Mozilla Central.
This line should get called n times instead of n^2 times where n is the total number of attachments: https://searchfox.org/mozilla-central/source/widget/nsTransferable.cpp#248,256
Anyone has any idea?

Comment 28

[Jorg K (CEST = GMT+2)]

Where do the n^2 calls come from? I don't see a loop in the M-C code. Can you attach the debugger to it?

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 9130079 [details] [diff] [review]
Bug-1494588_dnd-of-attachments-creates-duplicates-osx-0.patch

Review of attachment 9130079 [details] [diff] [review]:
-----------------------------------------------------------------

I think we need to find the cause and fix that instead.
I don't understand what is called more than N times. 

What does it output if you add printfs for each of the functions you mark, like
printf("xxx nsTransferable::GetTransferData aFlavor=%s\n", aFlavor.get()); and similar for the other two

Comment 30

[Khushil Mistry [:khushil324]]

Here is the flow of the whole event:

1. When attachments get dragged, setupDataTransfer is called to add the data in event.dataTransfer: https://searchfox.org/comm-central/source/mail/base/content/mailCore.js#841, https://searchfox.org/comm-central/source/mail/base/content/msgHdrView.js#3392

2. nsFlavorDataProvider object is saved as "application/x-moz-file-promise" event.dataTransfer. nsFlavorDataProvider interface has getFlavorData method. When you drop any item in the local machine, this should get the call(only once) and it will save the file into the local machine with the help of the saved data in event.dataTransfer: https://searchfox.org/comm-central/source/mail/base/content/mailCore.js#841,891,903,940

3. nsFlavorDataProvider interface is coming from here: https://searchfox.org/mozilla-central/source/widget/nsITransferable.idl#80

4. I checked with all the nsFlavorDataProvider instances. When you drop a file in the local machine, nsTransferable::GetTransferData is getting called and we have call to dataProvider->GetFlavorData(this, aFlavor, getter_AddRefs(dataBytes)); there: https://searchfox.org/mozilla-central/source/widget/nsTransferable.cpp#248,253

5. I used printf statements to debug the code in nsTransferable::GetTransferData just like printf("xxx nsTransferable::GetTransferData aFlavor=%s\n", aFlavor.get()); It was getting printed n^2 times always with aFlavor.get() is equal to application/x-moz-file-promise. I think once it's done, it should not pass the if statemenet here: https://searchfox.org/mozilla-central/source/widget/nsTransferable.cpp#253. But I don't know what does this if statement do/check.

Comment 31

[Khushil Mistry [:khushil324]]

Attachment: Bug-1494588_dnd-multiple-items-mac-0.patch

It's working on MacOS. Can you check if it's not breaking anything on Linux?

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 9160378 [details] [diff] [review]
Bug-1494588_dnd-multiple-items-mac-0.patch

Review of attachment 9160378 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mailCore.js
@@ +949,4 @@
>          );
>          aData.value = destFilePath.QueryInterface(Ci.nsISupports);
>        }
> +      aTransferable.removeDataFlavor("application/x-moz-file-promise");

Seems like a hack.But maybe we could still do it as
```
if (AppConstants.platform == "macosx") {
  // Workaround dnd of multiple attachments creating duplicates. See bug 1494588.
  aTransferable.removeDataFlavor("application/x-moz-file-promise");
}
```

Comment 33

[Khushil Mistry [:khushil324]]

Attachment: Bug-1494588_dnd-multiple-items-mac-1.patch

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4b7f17978f58
Fix drag & drop of multiple attachments creating duplicates in MacOS. r=mkmelin

Comment 35

[Jorg K (CEST = GMT+2)]

Please don't forget uplift here.

Comment 36

[Magnus Melin [:mkmelin]]

Comment on attachment 9160599 [details] [diff] [review]
Bug-1494588_dnd-multiple-items-mac-1.patch

[Approval Request Comment]
User impact if declined: per summary
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): Quite contained fix, so not very risky. Though it's not well understood why it happens.

Comment 37

[Wayne Mery (:wsmwk)]

Comment on attachment 9160599 [details] [diff] [review]
Bug-1494588_dnd-multiple-items-mac-1.patch

Approved for beta

Eckard, can you test this when we smoketest beta?

Comment 38

[Wayne Mery (:wsmwk)]

Comment on attachment 9160599 [details] [diff] [review]
Bug-1494588_dnd-multiple-items-mac-1.patch

Approved for esr78

Comment 39

[Rob Lemley [:rjl]]

Thunderbird 79.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/ef7205c5cdd2

Comment 40

[Rob Lemley [:rjl]]

Thunderbird 78.0.1:
https://hg.mozilla.org/releases/comm-esr78/rev/4d03a662e43f

Comment 41

[Eckard Berberich]

(In reply to Wayne Mery (:wsmwk) from comment #37)

Comment on attachment 9160599 [details] [diff] [review]
Bug-1494588_dnd-multiple-items-mac-1.patch

Approved for beta

Eckard, can you test this when we smoketest beta?

I can't reproduce this issue in TB 79.0b3.