604860 - Dragging a file from the download manager to the desktop leaks

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

|<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total
 Rem      Mean       StdDev
   0 TOTAL                                          22      248  1250457       13 ( 2022.17 +/-  3244.03)  1427870
  10 ( 2257.10 +/-  4071.94)
  28 CEnumFormatEtc                                 16       16        4        1 (    1.43 +/-     0.53)        4
   1 (    1.43 +/-     0.53)
 364 nsDataObj                                      56       56        1        1 (    1.00 +/-     0.00)      396
   1 (    6.95 +/-     0.87)
 556 nsLocalFile                                    88       88     1903        1 (  156.22 +/-    87.18)     8188
   1 (  288.54 +/-   174.52)
 712 nsStringBuffer                                  8       32    34397        4 ( 7817.10 +/-  3803.06)    71661
   5 (12665.42 +/-  5268.01)
 750 nsSupportsInterfacePointerImpl                 20       20        1        1 (    1.00 +/-     0.00)       21
   1 (    1.88 +/-     0.71)
 756 nsTArray_base                                   4       16    88813        4 ( 8807.66 +/-  2783.60)        0
   0 (    0.00 +/-     0.00)
 778 nsTransferable                                 20       20        1        1 (    1.00 +/-     0.00)        7
   1 (    1.92 +/-     0.95)

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Wow, that's completely unreadable.  We're leaking an nsDataObj, 4 CEnumFormatEtcs, 1 nsLocalFile, a transferable, and a few other things.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

The issue here is that we're doing (bad) manual management of the IAsyncOperation pointer.  In particular, if the target chooses to do the operation asynchronously we leak the IAsyncOperation pointer and everything the nsDataObj owns, which consists of the CEnumFormatEtc, the nsTransferable, and then whatever arbitrary XPCOM object lives inside said nsTransferable.

While I was here, I adjusted our implementation of these methods to follow the IAsyncOperation spec and to remove the duplicate implementation on nsDataObjCollection which inherits from nsDataObj.

Comment 3

[Jim Mathies [:jimm]]

PRBool isAsyncAvailable = LL_UCMP(lShellVersion, >=, LL_INIT(5, 0));
if (isAsyncAvailable) {
..
}

Do we need this code? Couldn't we rely on the query interface call here?

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well, it's checking the shell version, so no, but shell32 hit 5.0 with Win2k so that check can be dropped anyways.

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #4)
> Well, it's checking the shell version, so no, but shell32 hit 5.0 with Win2k so
> that check can be dropped anyways.

You removed the lower block of |if (isAsyncAvailable)| code, so I think we can remove the top block as well. The query on the interface will detect if the shell supports IAsyncOperation.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 7

[Joe Drew (not getting mail)]

Comment on attachment 483827 [details] [diff] [review]
Patch

Is there any way we can add a test for this?

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Not at the moment.  It might be possible to cover it with a MozMill test eventually.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/4226f35f75f8