Closed
Bug 341301
Opened 19 years ago
Closed 19 years ago
1.8 branch firefox leaks like a sieve
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Assigned: bryner)
References
Details
(Keywords: fixed1.8.1, memory-leak)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
dbaron
:
superreview+
dbaron
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•19 years ago
|
Flags: blocking1.8.1+
Reporter | ||
Updated•19 years ago
|
Summary: (1.8 branch) 1.8 branch firefox leaks like a sieve → 1.8 branch firefox leaks like a sieve
Comment 1•19 years ago
|
||
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.
Reporter | ||
Comment 2•19 years ago
|
||
This regressed between 2006-05-25-04-mozilla1.8 and 2006-05-26-04-mozilla1.8
Reporter | ||
Comment 3•19 years ago
|
||
Reporter | ||
Comment 4•19 years ago
|
||
The most likely checkin looks like bug 336696.
Reporter | ||
Comment 5•19 years ago
|
||
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.
Reporter | ||
Comment 6•19 years ago
|
||
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.
Reporter | ||
Comment 7•19 years ago
|
||
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...
Reporter | ||
Comment 8•19 years ago
|
||
bryner, smaug: Any chance you guys could figure out what the right thing to do here is?
Reporter | ||
Updated•19 years ago
|
Attachment #225334 -
Flags: superreview?(bryner)
Attachment #225334 -
Flags: review?(Olli.Pettay)
Reporter | ||
Updated•19 years ago
|
Assignee: nobody → events
Component: Layout → DOM: Events
QA Contact: layout → ian
Comment 9•19 years ago
|
||
(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.
Assignee | ||
Comment 10•19 years ago
|
||
This is for the branch, I'll also double check the trunk to make sure there's no leak.
Assignee: events → bryner
Attachment #225334 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #225371 -
Flags: superreview?
Attachment #225371 -
Flags: review?
Attachment #225334 -
Flags: superreview?(bryner)
Attachment #225334 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•19 years ago
|
Attachment #225371 -
Flags: superreview?(dbaron)
Attachment #225371 -
Flags: superreview?
Attachment #225371 -
Flags: review?(Olli.Pettay)
Attachment #225371 -
Flags: review?
Reporter | ||
Comment 11•19 years ago
|
||
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.
Attachment #225371 -
Flags: superreview?(dbaron) → superreview+
Assignee | ||
Comment 12•19 years ago
|
||
I was mostly just mirroring some code that exists in several other places. It's gone on the trunk, thank god.
Reporter | ||
Comment 13•19 years ago
|
||
Yeah, it's just that one of the other places is in the same function, so you're shadowing variables.
Updated•19 years ago
|
Attachment #225371 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 14•19 years ago
|
||
(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.
Assignee | ||
Comment 15•19 years ago
|
||
Attachment #225371 -
Attachment is obsolete: true
Attachment #225456 -
Flags: superreview?(dbaron)
Attachment #225456 -
Flags: approval-branch-1.8.1?(dbaron)
Reporter | ||
Updated•19 years ago
|
Attachment #225456 -
Flags: superreview?(dbaron)
Attachment #225456 -
Flags: superreview+
Attachment #225456 -
Flags: approval-branch-1.8.1?(dbaron)
Attachment #225456 -
Flags: approval-branch-1.8.1+
Assignee | ||
Comment 16•19 years ago
|
||
checked into branch. no trunk fix needed.
You need to log in
before you can comment on or make changes to this bug.
Description
•