287766 - message compose calls TearDownEditorWindow too many times

Status: NEW

(Core :: DOM: Editor, defect, P3)

Description

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

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.

Comment 1

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

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.

Comment 2

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

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

Comment 3

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

Attachment: patch

Comment 4

[[e-mail]]

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

Sorry, I don't know this stuff.

Comment 5

[Simon Fraser [no longer active]]

Does this break editing pages in composer which do a charset-related reload?

Comment 6

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

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

[Kathleen :Brade]

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.

Comment 8

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

http://www.mozilla.org/editor/midasdemo/ still works fine.

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

Comment 9

[Kathleen :Brade]

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 10

[Kathleen :Brade]

Also, try editing:  http://www.mozilla.org/editor/midasdemo/

Comment 11

[Simon Fraser [no longer active]]

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

Deny review pending more testing.

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I wonder if this does not impact Midas when you move from an editable page to a
non-editable one through a link. Hmmmm.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information