message compose calls TearDownEditorWindow too many times

ASSIGNED
Assigned to

Status

()

P2
normal
ASSIGNED
14 years ago
9 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({memory-leak})

Trunk
Future
x86
Linux
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(1 attachment)

Not sure whose bug this is (app bug or composer core bug) -- I may look into it
a bit more to try to distinguish, but I wanted to get a bug on file about it. 
Following the steps to reproduce for bug 283374, I see
nsEditingSession::TearDownEditorWindow called three times:
 1) before nsEditingSession::PrepareForEditing is called (so it does nothing)
 2) right after PrepareForEditing is called (so mDoneSetup becomes false)
 3) when the compose window is closed (it does nothing, since mDoneSetup is false)

Since the last one does nothing (since it remembers it's already been called),
the steps in bug 283374 cause a leak of an nsTimerImpl and an
nsComposerCommandsUpdater since
nsComposerCommandsUpdater::NotifyDocumentWillBeDestroyed doesn't get called to
null out mUpdateTimer.
The problem seems to be in nsEditingSession:  the first call is from
nsEditingSession::MakeWindowEditable, and the second call is from
nsEditingSession::StartDocumentLoad , which seems to duplicate what the first
was supposed to do, and too late.
Assignee: mscott → mozeditor
Component: Message Compose Window → Editor
Product: Thunderbird → Core
QA Contact: bugzilla
Version: unspecified → Trunk
The later call is older (revision 1.2); I think it should just be removed, it
duplicates the earlier call added later (revision 1.6).
Created attachment 178645 [details] [diff] [review]
patch
Assignee: mozeditor → dbaron
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8beta2
Attachment #178645 - Flags: superreview?(roc)
Attachment #178645 - Flags: review?(neil.parkwaycc.co.uk)

Comment 4

14 years ago
Comment on attachment 178645 [details] [diff] [review]
patch

Sorry, I don't know this stuff.
Attachment #178645 - Flags: review?(neil.parkwaycc.co.uk) → review?(brade)
Attachment #178645 - Flags: superreview?(roc) → superreview+
Attachment #178645 - Flags: review?(brade) → review?(sfraser_bugs)

Comment 5

14 years ago
Does this break editing pages in composer which do a charset-related reload?
Nope, that works fine.  I saved a page with Japanese characters in both UTF-8
and ISO-2022-JP (as distinguished by a <meta http-equiv="content-type"
content="text/html; charset=...">), and both versions loaded fine in the editor.
 (That's what you were asking, right?)

Comment 7

14 years ago
Does this patch work when opening/closing a frameset?
Does this patch work with Midas?

jst may or may not have changes that are in this same vicinity so one of you may
see conflicts.
http://www.mozilla.org/editor/midasdemo/ still works fine.

What do you mean by "opening/closing a frameset"?

Comment 9

14 years ago
There are many documents that contain frames / iframes.  I used to always load
certain pages on rottentomatoes.com to test against frames.  I'm not sure if
they still use frames or not.  MSDN still uses frames on some pages such as:
 
http://msdn.microsoft.com/library/default.asp?url=/workshop/author/dhtml/reference/objects.asp

I would make sure that you can edit each frame.

I'm actually not positive that this works in recent builds (has anyone tested it
lately? Daniel?).

Comment 11

13 years ago
Comment on attachment 178645 [details] [diff] [review]
patch

Deny review pending more testing.
Attachment #178645 - Flags: review?(sfraser_bugs) → review-
Target Milestone: mozilla1.8beta2 → Future
I wonder if this does not impact Midas when you move from an editable page to a
non-editable one through a link. Hmmmm.
QA Contact: bugzilla → editor
You need to log in before you can comment on or make changes to this bug.