235395 - PresShell needs to make calling methods after Destroy() not crash

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Brian Ryner (not reading)]

mscott pointed me at a thunderbird crash on exit where nsMenuFrame is calling
PresShell::GetPrimaryFrameFor after dispatching an event that causes
PresShell::Destroy to be called.  I had recently removed the null check in
GetPrimaryFrameFor.

It seems like for the frame manager and other members that we have accessors for
that we guarantee won't be null after a successful Init(), we should make them
valid the entire lifetime of the PresShell.  This includes after Destroy() is
called (there's no way to check whether Destroy has been called, either...).

So my suggestion is just to move the actual |delete| calls from Destroy() into
the PresShell's dtor.  This keeps the pointer around and valid.  Maybe someone
else has a better idea.  Maybe instead we should just put the responsibility for
checking everything on the callers who dispatch DOM events, since that's about
the only way to destroy a pres shell without knowing it. We'd still have to
provide a way to check whether it had been shut down.  Maybe that should be part
of the return value from HandleDOMEvent...

Comment 1

[Mitch]

Attachment: Crash from todays (20040207) cvs build.

I think this is related to the issue mentioned.

It happens when shutting down (quiting) mozilla. 100% reproduceable.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm working on bug 235335 and I've resolved all the #include issues, just need
to add a few dependencies to Makefiles and I should be done.

So I favour moving the delete to the nsPresShell destructor since that's
effectively what I'll soon do anyway.

We definitely need to provide a way to check for shutdown. I think however that
it's a good idea to make method calls on a shutdown presshell do something
reasonable without crashing, insofar as we can.

Comment 3

[Brian Ryner (not reading)]

Attachment: patch

This is basically what I was thinking, I guess... I haven't tested this yet to
see if it causes leaks or crashes during teardown.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 142159 [details] [diff] [review]
patch

Looks good. This should stop the crash but I hope you get a chance to test it
before you check in :-)

Comment 5

[Brian Ryner (not reading)]

I ran this through valgrind and didn't see any leaks or other badness.

I was thinking, though, about whether we should look at deferring teardown of
the pres shell until we unwind from event dispatch.  I think that would help
prevent some of these crashes as well, and you wouldn't have to remember to hold
a reference to certain objects if you need to access them across DOM event dispatch.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe we should do that as well.

Comment 7

[Brian Ryner (not reading)]

checked in.