Dragging a file from the download manager to the desktop leaks

RESOLVED FIXED in mozilla2.0b7

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

Trunk
mozilla2.0b7
x86
Windows 7
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

|<----------------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)
Wow, that's completely unreadable.  We're leaking an nsDataObj, 4 CEnumFormatEtcs, 1 nsLocalFile, a transferable, and a few other things.
Created attachment 483717 [details] [diff] [review]
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.
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Attachment #483717 - Flags: review?(jmathies)
Attachment #483717 - Flags: approval2.0?

Comment 3

8 years ago
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?
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

8 years ago
(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.
Created attachment 483827 [details] [diff] [review]
Patch
Attachment #483717 - Attachment is obsolete: true
Attachment #483827 - Flags: review?(jmathies)
Attachment #483717 - Flags: review?(jmathies)
Attachment #483717 - Flags: approval2.0?

Updated

8 years ago
Attachment #483827 - Flags: review?(jmathies) → review+
Comment on attachment 483827 [details] [diff] [review]
Patch

Is there any way we can add a test for this?
Attachment #483827 - Flags: approval2.0? → approval2.0+
Not at the moment.  It might be possible to cover it with a MozMill test eventually.
Flags: in-testsuite?
http://hg.mozilla.org/mozilla-central/rev/4226f35f75f8
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Version: unspecified → Trunk
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.