Potential memory leak in AsyncChannel::Echo(), ::Send()

RESOLVED FIXED in mozilla11

Status

()

Core
IPC
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nmatsakis, Assigned: nmatsakis)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
These methods take ownership of the Message* parameter, but in the case of error they do not delete it.
(Assignee)

Comment 1

6 years ago
Created attachment 570336 [details] [diff] [review]
Convert affected functions to use nsAutoptr<> and thus guarantee msg is freed
(Assignee)

Updated

6 years ago
Attachment #570336 - Flags: review?(jones.chris.g)
Comment on attachment 570336 [details] [diff] [review]
Convert affected functions to use nsAutoptr<> and thus guarantee msg is freed

The same fix is needed in RPCChannel::Call.

Please add that, then post another version of the patch and re-request review from me.  This looks good otherwise.
Attachment #570336 - Flags: review?(jones.chris.g)
(Assignee)

Comment 3

6 years ago
Created attachment 570422 [details] [diff] [review]
Add use of nsAutoPtr<> into RPCChannel::Call
Attachment #570422 - Flags: review?(jones.chris.g)
Attachment #570422 - Flags: review?(jones.chris.g) → review+

Comment 4

6 years ago
Try run for 2c4a388f862d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2c4a388f862d
Results (out of 247 total builds):
    exception: 1
    success: 226
    warnings: 16
    failure: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/nmatsakis@mozilla.com-2c4a388f862d

Comment 5

6 years ago
Try run for 4d691c3173e6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4d691c3173e6
Results (out of 247 total builds):
    exception: 1
    success: 225
    warnings: 16
    failure: 5
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/nmatsakis@mozilla.com-4d691c3173e6
For the run in comment 4, the whole enchilada, here's the breakdown.

 - https://tbpl.mozilla.org/?tree=Try&rev=2c4a388f862d is the link you want
 - the "Linux debug" M (... 2 ...) "orange" doesn't look familiar.  I requested another run using the "+" button in the bottom-middle of TBPL.
 - the red "JP" tests, jetpack, are broken so often that no one pays attention to them.  Sad state of affairs.  No harm done by your patch patches.
 - the "Win debug" M(1 ...) "orange" doesn't look familiar either, so I requested another run.

If the linux Md2 (mochitest, debug, part 2) and windows Md1 (mochitest, debug, part 1) runs come back green, I would recommend landing.  Otherwise we probably have a bug to chase down.
(Assignee)

Comment 7

6 years ago
Looks like both came back green.  I will therefore push the patch (once I figure out how).
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Attachment #570336 - Attachment is obsolete: true

Updated

6 years ago
Assignee: nobody → nmatsakis

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a01206eba24

To save time for future patches (this one is fine for now), could you set your hgrc to include the author automatically & also add a commit message, along the lines of:
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Thanks :-)
Status: NEW → ASSIGNED
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/1a01206eba24
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.