Closed
Bug 1246517
Opened 8 years ago
Closed 8 years ago
Removing the dictionary removal observer doesn't work when switching from plaintext to HTML e-mail or vice versa.
Categories
(Thunderbird :: Message Compose Window, defect)
Thunderbird
Message Compose Window
Tracking
(thunderbird45 fixed, thunderbird46 fixed, thunderbird47 fixed)
RESOLVED
FIXED
Thunderbird 47.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
Details
(Keywords: regression)
Attachments
(1 file, 5 obsolete files)
4.07 KB,
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-aurora+
rkent
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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.
Attachment #8716827 -
Flags: feedback?(acelists)
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?
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' ?
Assignee | ||
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
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.
(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?
Assignee | ||
Comment 8•8 years ago
|
||
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.
Attachment #8716827 -
Attachment is obsolete: true
Attachment #8716827 -
Flags: feedback?(acelists)
Assignee | ||
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
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?
Summary: Removing the dictionary removal observer sometimes doesn't work. → Removing the dictionary removal observer doesn't work when switching from plaintext to HTML e-mail or vice versa.
Assignee | ||
Comment 12•8 years ago
|
||
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.
Assignee: nobody → mozilla
Attachment #8718387 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8718515 -
Flags: review?(acelists)
Assignee | ||
Comment 13•8 years ago
|
||
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?
Attachment #8718515 -
Flags: review?(acelists)
Assignee | ||
Comment 14•8 years ago
|
||
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.
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
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 ;-)
Attachment #8718515 -
Attachment is obsolete: true
Attachment #8718798 -
Flags: review?(acelists)
Assignee | ||
Comment 17•8 years ago
|
||
Regression from bug 1235205, the code added there wasn't quite right.
Blocks: 1235205
Keywords: regression
Comment 18•8 years ago
|
||
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.
Assignee | ||
Comment 19•8 years ago
|
||
(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).
Assignee | ||
Comment 20•8 years ago
|
||
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
Attachment #8718798 -
Attachment is obsolete: true
Attachment #8718798 -
Flags: review?(acelists)
Attachment #8718838 -
Flags: review?(acelists)
Assignee | ||
Comment 21•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
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?
Attachment #8718838 -
Flags: feedback+
Assignee | ||
Comment 24•8 years ago
|
||
(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•8 years ago
|
||
(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.
Assignee | ||
Comment 26•8 years ago
|
||
<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•8 years ago
|
||
Comment on attachment 8718838 [details] [diff] [review] Proposed solution (v2a). Review of attachment 8718838 [details] [diff] [review]: ----------------------------------------------------------------- I think I like this now :)
Attachment #8718838 -
Flags: review?(acelists) → review+
Assignee | ||
Comment 28•8 years ago
|
||
Thanks for the review.
Attachment #8718838 -
Attachment is obsolete: true
Attachment #8718972 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 29•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/20fa8eb8cbafb5bbe6f87a7f51f5d08674cea190 Bug 1246517 - Protect better against double removal of "spellcheck-dictionary-remove" observer. r=aceman
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
Assignee | ||
Comment 30•8 years ago
|
||
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.
Attachment #8718972 -
Flags: approval-comm-beta?
Attachment #8718972 -
Flags: approval-comm-aurora+
Assignee | ||
Updated•8 years ago
|
status-thunderbird45:
--- → affected
status-thunderbird46:
--- → affected
status-thunderbird47:
--- → fixed
Assignee | ||
Comment 31•8 years ago
|
||
Aurora (TB 46): https://hg.mozilla.org/releases/comm-aurora/rev/3b1107f78560
Comment 32•8 years ago
|
||
Comment on attachment 8718972 [details] [diff] [review] Proposed solution (v2a) with dump() removed. http://hg.mozilla.org/releases/comm-beta/rev/5b79eb3f9dd6
Attachment #8718972 -
Flags: approval-comm-beta? → approval-comm-beta+
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•