114221 - view interfaces implemented on frames cause frames to be accessed after destruction

Status: NEW

(Core :: Web Painting, defect, P2)

Description

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

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.

Comment 1

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

Attachment: patch making nsIScrollPositionListener and nsICompositeListener not inherit from nsISupports

Comment 2

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

Attachment: updated patch

Comment 3

[Simon Montagu :smontagu]

Is bug 133655 the same issue?

Comment 4

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

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

Comment 5

[Simon Montagu :smontagu]

Sorry, I meant to write bug 136552.

Comment 6

[Chris Waterson]

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.

Comment 7

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

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

Comment 8

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

Attachment: attempt to solve problem by unregistration

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.

Comment 9

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

Attachment: working unregistration patch

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

Comment 10

[Chris Waterson]

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 11

[Chris Waterson]

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

sr=waterson

Comment 12

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

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 13

[Marc Attinasi]

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...

Comment 14

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

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

Comment 15

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

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.