I've been looking into the cases where frames are accessed after they are freed to the pres shell arena (see bug 114219, which I started to try and debug another problem I thought was related to accessing freed data, but wasn't). One of the two things I've found that does this is releases of frames implementing nsIScrollPositionListener. There shouldn't really be a need to release frames. There are a number of ways to solve this -- I'll attach a patch that does one of them, which is to disinherit nsIScrollPositionListener and nsICompositeListener (which got dragged in because the only non-frame implementation of nsIScrollPositionListener is the only implementation of nsICompositeListener) from nsISupports. If this is a good approach the "nsI" shouuld probably change to "nsA". Better unregistration of the listeners would be another approach.

Is bug 133655 the same issue?

smontagu: What makes you think that? I think you commented on the wrong bug.

Sorry, I meant to write bug 136552.

I'm not very familiar with this area of the code, so forgive some dumb questions: 1. Are frames the _only_ scroll position listeners that we have? (If not, are there others that need reference counting to maintain the ownership model.) 2. While this seems reasonable, does it in fact fix anything? Or is it just cleanup? I think that changing the listeners to be un-owned is fine, assuming there are no other listeners that need to be kept alive by the view.

The only listener that wasn't a frame is the one in the pres shell that is changed in the above patch.

This is an attempt to solve the problem by unregistration, but it crashes within CanvasFrame::Destroy because the view manager is no longer connected to the pres shell, so we dereference a null pointer. I wonder if it's possible to get the view manager some other way, or whether, instead, the mPresContext should be an mViewManager.

This is a working patch doing unregistration instead. Could this cause the lifetime of the view manager to be extended? Would that be bad?

Presumably this _does_ extend the lifetime of the view manager. (Otherwise, couldn't you just get it from the pres context when you needed to unregister?) It seems sort of fortuitous the way we stagger from the view manage to the pres context in one spot, but it appears to be #ifdef DEBUG_CANVAS_FOCUS, so it's probably not a big deal.

Comment on attachment 82645 [details] [diff] [review] working unregistration patch sr=waterson

I think it actually doesn't extend the life of the VM in the normal case, since the doc viewer holds the view manager past the |Destroy| call on the pres shell (in fact, I think all the way until its own destructor), but the pres shell nulls out its own *weak* pointer to the view manager at the beginning of its |Destroy| method, and then destroys the frames. That said, I could move the nulling of the view manager pointer to later in PresShell::Destroy to work around the problem...

Comment on attachment 82645 [details] [diff] [review] working unregistration patch r=attinasi, but it seems a little presumptuous to assume that GetViewManager will always succeed in nsHTMLFrame - if that fails the result code shoudl be returned from Init, though you might want to check it in Destroy as well (or just assert that it is not null). Also, can nsGfxScrollFrame have a null mInner in Destroy? I guess the constructor would crash if the allocation failed anyway...

*** Bug 130154 has been marked as a duplicate of this bug. ***

Comment on attachment 82645 [details] [diff] [review] working unregistration patch This patch was checked in 2002-07-08 21:30 PDT, so the main problem is fixed. However, I think I still want to get the other patch in too.