777732 - Remove vestiges of compose window recycling

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Joshua Cranmer [:jcranmer]]

The title says it all.

Comment 1

[:Irving Reid (No longer working on Firefox)]

Compose window recycling is partly removed in bug 777063

Comment 2

[Jorg K (CEST = GMT+2)]

I am all in favour of removing the recycling stuff. It's a BIG headache in composition. It's also obsolete IMHO, since the software doesn't run on a Pentium II 200 MHz with 128 MB RAM any more.

Also see bug 779074.

Comment 3

[Wayne Mery (:wsmwk)]

I agree ... as long as we have good testing - since the default is 1 = enabled ;)

see also bug 814651, bug 547270, bug 833909

Comment 4

[Jorg K (CEST = GMT+2)]

OK, I can remove it, although it's no vestige, it is real and executed on every message written with the default settings. I've removed heaps of code in my life, so I'm good at that ;-)

The problem is: Do we have agreement? Joshua seems to be in favour. But how about our more conservative developers.

Magnus, Kent: Can I have the "go ahead"?

Comment 5

[Kent James (:rkent)]

Yes composer window recycling is a pain, and it would be good to remove it. But that is about all I can say. I don't know the original reasons why composer window recyling was added, nor what problem it was supposed to solve. I've sort of assumed it was a performance thing, and 10 years later perhaps it is no longer needed.

Does anyone know what problem composer recycling was supposed to solve?

Comment 6

[Jorg K (CEST = GMT+2)]

Performance, see bug 779074.

Comment 7

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #5)
> Yes composer window recycling is a pain, and it would be good to remove it.
> But that is about all I can say. I don't know the original reasons why
> composer window recyling was added, nor what problem it was supposed to
> solve. I've sort of assumed it was a performance thing, and 10 years later
> perhaps it is no longer needed.
> 
> Does anyone know what problem composer recycling was supposed to solve?

The original reason was performance. I at one point timed that compose window caching actually took more time than starting it up from scratch.

A secondary reason, one that caused bienvenu to oppose this path, was that caching the compose window helped minimize the issues of compose leaks, of which we've certainly had some trouble.

Comment 8

[Magnus Melin [:mkmelin]]

Yes, certainly moving forward there's little room for this. IIRC recycling was broken all through the tb24(?) cycle and nobody really noticed.

Comment 9

[WBT]

It's still broken.  See Bug 1046207 ("Compose Message window ghost exposes contents of previous composition for a moment when starting a new message") but can't quite be preferenced out due to Bug 814651 (breaking Sendfilters).

Comment 13

[:aceman]

On a debug trunk build, there is still a perf difference between using a recycled window and a new one (tested by opening compose in HTML mode and then in plain text mode, I think that needs to build a new compose window without recycling).

But on TB38 release (optimized), I see no difference optically. Both opens are instant (on my beefy 3.6Ghz machine). When I underclock it to 800Mhz. The different opens (with different compose modes) are not instant, they take a bit longer, and they are not the same speed, but still very close. And even the slower open (without recycling) is acceptable to me.

So I would also support removing the code complexity when it brings no gain anymore.

Comment 14

[Jorg K (CEST = GMT+2)]

Thanks for doing some timings here. This is next on my list right after Magnus approves my Eudora kill. (Aceman, this has been sitting there for over a month, perhaps you want to steal the review in bug 1243498.)

Comment 15

[David E. Ross]

It was noted in bug #1046207 that a proposed fix for this bug might adversely impact the "Send Filter" capability of Thunderbird.  The reply to that comment cited the Send Filter extension.  

The Send Filter extension for Thunderbird was rendered obsolete by the implementation of its functionality into "vanilla" Thunderbird.  I was using that extension but removed it about a year ago.  See bug #11039.  Thus, it is important that correcting this bug not adversely impact the inherent "send filter" capability.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: Remove recycling (v1).

Comment 17

[Jorg K (CEST = GMT+2)]

Let's see what breaks. I gave it a quick test and everything still seemed to work.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c7200a7764a5

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8735298 [details] [diff] [review]
Remove recycling (v1).

OK, Magnus said he's available, so r?
Of course the try isn't finished yet.

This is all straight forward deletion. The diff looks confusing where if/else parts got removed and what was not removed had the indentation level changed.

I've done the SM part as well, frankly, no need to ask them for review. If mailnews goes away, they have no chance ;-)

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 8735298 [details] [diff] [review]
Remove recycling (v1).

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

Looks ok to me. r=mkmelin if the try run is fine

Comment 20

[Jorg K (CEST = GMT+2)]

Almost worked, one failure:
test-charset-upgrade.js::test_encoding_upgrade_plaintext_compose

Comment 21

[:aceman]

Are all the relevant cleanups from gComposeRecyclingListener.onClose done elsewhere on window close?

Comment 22

[Jorg K (CEST = GMT+2)]

This is about the weirdest thing I've seen in a while.

Hard to believe, but this fails on:
compWin.keypress(null, "VK_RETURN", {shiftKey: true, accelKey: true});
with
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\composition\test-charset-upgrade.js | test-ch
arset-upgrade.js::test_encoding_upgrade_plaintext_compose
  EXCEPTION: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendKeyEvent]
    at: nonesuch line 342

This failure in fact has nothing to do with my patch. It can be made to fail without the patch by adding
Services.prefs.setIntPref("mail.compose.max_recycled_windows", 0);
to the test.

So the test_encoding_upgrade_plaintext_compose test only ever worked in recycling mode.

It's ever weirder, I stripped it down to this failing:

function test_encoding_upgrade_plaintext_compose() {
  Services.prefs.setBoolPref("mail.identity.default.compose_html", false);
  let compWin = open_compose_new_mail();
  Services.prefs.setBoolPref("mail.identity.default.compose_html", true);

  compWin.type(null, "[e-mail]");
  compWin.type(compWin.eid("msgSubject"), "encoding upgrade test - plaintext");

  // Ctrl+Shift+Return = Send Later.
  compWin.keypress(null, "VK_RETURN", {shiftKey: true, accelKey: true});
}

This already fails!!!

There are actually very few spots where we simulate <ctrl><shift><enter> to send later. The only other occurrence uses:
+  // Send it later.
+  plan_for_window_close(compWin);
   // Ctrl+Shift+Return = Send Later
   compWin.keypress(null, "VK_RETURN", {shiftKey: true, accelKey: true});
+  wait_for_window_close(compWin);

but that doesn't help here.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Showing the test failure.

Apply this patch WITHOUT applying the other one. Then run:
mozmake SOLO_TEST=composition/test-charset-upgrade.js mozmill-one

Comment 24

[Jorg K (CEST = GMT+2)]

The patch strips down test-charset-upgrade.js to the bit that fails.

If you additionally comment out
  Services.prefs.setBoolPref("mail.identity.default.compose_html", false);
you'll get a different error "There was an error saving the message to Outbox. Retry?"

Totally weird.

Comment 25

[Jorg K (CEST = GMT+2)]

Capturing some of the conversation with Aceman on IRC:

He was asking whether "send later" relies on window recycling, that is, the hidden composition window might be accessed to create the message to store in the outbox. Of course if the composition window gets destroyed too early, the "send later" will fail.

I'll investigate along those lines.

In nsMsgCompose::SendMsg we see:
i'm assuming the compose window is still up at this point

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: Some more tweaks.

OK, I tried sending a 10 MB to the outbox. There is no problem. The composition windows stays open until the message has made it's way to the outbox.

In the end, this turned out to be a timing problem. Placing sending the <ctrl><shift><enter> into a setTimeout with zero timeout solved the problem.

Comment 27

[Jorg K (CEST = GMT+2)]

Let's see whether this works, it's fine on local Windows:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a5b2331e69a4

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: Part 1: Remove recycling (v1a = v1 + more tweaks) [landed, comment #29]

Here is version v1 together with the few tweaks from the "more tweaks" patch (comments, fixed test failure).

Comment 29

[Jorg K (CEST = GMT+2)]

Part 1 landed:
https://hg.mozilla.org/comm-central/rev/158bced7b129

Let the regressions roll in ;-)
SM people, can you please build and see whether you can still send e-mail.

Part 2 will be about removing all the listeners to
compose-window-close and compose-window-reopen since these events aren't sent any more.
Patch coming up.

Comment 30

[:aceman]

Why would compose-window-close go away? That event still does happen. I can understand compose-window-reopen.

Comment 31

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #30)
> Why would compose-window-close go away? That event still does happen. I can
> understand compose-window-reopen.
Nope, this event does not happen any more:
https://hg.mozilla.org/comm-central/rev/158bced7b129#l1.79
This was the *only* place to dispatch the event:
https://dxr.mozilla.org/comm-central/search?q=compose-window-close&redirect=true&case=false

Comment 32

[:aceman]

I mean why it is OK to remove it? We still do close the window (whether there is any recycling or not). Surely some extensions listen to the event?

Comment 33

[Jorg K (CEST = GMT+2)]

When running with mail.compose.max_recycled_windows = 0, the event never fired.
When running with mail.compose.max_recycled_windows = 1, it never fired on the second concurrent window, not even at application shutdown time.

Now we *destroy* windows and that will destroy everything that was attached to them.
Destroying will run ComposeUnload() in MsgComposeCommands.js.

Comment 34

[:aceman]

Yes, and before destroying the window extensions want to run their stuff.

So is there any event dispatched in ComposeUnload they could listen to? Are you sure when the window was being destroyed that gComposeRecyclingListener.onClose() wasn't run?

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: Remove listeners to compose-window-close/reopen

There has been some discussion on IRC whether we should now call compose-window-close in ComposeUnload() since about 36 add-ons (I've checked on https://mxr.mozilla.org/addons) were listening to that call.

I think this makes no sense. The default was always mail.compose.max_recycled_windows = 1, that is, a second or third concurrent composition just got destroyed with no such event being fired. All the add-ons relying on this event were in fact wrong and didn't handle concurrent composition. They also didn't handle mail.compose.max_recycled_windows = 0.

If someone is interested the compose window being closed/destroyed, we could invent a new event compose-window-destroy and fire it.

Comment 36

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #34)
> Are you sure when the window was being destroyed that
> gComposeRecyclingListener.onClose() wasn't run?
Yes. I am sure. And BTW, you know it, too. Remember bug 1246517 where we had a real bad time finding a solution to always remove the spellcheck-dictionary-remove listener once only. There were various cases:
1) Window got recycled, that removed the listener.
2) Window got destroyed without being recycled, that also removed the listener.
3) Window got recycled first, and then destroyed. That double-removed the listener until we fixed that.

In the past, windows *did* get destroyed without the gComposeRecyclingListener.onClose() ever being run on them.

Comment 37

[:aceman]

So can we move that event to ComposeUnload? Can you look into the addons and try to guess what they actually intended? If they wanted to listen to the window being closed or removed, making the event actually work would be better than making all the addons adapt to a new event name, that does the same.

Comment 38

[Jorg K (CEST = GMT+2)]

Please read comment #35: It makes no sense.

I'd like to hear Magnus's opinion here.

Also, I'm not quite sure how these listeners work. Sure, you can run a listener on a window that is being hidden. There add-ons can reach into the window's document and do their thing. If the windows is being destroyed, it would be absolutely fatal to call the same event with changed semantics. Whatever they did before might not work on a destroyed window. And no, I won't look at 36 add-ons and guess.

This is being landed on TB 48, so they have four cycles to fix their add-ons.

Comment 39

[Jorg K (CEST = GMT+2)]

Or let's ask Neil:

Neil, as part of the removal of compose window recycling, gComposeRecyclingListener.onClose() and gComposeRecyclingListener.onReopen() were removed together with a whole lot of other processing:
https://hg.mozilla.org/comm-central/rev/158bced7b129#l1.13
Consequently the events compose-window-close/reopen are no longer fired. All windows are destroyed via ComposeUnload().

Question: Should we implement some event in ComposeUnload() or even call compose-window-close from there? And should some of the processing that was removed from gComposeRecyclingListener.onClose() be restored back to ComposeUnload().

So far I have assumed that the answer is "no", since with mail.compose.max_recycled_windows = 0 windows were simply destroyed without any of the gComposeRecyclingListener.onClose() being executed. Equally with mail.compose.max_recycled_windows = n (where n > 0), the concurrent composition number (n+k) (k > 0) was just destroyed.

Comment 40

[:aceman]

Comment on attachment 8735550 [details] [diff] [review]
Part 2: Remove listeners to compose-window-close/reopen

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

::: mail/components/compose/content/bigFileObserver.js
@@ -47,5 @@
> -    bucket.removeEventListener("attachments-removed", this, false);
> -    bucket.removeEventListener("attachments-uploading", this, false);
> -    bucket.removeEventListener("attachment-uploaded", this, false);
> -    bucket.removeEventListener("attachment-upload-failed", this, false);
> -    bucket.removeEventListener("attachments-converted", this, false);

It it OK to NOT remove listeners that we added (above) even if the window is being destroyed? Won't that leak something?

Comment 41

[:aceman]

(In reply to Jorg K (GMT+1) from comment #38)
> Also, I'm not quite sure how these listeners work. Sure, you can run a
> listener on a window that is being hidden. There add-ons can reach into the
> window's document and do their thing. If the windows is being destroyed, it
> would be absolutely fatal to call the same event with changed semantics.
> Whatever they did before might not work on a destroyed window.
No, they ran the code while the window was still existing (the event is dispatched before destroying it).
So I do not see a difference between running the code before the window is just hidden (due to recycling) and when it is removed completely.

A difference may be that some of that code was just doing reseting the addon widgets to initial state so that the the window can be reused on next unhiding. Similar code that you just remove here. Then that code is no longer needed.
Anyway, we need to document this change in the proper 'TB changelog for developers' page so that addon authors can decide whether to remove that code, or start watching the new event you propose (and I support there being at least some event).

> And no, I won't look at 36 add-ons and guess.
I meant a random sample.

Comment 42

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #40)
> It it OK to NOT remove listeners that we added (above) even if the window is
> being destroyed? Won't that leak something?
The window/document destruction should take care of it. Note that other listeners are now no longer removed, for example:
https://hg.mozilla.org/comm-central/rev/158bced7b129#l1.35
https://hg.mozilla.org/comm-central/rev/158bced7b129#l1.39

The ones that *must* be removed since they are *not* attached to the window/document are still removed here, for example:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2422

Comment 43

[Jorg K (CEST = GMT+2)]

Attachment: Part 1a: Remove recycling, part 1a, missed bits [landed comment #45]

Oops, this is inconsequential, but should be fixed.

Comment 44

[Kent James (:rkent)]

Comment on attachment 8735901 [details] [diff] [review]
Part 1a: Remove recycling, part 1a, missed bits [landed comment #45]

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

These changes are simple missed parts of the original, so rs+=me

Comment 45

[Jorg K (CEST = GMT+2)]

Comment on attachment 8735901 [details] [diff] [review]
Part 1a: Remove recycling, part 1a, missed bits [landed comment #45]

Part 1a landed, sorry about the missed bits:
https://hg.mozilla.org/comm-central/rev/e013e00901f0

Comment 46

[Philip Chee]

Comment on attachment 8735901 [details] [diff] [review]
Part 1a: Remove recycling, part 1a, missed bits [landed comment #45]

rs=me thanks!

Comment 47

[Magnus Melin [:mkmelin]]

Comment on attachment 8735550 [details] [diff] [review]
Part 2: Remove listeners to compose-window-close/reopen

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

I think this is ok. 
People can probably listen to a window "close" event for the same purpose too (didn't try!).

Comment 48

[Jorg K (CEST = GMT+2)]

Thanks for the review.

Since Aceman seems to be opposed to tossing it all, I've written to Patrick Brunschwig, the maintainer of Enigmail. Enigmail listens to the discontinued event. They also listen to the "unload" event (which is what you meant, I assume).

Let's land this and be done here. Note the suite part here in the patch.

Comment 49

[WBT]

In TB 38.5 without this patch applied, I just had an experience of:
1. Receive a forwarded message which had a space for a banner image showing just the "file link broken" icon and image box, which also had an attached png image with the banner that presumably was meant to go in that slot.  
2. Click to forward the message.  A Compose window opens, showing that the png file will be attached.
3. Edit the message text and send it.  It sends successfully and the window closes.
4. Receive another message (on a different account, if that matters) with an image attached, and the text refers to the image as being "attached" with no apparent spot for it in the body of the message.
5. Click to forward this message as well (to a different recipient).
6. Edit the text in that message and prepare to send.
7. Notice that in the Attachments section, *BOTH IMAGES* are listed as being attached: the one that should be attached, AND the one from the previous message which should NOT be attached or sent to the recipient of the second forward.  The expected behavior is that only the image attached to the message of Step 4 is attached to the forward. 

This seems to be not trivially reproducible, so I'm wondering if it would be worth investing the time into writing this up as a separate bug or if that'd be closed right away with a reference to the patch in this one.

Comment 50

[Patrick Brunschwig]

(In reply to Jorg K (GMT+1) from comment #48)
> Thanks for the review.
> 
> Since Aceman seems to be opposed to tossing it all, I've written to Patrick
> Brunschwig, the maintainer of Enigmail. Enigmail listens to the discontinued
> event. They also listen to the "unload" event (which is what you meant, I
> assume).
> 
> Let's land this and be done here. Note the suite part here in the patch.

I'm glad to see that this gets landed. Enigmail does not do anything (anymore) with the unload event. If windows are no longer re-opened, then there is no need for these events for Enigmail anymore.

Comment 51

[:aceman]

Can we now remove code that was only called from the removed .onClose() and was only clearing out the compose widgets for next composition?
E.g. awResetAllRows(), ClearIdentityListPopup(), maybe EditorResetFontAndColorAttributes() (references recycling).

ReleaseGlobalVariables() is still used in TB, but not in SM. Why?

Comment 52

[:aceman]

(In reply to Jorg K (GMT+1) from comment #48)
> They also listen to the "unload" event (which is what you meant, I
> assume).

Can you check if listening to the window "unload" runs extension code before the onunload=ComposeUnload we have in base TB ?
If yes, that could be enough.

Comment 53

[Magnus Melin [:mkmelin]]

Yes there are probably a whole lot of other simplifications and removals that can be done.

Comment 54

[Jorg K (CEST = GMT+2)]

(In reply to WBT from comment #49)
> This seems to be not trivially reproducible, so I'm wondering if it would be
> worth investing the time into writing this up as a separate bug or if that'd
> be closed right away with a reference to the patch in this one.
We won't investigate anything that will be fix by this bug here.
You can use a Daily (I'm using the one compiled 2016-03-29) and see whether it still shows problems.

(In reply to :aceman from comment #51)
> Can we now remove code that was only called from the removed .onClose() and
> was only clearing out the compose widgets for next composition?
> E.g. awResetAllRows(), ClearIdentityListPopup(), maybe
> EditorResetFontAndColorAttributes() (references recycling).
Will do. Thanks for checking. editor/ was the only directory I didn't check for "recycl*", sorry.

> ReleaseGlobalVariables() is still used in TB, but not in SM. Why?
You'd have to ask the SM people.

(In reply to :aceman from comment #52)
> Can you check if listening to the window "unload" runs extension code before
> the onunload=ComposeUnload we have in base TB ?
Not easy to check, I'd have to register that in an extension and see what happens.
If you insist, I'll do so. However, why would that make a difference? The windows/document goes away and everyone shuts down their stuff. Is the order important?

Comment 55

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: Remove listeners to compose-window-close/reopen - TB only [landed, comment #57]

Splitting part 2.

Comment 56

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: Remove listeners to compose-window-close/reopen - SM [landed modified, comment #71]

OK, here is the SM part split off so the SM people can give their OK.

The problem is that onAbClearSearch() is no longer used, so I removed it. TB still uses it in DirPaneSelectionChange(), so the SM folks might want to preserve it for future use.

Comment 57

[Jorg K (CEST = GMT+2)]

Comment on attachment 8736192 [details] [diff] [review]
Part 2: Remove listeners to compose-window-close/reopen - TB only [landed, comment #57]

Part 2 (TB only) landed:
https://hg.mozilla.org/comm-central/rev/452569be7f75

Part 3 coming up for some further clean-up, see comment #51.

Comment 58

[:aceman]

(In reply to Jorg K (GMT+1) from comment #54)
> (In reply to :aceman from comment #52)
> > Can you check if listening to the window "unload" runs extension code before
> > the onunload=ComposeUnload we have in base TB ?
> Not easy to check, I'd have to register that in an extension and see what
> happens.
> If you insist, I'll do so. However, why would that make a difference? The
> windows/document goes away and everyone shuts down their stuff. Is the order
> important?

Yes, it makes a difference. IF ComposeUnload runs first, and runs ReleaseGlobalVariables(), running any code in addons after that may be nonsense. The addons may need the variables initialized and whole window still working.

So if our ComposeUnload runs first, I would still think we need to provide a new event (with new name) as the first thing in ComposeUnload. If addons run first, we do not need the event.

Comment 59

[Jorg K (CEST = GMT+2)]

Attachment: Part 3: Remove awResetAllRows(), ClearIdentityListPopup(), EditorResetFontAndColorAttributes() - TB+SM [landed, comment #71]

OK, as mentioned in comment #51, awResetAllRows(), ClearIdentityListPopup() and EditorResetFontAndColorAttributes() can also go.

Comment 60

[Philip Chee]

Comment on attachment 8736196 [details] [diff] [review]
Part 2: Remove listeners to compose-window-close/reopen - SM [landed modified, comment #71]

(In reply to Jorg K (GMT+1) from comment #56)
> OK, here is the SM part split off so the SM people can give their OK.
> 
> The problem is that onAbClearSearch() is no longer used, so I removed it. TB
> still uses it in DirPaneSelectionChange(), so the SM folks might want to
> preserve it for future use.

I think we should retain onAbClearSearch() to make it easier to port Thunderbird patches to SeaMonkey MailNews.
CC IanN: what do you think?

Comment 61

[Jorg K (CEST = GMT+2)]

Attachment: Simple add-on that will attach an unload listener to each composition window

(In reply to :aceman from comment #58)
> So if our ComposeUnload runs first, I would still think we need to provide a
> new event (with new name) as the first thing in ComposeUnload. If addons run
> first, we do not need the event.

The result/debug with this add-on is shown below:
Add-on> Preparing compose window
and later ...
TB core: ComposeUnload started
TB core: ComposeUnload exiting
Add-on> Unload arrived

So the add-on receives the unload event after we're done in ComposeUnload().

I think that is expected. ComposeUnload() is triggered from here:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#34

The unload event for the window gets triggered when the window is destroyed by Gecko.

May I enquire one more time: In the past, with mail.compose.max_recycled_windows set to 1, many windows got destroyed without any notification, that is, all the second and third, etc. compositions. They *never* received any notification and to my knowledge, that has never caused a problem. So now all composition windows get destroyed without further notification, so why would that suddenly be a problem when it was part of the normal accepted behaviour of the system?

The notifications compose-window-close/reopen were dispatched so listeners could "clean" the window for further reuse. I have an add-on myself (dictionary for recipient) which listens to these events since it stores stuff in the window and needs to do a clean-up when the window gets recycled. Of course my add-on works on windows which get destroyed immediately.

Look, it's a five minute job to code:
    document.getElementById("msgcomposeWindow").dispatchEvent(
      new Event("compose-window-about-to-be-destroyed", { bubbles: false , cancelable: false }));
I just don't see the necessity.

Even the very sophisticated add-on Enigmail has no need for such an event.

And I repeat one more time: If some add-on does something on a second composition and doesn't clean up, then it has had the problem until today an will keep having the problem, only more frequently.

Comment 62

[:aceman]

(In reply to Jorg K (GMT+1) from comment #61)
> So the add-on receives the unload event after we're done in ComposeUnload().
>
> The notifications compose-window-close/reopen were dispatched so listeners
> could "clean" the window for further reuse. I have an add-on myself
> (dictionary for recipient) which listens to these events since it stores
> stuff in the window and needs to do a clean-up when the window gets
> recycled. Of course my add-on works on windows which get destroyed
> immediately.

Then why are addons (and even our code) listening for "compose-window-init" event? Why is window.onload not enough?
I think exactly because the want to run only after the window and variables is also set up. Therefore "compose-window-init" is run late in ComposeLoad() and ComposeStartup().

I just want to provide a symmetric event when the window is going away. I don't say it is required now, just that somebody could use it. You said you didn't look at all those addons. Maybe some of them was observing "compose-window-close" just because there was nothing better available (even if unreliable). And window.onunload does not get the job done as the variables are teared down.

Is that understandable?

> Look, it's a five minute job to code:
>     document.getElementById("msgcomposeWindow").dispatchEvent(
>       new Event("compose-window-about-to-be-destroyed", { bubbles: false ,
> cancelable: false }));

Yes, that is all that is needed (at the top of ComposeUnload). If you do not want to do it here, I can do it in a new bug.

Comment 63

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #62)
> Yes, that is all that is needed (at the top of ComposeUnload). If you do not
> want to do it here, I can do it in a new bug.
That would be better so we can refer people to this new bug with few comments instead of this bug which is quickly becoming very messy. Shall we call it "compose-window-unload"?

Comment 64

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: Correction, bringing back the erroneously removed onComposerReOpen as onComposerLoad. [landed comment #67]

Sorry about that :-((

Comment 65

[:aceman]

Comment on attachment 8736487 [details] [diff] [review]
Part 2: Correction, bringing back the erroneously removed  onComposerReOpen as onComposerLoad. [landed comment #67]

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

Yes, without this the View -> "Message security info" does not work as the SMIME variables are not initialized.

Comment 66

[:aceman]

Comment on attachment 8736487 [details] [diff] [review]
Part 2: Correction, bringing back the erroneously removed  onComposerReOpen as onComposerLoad. [landed comment #67]

This is just putting back code that was mistakenly removed so please your rs here ;)

Comment 67

[Jorg K (CEST = GMT+2)]

Comment on attachment 8736487 [details] [diff] [review]
Part 2: Correction, bringing back the erroneously removed  onComposerReOpen as onComposerLoad. [landed comment #67]

Correction of Part 2 (TB only) landed:
https://hg.mozilla.org/comm-central/rev/e2b4f6083662

Comment 68

[Philip Chee]

> Correction of Part 2 (TB only) landed:
> https://hg.mozilla.org/comm-central/rev/e2b4f6083662
What about SeaMonkey???

Comment 69

[Jorg K (CEST = GMT+2)]

Sorry if this is confusing.

Part 1 and Part 1a, TB+SM:
https://hg.mozilla.org/comm-central/rev/158bced7b129
https://hg.mozilla.org/comm-central/rev/e013e00901f0

Part 2, TB only, and it's correction:
https://hg.mozilla.org/comm-central/rev/452569be7f75
https://hg.mozilla.org/comm-central/rev/e2b4f6083662

Part 2, SM is awaiting Ian's review.

Part 3, TB+SM is awaiting Magnus' review.

Still to review/land: Part 2, SM and Part 3 TB+SM.

Comment 70

[Jorg K (CEST = GMT+2)]

Comment on attachment 8736196 [details] [diff] [review]
Part 2: Remove listeners to compose-window-close/reopen - SM [landed modified, comment #71]

I've landed this but I've maintained onAbClearSearch() as requested by Ratty for possible future ports.

Comment 71

[Jorg K (CEST = GMT+2)]

We're finally done here. Summary:

Part 1 and Part 1a, TB+SM:
https://hg.mozilla.org/comm-central/rev/158bced7b129
https://hg.mozilla.org/comm-central/rev/e013e00901f0

Part 2, TB only, and its correction:
https://hg.mozilla.org/comm-central/rev/452569be7f75
https://hg.mozilla.org/comm-central/rev/e2b4f6083662

Part 2, SM only:
https://hg.mozilla.org/comm-central/rev/e3c972f493fd

Part 3, TB+SM:
https://hg.mozilla.org/comm-central/rev/86bd8ee03e9d

Comment 72

[David E. Ross]

Thunderbird 38.7.1 was recently released.  This bug report is scheduled for Thunderbird 48.  When might that version be released?

Comment 73

[Jorg K (CEST = GMT+2)]

Please refer to the release calendar: https://wiki.mozilla.org/RapidRelease/Calendar
Thunderbird 48 will be available as beta version shortly after 2016-06-07.

The next official ESR release is TB 52 due in early 2017. It's not on the calendar yet.