Last Comment Bug 840474 - Clicking on new mail notification opens two MailNews windows when no MailNews window is open
: Clicking on new mail notification opens two MailNews windows when no MailNews...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: seamonkey2.22
Assigned To: Frank Wein [:mcsmurf]
:
Mentors:
Depends on:
Blocks: 404580
  Show dependency treegraph
 
Reported: 2013-02-12 04:25 PST by Frank Wein [:mcsmurf]
Modified: 2013-07-22 08:04 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
Patch (2.13 KB, patch)
2013-04-28 06:07 PDT, Frank Wein [:mcsmurf]
no flags Details | Diff | Review
Patch (5.52 KB, patch)
2013-04-28 10:06 PDT, Frank Wein [:mcsmurf]
no flags Details | Diff | Review
Patch v2 (4.18 KB, patch)
2013-04-28 10:11 PDT, Frank Wein [:mcsmurf]
mnyromyr: review+
bugspam.Callek: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Review
Disable new mail alert for SeaMonkey 2.18 release (1015 bytes, patch)
2013-05-06 23:38 PDT, Frank Wein [:mcsmurf]
philip.chee: review+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Review

Description Frank Wein [:mcsmurf] 2013-02-12 04:25:46 PST
To reproduce:
1. Fetch trunk build with the new mail notification code
2. Wait for new mail to arrive (or send yourself a mail :)
3. Click on the Subject in the new mail notification to open MailNews and display the mail

Results:
Roughly half the time a new, second MailNews window opens although a MailNews window is already open. In the already open window the new mail gets displayed, in the second, new window I just see a blank message pane.
Comment 1 Philip Chee 2013-02-17 07:56:56 PST
When you click on the Subject in the new mail notification. The onclick handler in the XBL binding looks for an existing mail 3pane window to focus and select the mail message:
http://hg.mozilla.org/comm-central/annotate/8a074bb38774/suite/mailnews/mailWidgets.xml#l2075

Then if there is a callback the callback is called:
http://hg.mozilla.org/comm-central/annotate/8a074bb38774/suite/mailnews/mailWidgets.xml#l2100

And of course we have a callback in nsMessengerWinIntegration.cpp:
http://hg.mozilla.org/comm-central/annotate/8a074bb38774/mailnews/base/src/nsMessengerWinIntegration.cpp#l638

Which then calls nsMessengerWinIntegration::AlertClicked():
http://hg.mozilla.org/comm-central/annotate/8a074bb38774/mailnews/base/src/nsMessengerWinIntegration.cpp#l612

This looks for an existing mail 3pane window and if found focuses on it. If there is no open 3pane window then a new one is opened:
http://hg.mozilla.org/comm-central/annotate/8a074bb38774/mailnews/base/src/nsMessengerWinIntegration.cpp#l632

So offhand I'd guess that there is a race condition somewhere.
Comment 2 Frank Wein [:mcsmurf] 2013-03-04 05:19:44 PST
Should block SeaMonkey 2.18, highly visible bug when using MailNews (at least for me).
Comment 3 Frank Wein [:mcsmurf] 2013-04-27 10:06:36 PDT
Possible cause of this: Getting the domWindow at http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessengerWinIntegration.cpp#147 fails (can this happen?). Then it won't execute the return inside that code block and instead continue to the next code block that happens another window. The "else" before that code block has been removed a while ago as part of Bug Bug 556114. Not sure if this was a good idea.
Comment 4 Frank Wein [:mcsmurf] 2013-04-28 01:19:24 PDT
To reliable reproduce this the MailNews window needs to be closed. Then two MailNews windows will open when clicking on the new mail notification.
My theory from Comment 3 was wrong, in the case of this bug it does not fail to get a domWindow. Instead it fails to get a topMostMsgWindow, so it opens a new window.
Comment 5 Frank Wein [:mcsmurf] 2013-04-28 02:04:05 PDT
So basically the problem is: This code
141   nsCOMPtr<nsIMsgWindow> topMostMsgWindow;
142   rv = mailSession->GetTopmostMsgWindow(getter_AddRefs(topMostMsgWindow));

uses the member variable mWindows (in nsMsgMailSession) to check if there is an already open window. But in our case mWindows.count() is zero. That's because a new window gets opened via this code
2081             var mailWindowService = Components.classes["@mozilla.org/messenger/windowservice;1"].getService(Components.interfaces.nsIMessengerWindowService);
2083             mailWindowService.openMessengerWindowWithUri("mail:3pane", this.folderUri, this.msgKey);

But the member variable mWindows in nsMsgMailSession gets updated via the function nsIMsgMailSession->AddMsgWindowThis function gets called in suite/mailnews/mailWindow.js InitMsgWindow(). This is too late for us as the JS init code is basically async (not tied to the openMessengerWindowWithUri code). That is why the GetTopmostMsgWindow function does not know about the new mail window yet.
Comment 6 Frank Wein [:mcsmurf] 2013-04-28 06:07:37 PDT
Created attachment 742816 [details] [diff] [review]
Patch

This patch moves the logic from the C++ code to the JS file (mostly), after all the C++ code mostly duplicated the logic from the JS file. Maybe a better name should be used for the new function though.
Comment 7 Frank Wein [:mcsmurf] 2013-04-28 10:06:10 PDT
Created attachment 742837 [details] [diff] [review]
Patch

Mark: This is a bit of a hack (but only a small one), I added a bit of #ifdef MOZ_SUITE only code to a few .cpp files in mailnews/. Basically this patch moves the code logic for opening the MailNews window after clicking on a new mail alert mostly from the C++ code to JS code. See Comment 5 why this is required. The problem is when there is no MailNews window, you can currently either get two new MailNews windows (bad) or one window, but in that one window it does not select the new message (see for example http://hg.mozilla.org/comm-central/annotate/c03aec453327/mailnews/base/src/nsMessengerWinIntegration.cpp#l633 it only opens the folder with the new mail there, but does not select the new mail). I had the impression that adding code for selecting the new mail might not be the correct thing either, especially since the MailNews JS code might not be initialized yet at that point (see again Comment 5 for that). Also Thunderbird might not want changes there since it uses another message selection method at http://hg.mozilla.org/comm-central/annotate/c03aec453327/mail/base/content/mailWidgets.xml#l2643 right before executing the observe/AlertClicked code.
Comment 8 Frank Wein [:mcsmurf] 2013-04-28 10:11:31 PDT
Created attachment 742838 [details] [diff] [review]
Patch v2

This patch is better, the unix changes were not required.
Comment 9 Frank Wein [:mcsmurf] 2013-05-02 05:37:19 PDT
Comment on attachment 742838 [details] [diff] [review]
Patch v2

Karsten: Can you check if this patch works on OS X? As in: Do mail notification (the new ones which display subject plus first line of mail body) still work as before? Check especially if the MailNews window comes to foreground in different situations (as in: Browser window is foreground or no SeaMonkey window is in foreground). As this patch changes the MailNews window gets focus when clicking on the notification.
Comment 10 Frank Wein [:mcsmurf] 2013-05-02 05:38:54 PDT
Make that: "As this patch changes how the MailNews window"
Comment 11 Karsten Düsterloh 2013-05-05 15:49:51 PDT
Comment on attachment 742838 [details] [diff] [review]
Patch v2

(In reply to Frank Wein [:mcsmurf] from comment #9)
> Karsten: Can you check if this patch works on OS X? As in: Do mail
> notification (the new ones which display subject plus first line of mail
> body) still work as before?

We do not have such notifications under OS X. (I only recalled after looking at bug 404580, sorry.) I see no changes wrt the notification behaviour else.
Comment 12 Frank Wein [:mcsmurf] 2013-05-05 15:53:54 PDT
Karsten: But new mail notifications exist in general on OS X (which you can click on and MailNews opens)? After all I changed some code in nsMessengerOSXIntegration.mm which looks like it opens the MailNews window :) or is this code not in use then?
Comment 13 Karsten Düsterloh 2013-05-05 22:47:53 PDT
(In reply to Frank Wein [:mcsmurf] from comment #12)
> But new mail notifications exist in general on OS X

Yes, of course. By "such" I only meant "the new ones".

> (which you can click on and MailNews opens)?

Exactly.
The notification link basically says that you have n new mails and the link itself opens the respective Inbox (afaict).
Comment 14 Frank Wein [:mcsmurf] 2013-05-06 23:38:18 PDT
Created attachment 746255 [details] [diff] [review]
Disable new mail alert for SeaMonkey 2.18 release

Urgent review needed, this will disable the new mail alert for the SeaMonkey 2.18 release. Looks like the real fix will not be ready in time for the release.
Comment 15 Philip Chee 2013-05-07 04:23:51 PDT
Comment on attachment 746255 [details] [diff] [review]
Disable new mail alert for SeaMonkey 2.18 release

r=me
Comment 16 Frank Wein [:mcsmurf] 2013-05-07 09:41:18 PDT
Comment on attachment 746255 [details] [diff] [review]
Disable new mail alert for SeaMonkey 2.18 release

[Approval Request Comment]
Regression caused by (bug #): Bug 404580
User impact if declined: Rather bad behavior of the new mail notification when no MailNews window is open
Testing completed (on m-c, etc.): Tested locally only as this patch is comm-beta only for now
Risk to taking this patch (and alternatives if risky): Few risk, this patch disables the new mail notification code with a pref; the old mail notification  code is still in place and will display the old mail notification then instead
String changes made by this patch: None
Comment 17 Frank Wein [:mcsmurf] 2013-05-10 14:19:09 PDT
Comment on attachment 746255 [details] [diff] [review]
Disable new mail alert for SeaMonkey 2.18 release

I got approval+ for comm-beta via IRC from Callek.
Comment 18 Frank Wein [:mcsmurf] 2013-05-10 14:30:33 PDT
Comment on attachment 746255 [details] [diff] [review]
Disable new mail alert for SeaMonkey 2.18 release

Pushed to beta: https://hg.mozilla.org/releases/comm-beta/rev/cbbf1213f2e5
Comment 19 rsx11m 2013-05-22 05:16:14 PDT
Note that bug 856454 will provide a UI for the associated preferences in the Notifications pane with SeaMonkey 2.20; if the new alert remains off until then, those will have to be hidden to avoid user confusion.

If mail.biff.show_new_alert will stay even after the new alert if finalized, it may be a good idea either way to let visibility of those prefs depend on its value. I'll be happy to take care of this in a new bug, but would like to get some feedback on what intentions and timelines are on the old vs. new alerts.
Comment 20 rsx11m 2013-05-22 08:25:52 PDT
Never mind - hiding the new options can be accomplished with a single statement, thus I've opened bug 874899 to handle this.
Comment 21 Frank Wein [:mcsmurf] 2013-06-18 08:39:39 PDT
Comment on attachment 746255 [details] [diff] [review]
Disable new mail alert for SeaMonkey 2.18 release

[Approval Request Comment]
Regression caused by (bug #): Bug 404580 in some way
User impact if declined: Two mailnews windows open when a user clicks on the new mail notification (when no MailNews is open)
Testing completed (on m-c, etc.): Tested for a while on last beta and locally, seems to work fine
Risk to taking this patch (and alternatives if risky): This just disables a new feature via a pref
String changes made by this patch: -

Need to disable this feature once again, I'll look into forwarding the review request to someone else and fixing the actual regression.
Comment 22 Frank Wein [:mcsmurf] 2013-06-19 17:45:09 PDT
Comment on attachment 746255 [details] [diff] [review]
Disable new mail alert for SeaMonkey 2.18 release

Pushed to beta for SeaMonkey 2.19 release: https://hg.mozilla.org/releases/comm-beta/rev/b701a8e4cbbd
Comment 23 Frank Wein [:mcsmurf] 2013-06-20 05:34:09 PDT
Comment on attachment 742838 [details] [diff] [review]
Patch v2

Can you review this SeaMonkey only code?
Comment 24 Mark Banner (:standard8) 2013-06-24 03:50:10 PDT
Comment on attachment 742838 [details] [diff] [review]
Patch v2

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

Sorry, I hadn't picked up this was suite only, otherwise I'd have commented earlier. Generally I'm not doing suite reviews, unless the patch is also fixing something in TB and has pretty much the same code/solution.
Comment 25 Karsten Düsterloh 2013-07-06 07:28:52 PDT
Comment on attachment 742838 [details] [diff] [review]
Patch v2

Since I don't have Windows, I can't test this in real substance — Linux isn't affected, and Mac builds with it and seems to work undisturbed …
Comment 26 Frank Wein [:mcsmurf] 2013-07-08 08:59:56 PDT
Ok, I did test Windows enough with this patch, so pushed to comm-centrl: https://hg.mozilla.org/comm-central/rev/3c6b34c52e81
Comment 27 Jens Hatlak (:InvisibleSmiley) 2013-07-11 13:18:23 PDT
Came here mainly from bug 874899...

Hmm, the situation is a bit confusing now. For 2.19 we disabled the pref and for 2.22 we fixed the underlying issue. 2.21 and 2.20 however are both affected, and we just released 2.20b1. Also there are no tracking request flags set.

If you want to take your time between landing on trunk and branches, fine. But at least we need to make sure we don't forget - that's what tracking flags are here for (granted, these need to be checked and triaged by releng, too). But even then regressing between the last stable release and the first next beta is not nice.
Comment 28 Frank Wein [:mcsmurf] 2013-07-13 12:15:15 PDT
Comment on attachment 742838 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Regression caused by (bug #): Bug 404580
User impact if declined: Multiple MailNews windows might open when using the MailNews mail notification
Testing completed (on m-c, etc.): Works fine on comm-central, baked since 8th of July on comm-central; I tested this patch for multiple weeks on Windows already, I also tested various situations on Linux, Karsten tested the patch on OS X (Comment 25), it worked fine on all platforms.
Risk to taking this patch (and alternatives if risky): Rather low risk as the patch has been tested on all platforms and baked for a bit on trunk
String changes made by this patch: None
Comment 29 rsx11m 2013-07-18 15:13:04 PDT
Just a friendly reminder that bug 894212 targets the SM 2.20 beta 2 release for next Monday, thus would be nice to get this addressed on comm-beta in time. :-"
Comment 30 Frank Wein [:mcsmurf] 2013-07-18 16:42:18 PDT
IanN: Can you also approve this patch here? :) I saw you were doing approvals.
Comment 31 Frank Wein [:mcsmurf] 2013-07-19 16:42:35 PDT
Comment on attachment 742838 [details] [diff] [review]
Patch v2

Can I land this patch on comm-beta (and comm-aurora)?
Comment 32 Justin Wood (:Callek) 2013-07-21 21:18:12 PDT
Comment on attachment 742838 [details] [diff] [review]
Patch v2

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

a-CLOSED_TREE=me
Comment 33 Frank Wein [:mcsmurf] 2013-07-22 01:42:36 PDT
Pushed patch to comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/03ef7a3ec698
Pushed patch to comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/56d3f3b98820
Comment 34 Frank Wein [:mcsmurf] 2013-07-22 01:43:04 PDT
Everything is fixed, clearing tracking-* flags.
Comment 35 rsx11m 2013-07-22 08:04:10 PDT
This has just missed the SEAMONKEY_2_20b2_BUILD1 tagging... :-(

Note You need to log in before you can comment on or make changes to this bug.