Closed Bug 4127 Opened 26 years ago Closed 26 years ago

[BLOCK] MLK: We are leaking the entire nsWebShellWindow

Categories

(Core :: XUL, defect, P1)

Product:
Core
Component:
XUL
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: bruce, Assigned: scc-obsolete)

References

Details

(Whiteboard: QA BLOCKER)

Apprunner leaks the entire WebShell just by starting up and shutting down. Viewer doesn't. This causes nearly everything in apprunner to leak. I can reproduce this with both Linux and Solaris builds for some time now (today is March 21, 1999). I assume this is XP, but can't verify that for myself.
Actually, we leak nsWebShellWindow (xpfe/appshell/src/nsWebShellWindow.cpp). The destructor on that object is never called, nor is the Close() method.
Assignee: don → danm
Component: XPApps → XP Toolkit/Widgets
Re-assigned to danm@netscape.com, added others to the cc list, and changed component to XP Toolkit/Widgets. Dan, let me know if you need some help with this ...
Summary: MLK: We are leaking the entire WebShell → MLK: We are leaking the entire nsWebShellWindow
Look at http://cvs-mirror.mozilla.org/webtools/bonsai/cvsblame.cgi?file=mozilla/xpfe/app shell/src/nsAppShellService.cpp ... lines 177-207. rods@netscape.com checked in what looked like a decent version of uninitialization code which called CloseTopLevelWindow(). This being disabled looks like a strong candidate for the leak. That code didn't compile for me as it is now, but looks a whole lot better than the code in there now.
setting p2 for m4
Priority: P3 → P2
Target Milestone: M4
Fascinatingly enough ... if you close the window via the close button provided by the window manager in X, it frees this memory up! This showed that fixing this will reduce the startup/shutdown memory leak from 280k to about 70k, with the majority of the remainder outside of the XPToolkit land.
This bug is causing crashes in Editor, because the caret, whick blinks on a timer, relies on the pres shell being destroyed to stop it blinking.
I'm seeing that the webshell has a huge refCount (~298) when the window is closed. It seems that most of these come from the menus code, since each menu item appears to hold a reference to the wehshell. Chris, are the menus cleaning up after themselves when closing windows?
Oh, wait. It's the menu delegates that are holding webshell refs.
What's the status on this one? I'm getting bugs filed (e.g. 3914) that are caused by this.
Assignee: danm → saari
Reassigning to myself...
Simon, What were you using to find the leaks? I'm changing things now, hopefully improving the situation.
I exploded the addref and release macros in a few places (e.g. webshell), and just debugged with breakpoints.
Target Milestone: M4 → M5
I'm latering to M5 because of risk. I've been steadily knocking down the refcount, but it is touching a lot of files, and I'm starting to see cascading side effects. I've got the refcount down to 4 in my tree when the window closes, but after that things start going down hill fast. Fixing all of the menu references did not fix this problem.
*** Bug 4288 has been marked as a duplicate of this bug. ***
How is this coming along? The big leaks are to the point where it is nearly pointless to even bother looking at a leak report from Purify (which is why I've not been reporting leaks much for a while).
Summary: MLK: We are leaking the entire nsWebShellWindow → [BLOCK] MLK: We are leaking the entire nsWebShellWindow
What is the status here? Bruce is about to go on strike and we seriously need his work if the goal of M5 is small footprint. This bug is a blocker for Bruce (marking as such in the summary). Anyone? Anyone? Beuller?
saari, did all of your fixes make it into the tree so that the refcount is down around 4 now hopefully? I *NEED* this fixed or there can be no further meaningful memory leak detection, end of story. This drags down so much noise that any real leak is totally lost in the noise. It has been over a week and no response to my last ping (April 13), does _anyone_ care about this? It is blocking programatic window closing and the editor team from having a properly working caret. I guess I'm on strike.
I care about this, and I'll drop everything to look into it tomorrow unless someone else figures it out before then. /be
This is a top priority for me, my numero uno m5 bug, sorry I havn't got to this yet... I should be working on it tomorrow. I care quite a bit about this, I just don't have all the answers yet. The menu fixes are helpful, but you can simulate them by commenting out the menus in navigator.xul. With any luck I'll get them in tomorrow, but it involves changing the ownership model (more like creating one to being with) across all platforms in the first place. I'm cc'ing Scott Collins as he is going to assist with this.
Assignee: saari → scc
assigning to scc per our discussion in the house party meeting. chris, you're too doomed to have to worry about this one ;) ;) adding myself to the cc list since one of my bugs is caused by this (i think).
I'm not doomed, just mostly incapacitated!
So where are we today? Brendan, did you find anything? M5 _cannot_ ship with this bug, IMHO, so we need to get it nailed soon or I'm going to ask the build guys to keep the tree closed until we do.
Priority: P2 → P1
raising priority to P1
Status: NEW → ASSIGNED
Bug #3258 is probably caused by this bug as well.
Done some more investigating: We leak the webshell in viewer as well. The final refcnt of the webshell is 1. So this has nothing to do with xptoolkit.
OK, it seems like we don't know where the bug is, and we don't have an owner, so I've asked leaf and cyeh to hold the tree closed this morning until someone steps up to own it ``for real''. (No offence, scc, but pink says you're not going to be able to work on it in the near term.)
Of course, it's always possible that I'm a moron. I seem to have misunderstood pink, so if scc is really On This, then I'll just crawl back to my hole.
We also leak, in viewer, EVERY SINGLE WIDGET EVER CREATED, INCLUDING THE TOPLEVEL WIDGET. Opening a new bug for that one. Sigh....
Why would this suddenly warrant holding the tree closed? How would having someone claim 'real' ownership suddenly make the tree worth opening? Isn't setting the status to 'assigned' enough anymore?
Touche', I guess. There are other Critical, P1 bugs that have been in assigned state since, oh, January, so I guess my cynicism got the best of me. My bad, thanks to scc for taking it on, no offence intended, already back in my hole, etc., etc. All that said, we haven't had really any traction on this bug in the month+ since it was filed, and lots of people have claimed that it was really important to them. It's almost certainly not their fault that other stuff came up, but it is frustrating to see.
Some time back, pinkerton wrote: "We leak the webshell in viewer as well. The final refcnt of the webshell is 1. So this has nothing to do with xptoolkit." Mmm. In apprunner, the last release of the webshell leaves the refcount at 303. In viewer, it's 1. I'd say that that difference was significant, and until things look better in apprunner (mostly menu probs), then don't pass the buck.
It's a hard bug. There has been progress, but only incremental. SCC is on it full time as of today.
saari needs to checkin changes for menus (he has yet to do so). This will bring it back to a normal number and why we know that saari isn't the culprit here. Why he hasn't checked these in is unclear to me. Since this happens in viewer AND it happens even when there are no menus in apprunner....i stand by my previous statement. Whatever.
BTW, I'm not trying to pass the buck, but let everyone know the scope of this problem. Since it happens in viewer, it's not something we did in xptoolkit so there's no point focussing there first. Maybe I'm not being helpful with this info, if not, I'll stop commenting.
*** Bug 5483 has been marked as a duplicate of this bug. ***
Made significant progress based on advice from troy. This cleans up viewer. Still more work to do in AppRunner. Some objects seem to be still in use after destruction. Comments from beard indicate that this has been the case for some time.
Made significant progress based on advice from troy. This cleans up viewer. Still more work to do in AppRunner. Some objects seem to be still in use after destruction. Comments from beard indicate that this has been the case for some time.
Made significant progress based on advice from troy. This cleans up viewer. Still more work to do in AppRunner. Some objects seem to be still in use after destruction. Comments from beard indicate that this has been the case for some time.
Whiteboard: QA BLOCKER
scc and I just spent 10 hours on this and we're very close. A huge checkin followed. We have two small problems and everything will be solved, documented, and ready for the next person to screw up. Hopefully those can be fixed by end of the day friday.
*** Bug 5483 has been marked as a duplicate of this bug. ***
Status: ASSIGNED → RESOLVED
Closed: 26 years ago
Resolution: --- → FIXED
Fixed. WebShells are now being disposed in apprunner.
Hi Bruce, could you check this out and mark Verified if all is well? Thanks!
Status: RESOLVED → VERIFIED
This is resolved for now.
You need to log in before you can comment on or make changes to this bug.