315498 - Editor leaks an nsComposerCommandsUpdater

Status: NEW

(Core :: DOM: Editor, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Enabled cached mail compose windows
2)  Start composing a mail
3)  Quit

We leak the nsComposerCommandsUpdater, since it has a cycle with its timer.  We're never calling NotifyDocumentWillBeDestroyed() on it.  Further, my attempt to do so in nsEditingSession::TearDownEditorOnWindow failed, because mDoneSetup is false, though we've created a nsComposerCommandsUpdater...

I don't really know how this code fits together to guess at a fix for this...

Comment 1

[Simon Fraser [no longer active]]

Memory hazy.....

Tearing down the docshell should kick all this off (if it doesn't happen before then somehow), here:
http://lxr.mozilla.org/mozilla/source/docshell/base/nsDocShellEditorData.cpp#69

Does this only happen when the compose window is cached? Who's holding onto the window in that case?

Comment 2

[Boris Zbarsky [:bzbarsky]]

We do call TearDownEditorOnWindow.  It bails out without doing anything, since mDoneSetup is false on that editing session...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and I get the same leak if I disable the caching of the mailnews compose window.

Comment 4

[Simon Fraser [no longer active]]

(In reply to comment #2)
> We do call TearDownEditorOnWindow.  It bails out without doing anything, since
> mDoneSetup is false on that editing session...

So either it's getting called a second time, or PrepareForEditing() was never called (unlikely). Maybe the first call to TearDownEditorOnWindow() bails early on error?

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  So I set breakpoints in TearDownEditorOnWindow(), PrepareForEditing(), and SetupEditorOnWindow, then opened the mail compose window.  I see the following calls:

1)  TearDownEditorOnWindow called via MakeWindowEditable (this is the "tear down existing editor, if any" call), called via ComposeStartup() calling makeEditable() in the onload handler of messengercompose.xul.
2)  PrepareForEditing called from the same MakeWindowEditable call
3)  TearDownEditorOnWindow called from nsEditingSession::StartDocumentLoad, called ultimately from the same ComposeStartup() method as above -- this method loads about:blank in the editor, _after_ making it editable.  At this point the editing session's mStateMaintainer is null.
4)  SetupEditorOnWindow called from nsEditingSession::EndDocumentLoad.  This finally creates an mStateMaintainer, but our mDoneSetup is still false, since it's only set by PrepareForEditing.

Now when I close the mail compose window, I get:

5)  TearDownEditorOnWindow called from ~nsDocShellEditorData (bails early since mDoneSetup is false).

So it seems to me that the problem is that SetupEditorOnWindow doesn't set mDoneSetup to true and should...

Comment 6

[Simon Fraser [no longer active]]

mDoneSetup is rather bogus anyway. One of the tasks of ns{I}EditingSession was to handle frameset editing, where each sub-document can be made editable (hence the nsIDOMWindow* params to the various methods here).

It might be better to remove it, but we've have to be careful to avoid doing stuff like registering listeners twice. We also have to beware of charset-induced reloads etc in this code too.

Comment 7

[Kathleen :Brade]

Is MakeWindowEditable (makeEditable) called with true or false (for loading the uri)?

My gut says that there is a missing call somewhere.  This is the only documentation I could find and it is insufficient:
  http://lxr.mozilla.org/seamonkey/source/editor/docs/Editor_Embedding_Guide.html
(I thought there was more somewhere but ???).

One thing I just stumbled upon, I don't see where the gMsgEditorCreationObserver (for "obs_documentCreated") is removed. (Maybe editor.js does this or maybe it automatically gets cleaned up?)

Comment 8

[Boris Zbarsky [:bzbarsky]]

The MakeWindowEditable call is made with PR_TRUE for aDoAfterUriLoad.

Comment 9

[Jesse Ruderman]

See also bug 407259, another way to make nsComposerCommandsUpdater leak.