1246517 - Removing the dictionary removal observer doesn't work when switching from plaintext to HTML e-mail or vice versa.

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

In bug 1203957 and bug 1235205 I implemented an observer that watches "spellcheck-dictionary-remove".

The removal of this observer fails at times, as can be see here for example:
http://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-linux64/1454868000/comm-central_ubuntu64_vm_test-mozmill-bm53-tests1-linux64-build6.txt.gz
or here
http://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1454868000/comm-central_win7-ix_test-mozmill-bm109-tests1-windows-build12.txt.gz

We see this on the console:
10:30:51     INFO -  TEST-START | /builds/slave/test/build/tests/mozmill/composition/test-charset-upgrade.js | test_encoding_upgrade_plaintext_compose
10:30:51     INFO -  JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 2529: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]
10:30:58     INFO -  TEST-PASS | /builds/slave/test/build/tests/mozmill/composition/test-charset-upgrade.js | test-charset-upgrade.js::test_encoding_upgrade_plaintext_compose
10:30:58     INFO -  TEST-START | /builds/slave/test/build/tests/mozmill/composition/test-charset-upgrade.js | teardownModule

The failure seems to appear pretty reliably during this test.

Aceman has a different recipe for reproducing this, see bug 1235205 comment #18.

Comment 1

[Jorg K (CEST = GMT+2)]

There seems to be a timing issue. I added some dumps, and this is what I see:
dictionaryRemovalObserver is being added
Remove dictionaryRemovalObserver at recycling time
dictionaryRemovalObserver is being added
Remove dictionaryRemovalObserver at session shutdown time
Remove dictionaryRemovalObserver at recycling time

So clearly things are getting mixed up here.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: Added some debug and a try/catch

I don't have a good idea how to solve this. Since the error seems to happen at shutdown in ComposeUnload() I put a try/catch there. That makes the error go away, but I'm sure it won't satisfy the purists ;-)

Of course we could toss the current implementation and have one global observer that handles add composition windows instead of many, one for each window.

I've already discussed this option in bug 1235205 comment #4, under "Forth".

In bug 1235205 comment #5 Neil suggested to remove the observer in ComposeUnload(), so this is what I've done. But maybe another approach would avoid the problem we are seeing here.

Comment 3

[:aceman]

What I see is this:
=================== dictionaryRemovalObserver is being added
=================== Remove dictionaryRemovalObserver at ComposeUnload/shutdown
=================== Remove dictionaryRemovalObserver at ComposeUnload/shutdown FAILED!!
=================== Remove dictionaryRemovalObserver at recycling time

But then I do not understand the error. Why does the first removal attempt fail (the new code)? I would understand if the second (at recycling) failed as that one does not check if the observer is still there.

And why are we running recycling code when compose is unloaded? It does not run when recycling is disabled (recycled_windows=0).

To debug this I added Services.obs.enumerateObservers("spellcheck-dictionary-remove").getNext() at both .removeObserver() sites and this is what I get:

Error: at unload = [xpconnect wrapped nsISupports @ 0xd7e31d60 (native @ 0xddecfd00)]
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2538

Error: at recycle = [xpconnect wrapped nsISupports @ 0xd7ee2c70 (native @ 0xddecfd00)]
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 205

Now, is that the same object or not?

Comment 4

[:aceman]

Now when I .removeObserver(Services.obs.enumerateObservers("spellcheck-dictionary-remove").getNext()) instead of DictionaryRemovalObserver, then at the recycling step I get:
Error: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsISimpleEnumerator.getNext]
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 204

That actually looks like the expected behaviour. There is nothing more in the observer enumeration so .getNext fails here, at the second removal.

So maybe in the ComposeUnload function, the observer in the enumeration is not the same object as 
DictionaryRemovalObserver so the removal fails.

Could it be that we add DictionaryRemovalObserver object twice to the observers? The dumps do not indicate that. Or that DictionaryRemovalObserver address changes during run/recycling of the windows?

Maybe you could try the pattern at
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserver, instantiating the object with 'new' ?

Comment 5

[Jorg K (CEST = GMT+2)]

Thanks for looking at this.

(In reply to :aceman from comment #3)
> What I see is this:
> =================== dictionaryRemovalObserver is being added
> =================== Remove dictionaryRemovalObserver at
> ComposeUnload/shutdown
> =================== Remove dictionaryRemovalObserver at
> ComposeUnload/shutdown FAILED!!
> =================== Remove dictionaryRemovalObserver at recycling time
Yes. Me too.

> But then I do not understand the error. Why does the first removal attempt
> fail (the new code)? I would understand if the second (at recycling) failed
> as that one does not check if the observer is still there.
I don't understand this either.

> And why are we running recycling code when compose is unloaded?
That doesn't make sense to me either.
The normal cause of events with recycled_windows=1 is:
Compose window opens.
Compose window closes (on send, save draft, etc.). Recycler runs.
Compose window opens.
Compose window closes (on send, save draft, etc.). Recycler runs.
etc.
Session shutdown: Unload runs. Thus the test not to remove the observer twice.

Or, when concurrently composing two messages:
Compose window 1 opens.
Compose window 2 opens.
Compose window 1 closes (on send, save draft, etc.). Recycler runs.
Compose window 2 closes (on send, save draft, etc.). Unload runs. The window is destroyed.
Session shutdown: Unload for windows 1 runs. Thus the test not to remove it twice.

> To debug this I added
> Services.obs.enumerateObservers("spellcheck-dictionary-remove").getNext() at
> both .removeObserver() sites and this is what I get:
OK, you get the first element of the nsISimpleEnumerator. 

> Error: at unload = [xpconnect wrapped nsISupports @ 0xd7e31d60 (native @
> 0xddecfd00)]
> Source File:
> chrome://messenger/content/messengercompose/MsgComposeCommands.js
> Line: 2538
What to you do to get "Error: ..." and "Line: ..."? A dump() of some sort?
Maybe paste the debug line here.

> Error: at recycle = [xpconnect wrapped nsISupports @ 0xd7ee2c70 (native @
> 0xddecfd00)]
> Source File:
> chrome://messenger/content/messengercompose/MsgComposeCommands.js
> Line: 205
> Now, is that the same object or not?
No idea. And if not, where does the second one come from?

I think we need more debugging, so the debug needs to be added also straight after the observer got added so we know what got added.

Comment 6

[Jorg K (CEST = GMT+2)]

Sorry, wires crossed a bit here.

(In reply to :aceman from comment #4)
> Could it be that we add DictionaryRemovalObserver object twice to the
> observers?
Sure, it gets added once for every composition window opened.

> Maybe you could try the pattern at
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIObserver, instantiating the object with 'new' ?
I've considered that. I basically copied stuff we do for messageComposeOfflineQuitObserver.

Comment 7

[:aceman]

(In reply to Jorg K (GMT+1) from comment #5)
> > Error: at unload = [xpconnect wrapped nsISupports @ 0xd7e31d60 (native @
> > 0xddecfd00)]
> > Source File:
> > chrome://messenger/content/messengercompose/MsgComposeCommands.js
> > Line: 2538
> What to you do to get "Error: ..." and "Line: ..."? A dump() of some sort?
> Maybe paste the debug line here.
Yes, the "Error:" does not mean anything here, I just used Components.utils.reportError() instead of dump(). What is relevant here is the address of the object I got.

> > Error: at recycle = [xpconnect wrapped nsISupports @ 0xd7ee2c70 (native @
> > 0xddecfd00)]
> > Source File:
> > chrome://messenger/content/messengercompose/MsgComposeCommands.js
> > Line: 205
> > Now, is that the same object or not?
> No idea. And if not, where does the second one come from?
> 
> I think we need more debugging, so the debug needs to be added also straight
> after the observer got added so we know what got added.

Yes. As said in comment 4, if at both callsites I checked if there is actually is any observer and then removed the observer that I got from enumerateObservers.getNext(), then the code would pass without errors. It is just a mystery why the item (.getNext()) that is in the list of observers does not appear to be identical to dictionaryRemovalObserver.

Can we somehow debug when dictionaryRemovalObserver gets initialized?

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: Some more sophisticated debug

OK, I improved the debug.

The following shows a run where two composition windows are opened and closed:
First window gets opened, no observers to start with, one is added.
===== reportObserver - Start
===== >> dictionaryRemovalObserver is being added (before)
===== reportObserver - End
===== reportObserver - Start
===== >> dictionaryRemovalObserver is being added (after)
===== >> [xpconnect wrapped nsISupports @ 0xb6373d0 (native @ 0xc069280)]
===== reportObserver - End

Second window gets opened, one observer to start with, one is added, two at the end
===== reportObserver - Start
===== >> dictionaryRemovalObserver is being added (before)
===== >> [xpconnect wrapped nsISupports @ 0x116fa6a0 (native @ 0xc069280)]
===== reportObserver - End
===== reportObserver - Start
===== >> dictionaryRemovalObserver is being added (after)
===== >> [xpconnect wrapped nsISupports @ 0x116fa700 (native @ 0xceb8da0)]
===== >> [xpconnect wrapped nsISupports @ 0x116fa6a0 (native @ 0xc069280)]
===== reportObserver - End

Second window is closed and recycled. We start with the two observers we had, one
is removed leaving one, the one that was added first terminating in "80".
===== reportObserver - Start
===== >> Remove dictionaryRemovalObserver at recycling time (before)
===== >> [xpconnect wrapped nsISupports @ 0x116fa700 (native @ 0xceb8da0)]
===== >> [xpconnect wrapped nsISupports @ 0x116fa6a0 (native @ 0xc069280)]
===== reportObserver - End
===== reportObserver - Start
===== >> Remove dictionaryRemovalObserver at recycling time (after)
===== >> [xpconnect wrapped nsISupports @ 0x116fa6a0 (native @ 0xc069280)]
===== reportObserver - End

Closing the second window: The original observer gets removed leaving none.
===== reportObserver - Start
===== >> Remove dictionaryRemovalObserver at ComposeUnload/shutdown (before)
===== >> [xpconnect wrapped nsISupports @ 0xb6373d0 (native @ 0xc069280)]
===== reportObserver - End
===== reportObserver - Start
===== >> Remove dictionaryRemovalObserver at ComposeUnload/shutdown (after)
===== reportObserver - End

This is all good. If I close the first window first, then the observer added first
is removed first.

In the next comment I will post what happens in the test.

Comment 9

[Jorg K (CEST = GMT+2)]

Here is the result from running
mozmake SOLO_TEST=composition/test-charset-upgrade.js mozmill-one

Windows opens:
===== reportObserver - Start
===== >> dictionaryRemovalObserver is being added (before)
===== reportObserver - End
===== reportObserver - Start
===== >> dictionaryRemovalObserver is being added (after)
===== >> [xpconnect wrapped nsISupports @ 0xaef9670 (native @ 0xd67d9e0)]
===== reportObserver - End

Window closes:
===== reportObserver - Start
===== >> Remove dictionaryRemovalObserver at recycling time (before)
===== >> [xpconnect wrapped nsISupports @ 0x1088d0a0 (native @ 0xd67d9e0)]
===== reportObserver - End
===== reportObserver - Start
===== >> Remove dictionaryRemovalObserver at recycling time (after)
===== reportObserver - End

Another window opens:
===== reportObserver - Start
===== >> dictionaryRemovalObserver is being added (before)
===== reportObserver - End
===== reportObserver - Start
===== >> dictionaryRemovalObserver is being added (after)
===== >> [xpconnect wrapped nsISupports @ 0x172afd90 (native @ 0x1d5c6350)]
===== reportObserver - End

What's going on here?? Why ComposeUnload??
This call makes no sense.
===== reportObserver - Start
===== >> Remove dictionaryRemovalObserver at ComposeUnload/shutdown (before)
===== >> [xpconnect wrapped nsISupports @ 0x1c376e80 (native @ 0x1d5c6350)]
===== reportObserver - End
=================== Remove dictionaryRemovalObserver at ComposeUnload/shutdown FAILED!!

Window closes:
===== reportObserver - Start
===== >> Remove dictionaryRemovalObserver at recycling time (before)
===== >> [xpconnect wrapped nsISupports @ 0x172afd90 (native @ 0x1d5c6350)]
===== reportObserver - End
===== reportObserver - Start
===== >> Remove dictionaryRemovalObserver at recycling time (after)
===== reportObserver - End

Session shuts down:
Another call to ComposeUnload. I added another dump not included in the patch to see it. This compose unload *is* expected, since it unloads (finally destroys) the recycled window.

As far as I can see, the addition and removal of the observer works as intended.
If recycling is used and only one window is ever used, as in the test, then the observer is added and removed at recycle time.

What does indeed not make any sense at all is the ComposeUnload call before the last recycle. I will add a JS stack dump to see where this is coming from.

Comment 10

[Jorg K (CEST = GMT+2)]

OK, a "normal" ComposeUnload call when a second window which is not recycled is closed (see example in comment #8) looks like this:
===== ComposeUnload, JS stack dump:
===== JS> chrome://messenger/content/messengercompose/MsgComposeCommands.js (2541) - ComposeUnload
===== JS> chrome://messenger/content/messengercompose/messengercompose.xul (1)
===== JS> chrome://messenger/content/messengercompose/MsgComposeCommands.js (3638) - MsgComposeCloseWindow
===== JS> chrome://messenger/content/messengercompose/MsgComposeCommands.js (1697) - DoCommandClose
===== JS> chrome://messenger/content/messengercompose/messengercompose.xul (1)

The ComposeUnload at shutdown looks like this:
===== ComposeUnload, JS stack dump:
===== JS> chrome://messenger/content/messengercompose/MsgComposeCommands.js (2541)
===== JS> chrome://messenger/content/messengercompose/messengercompose.xul (1)

The obnoxious call that we don't understand yet form the test looks like this:
===== ComposeUnload, JS stack dump:
===== JS> chrome://messenger/content/messengercompose/MsgComposeCommands.js (2541)
===== JS> chrome://messenger/content/messengercompose/messengercompose.xul (1)
===== JS> chrome://messenger/content/messengercompose/MsgComposeCommands.js (2861) - GenericSendMessage calling SendMsg()
===== JS> chrome://messenger/content/messengercompose/MsgComposeCommands.js (2983) - SendMessageLater
===== JS> chrome://messenger/content/messengercompose/MsgComposeCommands.js (619)
===== JS> chrome://messenger/content/messengercompose/MsgComposeCommands.js (772)

Aha, SendMessageLater/SendMsg() (C++) closes the window.

I can reproduce the error if I first send a HTML message followed by a plaintext message. "Send later" of two messages does the same.

Comment 11

[Jorg K (CEST = GMT+2)]

Bingo! The problem occurs when switching from one message type to the other. The window of the previous type gets destroyed and the new type gets recycled.

Call stack:
nsMsgComposeService::CloseHiddenCachedWindow() Line 319	C++
nsMsgComposeService::CacheWindow() Line 864	C++
nsMsgCompose::CloseWindow() Line 1486	C++

The call that causes the JS ComposeUnload to be called is 
nsMsgComposeService::CloseHiddenCachedWindow() Line 318: baseWindow->Destroy();

Now that we know what happens, how do we fix it?

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1).

OK. Here is the fix. Don't go and try to remove an observer that is not ours since ours was already removed when our windows was recycled. Nasty stuff.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8718515 [details] [diff] [review]
Proposed solution (v1).

Oops, I didn't test this in the case of two concurrent compositions. This doesn't work, since the allObservers.getNext() == dictionaryRemovalObserver is never true. Anyway, this is the right approach, I just need to find a better way to identify the object. Suggestions?

Comment 14

[Jorg K (CEST = GMT+2)]

Hmm, the object could have a method to identify itself, but how do I convert allObservers.getNext() back to a JS object so I can call this method? I know very little about the XPCOM stuff.

Comment 15

[Jorg K (CEST = GMT+2)]

Forget all that. There is a very simple solution (I think): Let the object remember whether its observer got removed instead of using hasMoreElements() (which will find the other object's observer). Patch coming up.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2).

This works. I tested the concurrent composition with two windows and switching from HTML to plaintext.

I left the dump() statements in for you to test, they will of course be removed before checking in.

The solution is so simple that I ask myself why it hasn't occurred to us from the beginning, doh ;-)

Comment 17

[Jorg K (CEST = GMT+2)]

Regression from bug 1235205, the code added there wasn't quite right.

Comment 18

[:aceman]

Comment on attachment 8718798 [details] [diff] [review]
Proposed solution (v2).

Sorry I do not see how this solves the problem. It just hides it to no longer call removeObserver if we THINK it is already removed. But there may still be SOMETHING left in observers observing "spellcheck-dictionary-remove". Why do we want to leave it there? The original code pointed to that (by making exception) discrepancy, that that SOMETHING was not dictionaryRemovalObserver.

Or do you say, that that SOMETHING is the observer added by the other parallel compose window (it own copy of dictionaryRemovalObserver)? But I have never tried the scenario of having 2 windows.

But I can confirm the finding, that opening the window in the other compose mode (HTML vs . plaintext) makes the error come up more often.

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #18)
> Or do you say, that that SOMETHING is the observer added by the other
> parallel compose window (it own copy of dictionaryRemovalObserver)? But I
> have never tried the scenario of having 2 windows.

YES.

For every window a new object "dictionaryRemovalObserver" is instantiated. In carries inside by "closure" (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Closures) the document.documentElement, so when the observer finally gets to execute, this context is available. In the debugging you see two different objects.

In the case of the format switch from HTML to plaintext sequentially, this happens:

Window 1 (HTML) gets created, closed and recycled, the observer is removed.

Window 2 (plaintext) gets created, this is not the recycled one, and closed. Closing it decides to destroy the previously recycled window 1, whose observer was already removed. The error occurred because we tried to remove the observer again. Window 2 is then recycled and its observer gets removed.

At session shutdown, the Window 2 gets destroyed, and again, we can't remove its observer.

"Its observer" is not quite right, strictly speaking. This means: The observer being created for the window's document. You can also see this when removing dictionaries while you have multiple composition open. Multiple observers fire, and each one works on the windows/document it was created for.

I've just noticed a cut/paste error, it should be "this" in one spot more, fix coming up.

You will see it clearly in the debug:
Case 1: Open one new message, open another one concurrently, then close both.
Case 2: Send an e-mail in HTML followed by one in plaintext (or vice versa).

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2a).

Oops, it should use "this" at remove time.

Here is the debug for sending two messages of different type:

First message gets send, the observer is removed during the recycling.
======================================== Removing NOW!
======================================== removing recycle

Second message gets send, the previously recycled windows is destroyed
trying again to remove the observer. This time, we know it's already removed.
======================================== removing close

The window of the second type is now recycled.
======================================== Removing NOW!
======================================== removing recycle

Session shutdown: Nothing to do.
======================================== removing close

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #18)
> Or do you say, that that SOMETHING is the observer added by the other
> parallel compose window (it own copy of dictionaryRemovalObserver)? But I
> have never tried the scenario of having 2 windows.
I'm replying again here. There can be more than one observer. This case occurs "naturally" when there are concurrent/parallel compose windows active.
The case can also occur - as we painfully found out - when there are two subsequent compositions of different type. In this case, the first type is recycled but NOT reused, and a second one is created. The first one is destroyed when the second one is recycled. This happens here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#863

The proposed solution is dead simple. Every object adds and removes itself and keeps track if the "isAdded" state. So it doesn't care whether it's recycled and then destroyed or only destroyed. The first removal request succeeds, a potential second one is ignored.

Believe me, that recycling stuff has caused me many sleepless nights already. It's very complicated and also quite unnecessary, since the software doesn't run on a Pentium II 200 MHz with 128 MB RAM any more.

Comment 22

[:aceman]

(In reply to Jorg K (GMT+1) from comment #21)
> Believe me, that recycling stuff has caused me many sleepless nights
> already. It's very complicated and also quite unnecessary, since the
> software doesn't run on a Pentium II 200 MHz with 128 MB RAM any more.

Wanna try to rip it out? :) There is a bug for removing it (bug 777732), it just needs a clear case that the recycling really does not add anything these days. See bug 779074.

Comment 23

[:aceman]

Comment on attachment 8718838 [details] [diff] [review]
Proposed solution (v2a).

Review of attachment 8718838 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I think I understand that the Services.obs.enumerateObservers("spellcheck-dictionary-remove") call returns observers from a global list (whole app) so it included observers also from the parallel (even hidden) compose windows. Yes, we should not try to remove those. Each window should manage its added observers. So either we make the "topic" of "spellcheck-dictionary-remove" unique per compose window, or each observer keeps track whether it already removed itself from the global list.

I'll try to run with this patch in some hours.

How comes this problem does not happen for the other observers in compose window?

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #22)
> Wanna try to rip it out? :) There is a bug for removing it (bug 777732).
I would rip it out any time. However, that's a big job and it works right now. Also, people tend to be conservative around here and don't like clean-sweep actions. (I tried to rip out Eudora in bug 1243498, but the review is dragging along, despite being very very straight forward, indeed!)

(In reply to :aceman from comment #23)
> So either we make the
> "topic" of "spellcheck-dictionary-remove" unique per compose window, or each
> observer keeps track whether it already removed itself from the global list.
Sorry, the topic is set in stone. We get no information from M-C:
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/hunspell/glue/mozHunspell.cpp#569

> I'll try to run with this patch in some hours.
It will be fine. I finally got it right, third time lucky after bug 1203957 and bug 1235205 ;-)

> How comes this problem does not happen for the other observers in compose
> window?
There is only one other, the messageComposeOfflineQuitObserver. That one gets removed in RemoveMessageComposeOfflineQuitObserver called only in ComposeUnload() when the window is really destroyed for good.

I tried doing the same, so only removing my observer there, but that doesn't fly due to a subtle timing issue.

At shutdown, the last recycled window is destroyed *after* all the dictionaries get shutdown, so the observer actually runs at shutdown before it is removed notifying of *all* add-on dictionaries being removed. They are not removed by user action, which is what we need to track, they are removed because the system shuts down the add-ons. We really don't want to process those notifications since it would cause the spellchecker.dictionary preference to be corrected. Fortunately I documented that in bug 1235205 comment #7. The "Boing" in the debug over there meant that we just smashed the preference, which is what the other bug was about. (People may think that I write too many comments, but that is not the case: I document every little step and every conclusion, so my work can be 100% retraced and understood ;-))

Summarising, we are in a dilemma:
Removing at recycle time only is not an option since some windows are destroyed immediately via ComposeUnload(). And their observer hangs around (!!) and hits us at shutdown(*). This was the original implementation in bug 1203957 causing bug 1235205 which took me ages to understand.
Removing only at destruction time in ComposeUnload() would be nice, but comes too late (see previous paragraph).
So we need a mixed approach which was attempted but gotten wrong in bug 1235205. Now it's right ;-)

*): Neil commented on that in bug 1235205 comment #5 (quote):
===
ComposeUnload resets anything that doesn't automatically go away when the window is closed without being recycled, including things such as removing the quit observer, however it doesn't need to reset the recipients and attachments lists as those things are going away anyway.
===
So destroying a window makes some things go "away anyway" and others would hang around if not removed, like the observers.

Comment 25

[:aceman]

(In reply to Jorg K (GMT+1) from comment #24)
> (In reply to :aceman from comment #22)
> > Wanna try to rip it out? :) There is a bug for removing it (bug 777732).
> I would rip it out any time. However, that's a big job and it works right
> now. Also, people tend to be conservative around here and don't like
> clean-sweep actions. (I tried to rip out Eudora in bug 1243498, but the
> review is dragging along, despite being very very straight forward, indeed!)

We are conservative when trying to remove features. But this one does not change anything for the user and would simplify our architecture.

Comment 26

[Jorg K (CEST = GMT+2)]

<off-topic>

(In reply to :aceman from comment #25)
> We are conservative when trying to remove features. But this one does not
> change anything for the user and would simplify our architecture.
Actually, Eudora import is not a feature, since it simply doesn't work. And I would bet that there are no users for this feature since Eudora died in 2006. All the people who migrated to TB/Penelope, the add-on that gave some Eudora features to TB, already migrated their e-mail to TB ten years ago.

Removing windows recycling *does* make a change to the users who run under Windows XP on a Pentium II 200 MHz with 128 MB RAM. They are about as frequent as users wanting to import from Eudora in the year 2016.

I have never lobbied to remove working useful features. I would love to see the Outlook import work again.

</off-topic>

Comment 27

[:aceman]

Comment on attachment 8718838 [details] [diff] [review]
Proposed solution (v2a).

Review of attachment 8718838 [details] [diff] [review]:
-----------------------------------------------------------------

I think I like this now :)

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2a) with dump() removed.

Thanks for the review.

Comment 29

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/20fa8eb8cbafb5bbe6f87a7f51f5d08674cea190
Bug 1246517 - Protect better against double removal of "spellcheck-dictionary-remove" observer. r=aceman

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8718972 [details] [diff] [review]
Proposed solution (v2a) with dump() removed.

[Approval Request Comment]
Regression caused by (bug #): bug 1235205
User impact if declined: Unpredictable. Closing of composition window potentially leaving stuff behind which may lead to unpredictable consequences later.
Testing completed (on c-c, etc.): Manually and via test-charset-upgrade.js which is a test that always (silently) showed the symptom.
Risk to taking this patch (and alternatives if risky):
It is risky *not* to take the patch. The patch itself merely fixes some processing at composition window close time.

Comment 31

[Jorg K (CEST = GMT+2)]

Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/3b1107f78560

Comment 32

[Kent James (:rkent)]

Comment on attachment 8718972 [details] [diff] [review]
Proposed solution (v2a) with dump() removed.

http://hg.mozilla.org/releases/comm-beta/rev/5b79eb3f9dd6