Closed Bug 192336 Opened 17 years ago Closed 17 years ago

leak when processing key events on zombie pages

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.3final

People

(Reporter: dbaron, Assigned: dbaron)

Details

(Keywords: memory-leak, top-memory-leak, Whiteboard: [patch])

Attachments

(1 file, 1 obsolete file)

I've been noticing lately that we've been leaking a lot (the little memory
monitor in the corner of my gnome panel drops a lot when I exit Mozilla after
running for a while).  So I tried to do some leak tests on "normal browsing
situations" for me, and I found a major leak while handling key events during
page loads.

Basically, when aaronl fixed the bug about key events on zombie pages (I'm
citing this all, including the patch author, from memory), he added an early
return into a function that didn't have any early returns in the area where he
added it, and that relied on manual refcounting.  The early return prevented the
NS_RELEASE from happening.

I'll attach a patch to convert this manual refcounting to an nsCOMPtr to fix the
leak.
Attached patch patch (obsolete) — Splinter Review
This patch won't actually apply because I had to chop off some other changes at
the bottom.
Attached patch patchSplinter Review
This one should apply correctly.
Attachment #113850 - Attachment is obsolete: true
Attachment #113866 - Flags: superreview?(roc+moz)
Attachment #113866 - Flags: review?(roc+moz)
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.3beta
Comment on attachment 113866 [details] [diff] [review]
patch

nice catch
Attachment #113866 - Flags: superreview?(roc+moz)
Attachment #113866 - Flags: superreview+
Attachment #113866 - Flags: review?(roc+moz)
Attachment #113866 - Flags: review+
Comment on attachment 113866 [details] [diff] [review]
patch

This should be pretty low-risk, although there's a slight chance something
could go wrong if there's matching incorrect code (i.e., code that
over-releases) in the new codepath that aaronl added, which seems pretty
unlikely.

Then again, it also seems like if anything did go wrong, we'd find out from
talkback data, so it might also be reasonable to take this right after 1.3beta
ships.
Attachment #113866 - Flags: approval1.3b?
Comment on attachment 113866 [details] [diff] [review]
patch

Let's hold this 'till final opens. I think we have all the beta bugs wrapped up
and I'd like to get a build out the door ASAP.
Attachment #113866 - Flags: approval1.3b? → approval1.3b-
Comment on attachment 113866 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to 1.3 final.
Attachment #113866 - Flags: approval1.3? → approval1.3+
Fix checked in to trunk, 2003-02-11 12:26 PST.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.3beta → mozilla1.3final
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.