341301 - 1.8 branch firefox leaks like a sieve

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Trying to test my leak fixes (bug 336791) on the 1.8 branch was rather hard; I noticed lots of leaks in many cases when I was testing.  It turns out at that the following operations leak DOM windows (and thus documents as well) on Firefox 2.0 Linux nightly 2006-06-12-04-mozilla1.8:

 * Ctrl-N to open a new window
 * File | New Window to open a new window
 * Ctrl-T to open a new tab
 * File | New Tab to open a new tab
 * Alt-backarrow to go back
 * Ctrl-W to close the window
 * File | Close to close the window

Note that the following do not leak:
 * Back button to go back
 * middle-click to open a link in a new tab
 * clicking a link to load a new page in the same tab
 * clicking the [X] in the window title bar to close the window

Steps to reproduce:
mkdir deleteme
MOZ_NO_REMOTE=1 NSPR_LOG_MODULES=DOMLeak:5,DocumentLeak:5,nsDocShellLeak:5 \
  NSPR_LOG_FILE=nspr.log ./firefox -profile deleteme
<do one of above actions, and close window with [X]>
leak-gauge nspr.log

Comment 1

[Dietrich Ayala (:dietrich)]

The leak fixes that we put on the trunk for Session-restore have not been moved to the branch. Part of 337320 is to sync branch and trunk, so they're on the way.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This regressed between 2006-05-25-04-mozilla1.8 and 2006-05-26-04-mozilla1.8

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checkin window:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-05-25+02%3A00&maxdate=2006-05-26+05%3A00&cvsroot=%2Fcvsroot

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The most likely checkin looks like bug 336696.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The only other checkin in the window that affects code that I should have been executing is bug 338342, but that seems pretty unlikely as a cause.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: quick leak fix

This fixes the leak, although I have very little confidence that this is correct, since I suspect we may need the big block at the end of the function duplicated here as well.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, nsDOMEvent::DuplicatePrivateData is just return NS_OK, so I don't think failing to call it is a huge problem.  But I'm not sure if we want to call NS_MARK_EVENT_DISPATCH_DONE...

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

bryner, smaug:  Any chance you guys could figure out what the right thing to do here is?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> Actually, nsDOMEvent::DuplicatePrivateData is just return NS_OK

For what it's worth, bug 339697's patch will change that.

Comment 10

[Brian Ryner (not reading)]

Attachment: slightly more complete fix

This is for the branch, I'll also double check the trunk to make sure there's no leak.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 225371 [details] [diff] [review]
slightly more complete fix

sr=dbaron, although I'm not crazy about the shadowed variables (domEvent already, externalDOMEvent introduced by this patch).  Feel free to rename them; but if you do make sure to get all of the right occurrences and no more.

Comment 12

[Brian Ryner (not reading)]

I was mostly just mirroring some code that exists in several other places.  It's gone on the trunk, thank god.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, it's just that one of the other places is in the same function, so you're shadowing variables.

Comment 14

[Brian Ryner (not reading)]

(In reply to comment #13)
> Yeah, it's just that one of the other places is in the same function, so you're
> shadowing variables.
> 

How about if I just reuse the variables that were declared at the outer scope?  There shouldn't be any ambiguity since the inner scope always returns.

Comment 15

[Brian Ryner (not reading)]

Attachment: reuse outer-scope event variables

Comment 16

[Brian Ryner (not reading)]

checked into branch. no trunk fix needed.