Last Comment Bug 756971 - unable to modify message title and/or to add/remove recipients when replying or replying to all (regression)
: unable to modify message title and/or to add/remove recipients when replying ...
Status: RESOLVED FIXED
: regression
Product: MailNews Core
Classification: Components
Component: Composition (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
: 758555 759728 760186 761190 (view as bug list)
Depends on:
Blocks: 684865
  Show dependency treegraph
 
Reported: 2012-05-21 00:33 PDT by u331436
Modified: 2012-07-16 09:37 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed
fixed


Attachments
fix for reply case in tb (1.12 KB, patch)
2012-05-21 10:02 PDT, David :Bienvenu
standard8: review+
Details | Diff | Splinter Review
Fix for SM (6.40 KB, patch)
2012-05-21 23:23 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review
Fix for SM (5.23 KB, patch)
2012-05-22 16:21 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review
Fix (1.32 KB, patch)
2012-05-30 02:05 PDT, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Splinter Review
Fix for SM (1.85 KB, patch)
2012-06-12 03:46 PDT, Hiroyuki Ikezoe (:hiro)
mnyromyr: review+
mnyromyr: superreview+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description u331436 2012-05-21 00:33:13 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 SeaMonkey/2.12a1
Build ID: 20120520003031

Steps to reproduce:

Since nightly 20120520 (possibly 20120519 ??), it is impossible to modify a message title or add/remove a recipient in the distribution list of a message when replying (reply or "reply to all" ).


Actual results:

The "recipient" panel is grayed out as well as the message title line.


Expected results:

Previous version of the nightly 2.12 permitted the modification, i.e. the recipient panel was not grayed out nor the message title.
Comment 1 u331436 2012-05-21 00:50:26 PDT
After some more testing, it appears that the STR are:
- Open SM2.12a1 (nightly 20120520)
- opem SM mail
- reply or reply to all for a 1st message
---> all works fine
- reply or reply to all for a 2nd message
---> Recipient panel and message title grayed out.

In safe mode: the same behavior is seen.

Note: mail server : IMAP
Comment 2 u331436 2012-05-21 00:51:30 PDT
Found these two errors in the console when testing, not sure if they are related to the bug: 
Timestamp: 5/21/2012 9:42:00 AM
Warning: Use of attributes' nodeValue attribute is deprecated. Use value instead.
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2839


Timestamp: 5/21/2012 9:45:44 AM
Error: TypeError: names.value[i] is null
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2105
Comment 3 Philip Chee 2012-05-21 03:45:08 PDT
Can you bisect and find the last build that works?

Nothing here looks suspect:
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2012-05-17&enddate=2012-05-21
Except  Bug 756726 - Fix build in Mailnews due to infallible hashtables.
Comment 4 u331436 2012-05-21 04:56:32 PDT
Hi Philip,

I can for now only test Windows builds:
20120510 --> Works
20120518 --> Fails
20120520 --> Fails

I should be able to test later today with Linux !
Comment 5 :aceman 2012-05-21 05:59:06 PDT
I can see it randomly on TB trunk, Win XP, when replying a msg in Local Folders or POP3. On one message it behaves differently when using Reply to sender compared to Reply to All.

The names.value[i] is null error does not always show, only when the recipient list is empty. That is not always the case. Recipients are filled in but are not editable.
The blocked message can be "Sent later" and then can be "Edited as new" fine.

But it seems it is not from the 2012-05-21 fixes of mine ;)
Comment 6 u331436 2012-05-21 06:25:36 PDT
Did a bit more testing, this time with Linux (x86_64):
20120514 --> works
20120515 --> fails

Seesm to narrow the window to http://hg.mozilla.org/comm-central/pushloghtml?startdate=2012-05-14&enddate=2012-05-15

Possible culprit ?? :  http://hg.mozilla.org/comm-central/rev/ff3e0d0b3973
Comment 8 :aceman 2012-05-21 08:35:13 PDT
It is in TB too, so some patch that touched both apps.
Comment 9 David :Bienvenu 2012-05-21 09:18:44 PDT
Yeah, I'm reasonably sure it's Bug 684865 that regressed this. My debug build hasn't finished, but I can reproduce this easily on a nightly build. I suspect it's the nulling of the out params that caused the regression.
Comment 10 David :Bienvenu 2012-05-21 10:02:49 PDT
Created attachment 625675 [details] [diff] [review]
fix for reply case in tb

this fixes it for tb, and I bet a similar fix would fix it for SM. 

However, I'm worried that there might be other regressions caused by not always returning values, even empty values, for fields not present. If we find more regressions, we should strongly consider fixing bug 684865 a different way.
Comment 11 Philip Chee 2012-05-21 10:46:04 PDT
David, what about?:
http://mxr.mozilla.org/comm-central/source/mail/base/content/selectionsummaries.js#133
http://mxr.mozilla.org/comm-central/source/mail/base/content/msgHdrViewOverlay.js#1131
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abMailListDialog.js#144
http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/utils.js#91

> However, I'm worried that there might be other regressions caused by not always
> returning values, even empty values, for fields not present. If we find more
> regressions, we should strongly consider fixing bug 684865 a different way.
I suggest that you consider fixing bug 684865 a different way. ;)
Comment 12 Philipp Kewisch [:Fallen] 2012-05-21 22:45:49 PDT
I also suggest fixing it differently. I wouldn't have expected the value to be null, but instead an empty array or such. The extra check could be avoided if an empty array is returned.
Comment 13 Hiroyuki Ikezoe (:hiro) 2012-05-21 22:48:27 PDT
I am sorry for the regression.

We need to change the codes in suite.

http://mxr.mozilla.org/comm-central/source/suite/mailnews/addrbook/abSelectAddressesDialog.js:

257 function AddAddressIntoBucket(prefix, address, email)
258 {
259   if (email == "")
260   {
261     Services.prompt.alert(window,
262                           gAddressBookBundle.getString("emptyEmailAddCardTitle"),
263                           gAddressBookBundle.getString("emptyEmailAddCard"));
264   }
265   else


and

http://mxr.mozilla.org/comm-central/source/suite/mailnews/compose/MsgComposeCommands.js:

2093     // break the list of potentially many recipients back into individual names
2094     var hdrParser = Components.classes["@mozilla.org/messenger/headerparser;1"].getService(Components.interfaces.nsIMsgHeaderParser);
2095     var emailAddresses = {};
2096     var names = {};
2097     var fullNames = {};
2098     var numAddresses = hdrParser.parseHeadersWithArray(aAddressesToAdd, emailAddresses, names, fullNames);
2099     var tokenizedNames = [];
2100 
2101     // each name could consist of multiple words delimited by commas and/or spaces.
2102     // i.e. Green Lantern or Lantern,Green.
2103     for (let i = 0; i < names.value.length; i++)
2104     {


But unfortunately there are also the codes expecting null.

http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWindowOverlay.js:

2708   numAddresses = headerParser.parseHeadersWithArray(msgHdr.author, addresses, names, fullNames);
2709   var authorEmailAddress = addresses.value[0];
2710   if (!authorEmailAddress)
2711     return;

http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWidgets.xml:

2064                 var numAddresses = hdrParser.parseHeadersWithArray(msgHdr.mime2DecodedAuthor, emails, names, {});
2065                 msgPopup.setAttribute('sender', names.value[0] ? names.value[0] : emails.value[0]);
2066                 msgPopup.messageUri = aFolder.getUriForMsg(msgHdr);


http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWidgets.xml:

1113 function updateEmailAddressNode(emailAddressNode, address)
1114 {
1115   emailAddressNode.setAttribute("label", address.fullAddress || address.displayName);
Comment 14 Hiroyuki Ikezoe (:hiro) 2012-05-21 22:50:51 PDT
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWidgets.xml:
> 
> 1113 function updateEmailAddressNode(emailAddressNode, address)
> 1114 {
> 1115   emailAddressNode.setAttribute("label", address.fullAddress ||
> address.displayName);

The last one is not related...
Comment 15 Jim Porter (:squib) 2012-05-21 22:53:32 PDT
(In reply to David :Bienvenu from comment #10)
> Created attachment 625675 [details] [diff] [review]
> fix for reply case in tb
> 
> this fixes it for tb, and I bet a similar fix would fix it for SM. 
> 
> However, I'm worried that there might be other regressions caused by not
> always returning values, even empty values, for fields not present. If we
> find more regressions, we should strongly consider fixing bug 684865 a
> different way.

I'm seeing something similar in bug 671530, which predates the regressing patch, but I think what protz and I see is different from the original report. I also posted a similar (but not identical) patch over there.
Comment 16 Hiroyuki Ikezoe (:hiro) 2012-05-21 22:55:03 PDT
mail/base/content/msgHdrViewOverlay.js, mail/base/content/selectionsummaries.js and mail/base/content/mailWindowOverlay.js do expect null too.
Comment 17 Hiroyuki Ikezoe (:hiro) 2012-05-21 23:23:23 PDT
Created attachment 625903 [details] [diff] [review]
Fix for SM
Comment 18 Hiroyuki Ikezoe (:hiro) 2012-05-22 16:21:38 PDT
Created attachment 626237 [details] [diff] [review]
Fix for SM

The previous patch includes unrelated diff.
Comment 19 Mark Banner (:standard8) 2012-05-24 13:02:31 PDT
Comment on attachment 625675 [details] [diff] [review]
fix for reply case in tb

Looks fine, we'll probably want the gloda changes from the other patch as well, but this will unbreak the main case.
Comment 20 Hiroyuki Ikezoe (:hiro) 2012-05-24 15:31:23 PDT
Comment on attachment 625675 [details] [diff] [review]
fix for reply case in tb

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3049,5 @@
>      var names = {};
>      var fullNames = {};
>      var numAddresses = hdrParser.parseHeadersWithArray(aAddressesToAdd, emailAddresses, names, fullNames);
> +    if (!names)
> +      return;

I suppose parseHeadersWithArray returns array including null like this:
names = {
  value: [ "John", null, "Mike" ]
}

Does the code work with these names? Am I wrong?
Comment 21 David :Bienvenu 2012-05-24 15:40:00 PDT
(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)

> I suppose parseHeadersWithArray returns array including null like this:
> names = {
>   value: [ "John", null, "Mike" ]
> }
> 
> Does the code work with these names? Am I wrong?

names are optional, e-mail addresses are not, so in the real world, the issue is missing names. But I'm still thinking we should fix the regressing bug a different way as well. But this patch makes things a bit more stable for now.
Comment 22 David :Bienvenu 2012-05-24 16:21:45 PDT
fixed on trunk for TB - http://hg.mozilla.org/comm-central/rev/3b21da7e9eff
Comment 23 David :Bienvenu 2012-05-24 16:23:51 PDT
actually, I'm going to leave this open to make sure we handle the gloda case, if nothing else.
Comment 24 Mark Banner (:standard8) 2012-05-25 03:29:02 PDT
*** Bug 758555 has been marked as a duplicate of this bug. ***
Comment 25 u331436 2012-05-29 07:50:30 PDT
Adding one more comment:
The window title of the message is(no subject), although the message replied to has a subject. This one is not picked up when drawing the window.
Comment 26 Ludovic Hirlimann [:Usul] 2012-05-29 22:48:39 PDT
(In reply to David :Bienvenu from comment #23)
> actually, I'm going to leave this open to make sure we handle the gloda
> case, if nothing else.

So this morning I tried to reply to  bugzilla email and got the same symptoms as bug 758555 - shall I reopen it - or is this bug not totally fixed ?
Comment 27 Philip Chee 2012-05-30 01:38:38 PDT
> So this morning I tried to reply to  bugzilla email and got the same symptoms as
> bug 758555 - shall I reopen it - or is this bug not totally fixed ?
Error message? Date of your trunk build/Build ID?
Comment 28 :aceman 2012-05-30 01:59:36 PDT
I confirm the problem still existing in TB 20120529035024, reply all to a message in Local Folders. The error message as in comment 2, just different line (maybe due to being TB):
Timestamp: 30.5.2012 10:54:10
Error: TypeError: names.value[name] is null
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 3060

Clicking the error shows the source code and the function DOES contain the patch created here (the two line checking for !names).
Comment 29 Hiroyuki Ikezoe (:hiro) 2012-05-30 02:05:46 PDT
Created attachment 628274 [details] [diff] [review]
Fix

As I wrote in comment #20, we should also check each value of array.
Comment 30 Ludovic Hirlimann [:Usul] 2012-05-30 06:34:44 PDT
(In reply to Philip Chee from comment #27)
> > So this morning I tried to reply to  bugzilla email and got the same symptoms as
> > bug 758555 - shall I reopen it - or is this bug not totally fixed ?
> Error message? Date of your trunk build/Build ID?

I don't get error messages , latest trunk.
Comment 31 :aceman 2012-05-30 06:39:39 PDT
(In reply to Ludovic Hirlimann [:Usul] from comment #30)
> I don't get error messages , latest trunk.

Error console?
Comment 32 Ludovic Hirlimann [:Usul] 2012-05-30 07:00:35 PDT
*** Bug 759728 has been marked as a duplicate of this bug. ***
Comment 33 Philip Chee 2012-05-30 08:37:33 PDT
> I don't get error messages , latest trunk.
If you are not getting any error messages in the Error Console then your problem is some other bug.
Comment 34 Ludovic Hirlimann [:Usul] 2012-05-30 08:40:07 PDT
Timestamp: 5/30/12 5:39:25 PM
Error: TypeError: names.value[name] is null
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 3060
Comment 35 David :Bienvenu 2012-05-30 15:01:57 PDT
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> Created attachment 628274 [details] [diff] [review]
> Fix
> 
> As I wrote in comment #20, we should also check each value of array.

I really think we should back out the fix for bug 684865 and replace the backout with what I suggesed here - https://bugzilla.mozilla.org/show_bug.cgi?id=684865#c15
Comment 36 Jack Eidsness 2012-05-31 10:39:32 PDT
*** Bug 760186 has been marked as a duplicate of this bug. ***
Comment 37 Jack Eidsness 2012-05-31 11:48:25 PDT
I have found that switching between HTML and plain text composition modes seems to workaround the problem for some reason. I click reply, and it works once, then I shift click reply, and it works once. I have to keep alternating between them to avoid the problem. If this seems like a different problem, then I can re-open my duplicate.
Comment 38 David :Bienvenu 2012-06-02 10:13:53 PDT
Comment on attachment 628274 [details] [diff] [review]
Fix

thx for the patch, it looks ok - I'm still thinking we should back out the original patch so we don't get hit by more regressions and fix the unix mail alert in the unix mail alert code...
Comment 39 Philip Chee 2012-06-02 11:06:03 PDT
> I'm still thinking we should back out the original patch
> so we don't get hit by more regressions and fix the unix mail alert in the unix
> mail alert code...
Yes, we should really.
Comment 40 Ludovic Hirlimann [:Usul] 2012-06-04 00:44:15 PDT
chekin needed for attachment 628274 [details] [diff] [review]
Comment 41 Mike Conley (:mconley) - (needinfo me!) 2012-06-04 11:08:20 PDT
Attachment 628274 [details] [diff] checked in to comm-central as https://hg.mozilla.org/comm-central/rev/7cbaee447a20
Comment 42 [:Aureliano Buendía] 2012-06-11 06:29:10 PDT
*** Bug 761190 has been marked as a duplicate of this bug. ***
Comment 43 u331436 2012-06-12 00:31:23 PDT
Not fixed in the SM nightly 20120611 (windows).
I am still seeing the issue:
When replying to an Email message:
- unable to add/remove any recipient
- unable to edit the subject line
- Window title is Compose: (no subject) 

Anyone else still seeing this ?
Comment 44 :aceman 2012-06-12 00:47:30 PDT
No. Anything in Error console?
Comment 45 Hiroyuki Ikezoe (:hiro) 2012-06-12 02:08:52 PDT
SM needs attachment 626237 [details] [diff] [review].
Comment 46 :aceman 2012-06-12 02:51:40 PDT
Is anybody looking at the SM patch? There are no review requests...
Comment 47 Philip Chee 2012-06-12 03:41:29 PDT
> Is anybody looking at the SM patch? There are no review requests...
Well first a new patch needs to be created without the Thunderbird and /mailnews/ parts.
Comment 48 Hiroyuki Ikezoe (:hiro) 2012-06-12 03:46:06 PDT
Created attachment 632193 [details] [diff] [review]
Fix for SM

(In reply to Philip Chee from comment #47)
> > Is anybody looking at the SM patch? There are no review requests...
> Well first a new patch needs to be created without the Thunderbird and
> /mailnews/ parts.

I am sorry for the mistake.
Comment 49 :aceman 2012-06-12 03:47:28 PDT
Then this bug should probably not have been closed when it is not yet finished (it is in Mailnews Core).
Philip, who can review it?
Comment 50 u331436 2012-06-12 04:04:12 PDT
(In reply to :aceman from comment #44)
> No. Anything in Error console?

Yes, here it is:
Timestamp: 6/12/2012 1:02:48 PM
Error: TypeError: names.value[i] is null
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2071
Comment 51 Philip Chee 2012-06-12 20:11:28 PDT
> Philip, who can review it?
Proably Mnyromyr or IanN

>      for (let i = 0; i < names.value.length; i++)
>      {
> +      if (!names.value[i])
> +        continue;

I think we can do something like:

    for (let name of names.value)
    {
      var splitNames = name ? name.match(/[^\s,]+/g) : null;
      if (splitNames)
        tokenizedNames = tokenizedNames.concat(splitNames);
    }
Comment 52 neil@parkwaycc.co.uk 2012-06-13 02:33:56 PDT
(In reply to Philip Chee from comment #51)
>       var splitNames = name ? name.match(/[^\s,]+/g) : null;
name && name.match(/[^\s,]+/g) perhaps?
Comment 53 Philipp Kewisch [:Fallen] 2012-06-13 04:21:44 PDT
(In reply to Philip Chee from comment #51)
> > Philip, who can review it?
> Proably Mnyromyr or IanN
> 
> >      for (let i = 0; i < names.value.length; i++)
> >      {
> > +      if (!names.value[i])
> > +        continue;
> 
> I think we can do something like:
> 
>     for (let name of names.value)
>     {
>       var splitNames = name ? name.match(/[^\s,]+/g) : null;
>       if (splitNames)
>         tokenizedNames = tokenizedNames.concat(splitNames);
>     }

iirc then its possible that not only names.value is null but also names itself. Am I under a wrong impression?
Comment 54 Philip Chee 2012-06-13 08:34:59 PDT
> iirc then its possible that not only names.value is null but also names itself. Am I
> under a wrong impression?

I think you're right.
https://hg.mozilla.org/comm-central/rev/83a7ea3d40c2#l1.12
>     1.12 +  *aOutgoingFullName = nsnull;
>     1.13 +  *aOutgoingEmailAddress = nsnull;
>     1.14 +  *aOutgoingName = nsnull;

David fixed it in attachment 625675 [details] [diff] [review]
Comment 55 Hiroyuki Ikezoe (:hiro) 2012-06-13 14:07:00 PDT
(In reply to Philip Chee from comment #54)
> > iirc then its possible that not only names.value is null but also names itself. Am I
> > under a wrong impression?
> 
> I think you're right.
> https://hg.mozilla.org/comm-central/rev/83a7ea3d40c2#l1.12
> >     1.12 +  *aOutgoingFullName = nsnull;
> >     1.13 +  *aOutgoingEmailAddress = nsnull;
> >     1.14 +  *aOutgoingName = nsnull;
> 
> David fixed it in attachment 625675 [details] [diff] [review]

The part is each value in the array.

112 NS_IMETHODIMP nsMsgHeaderParser::ParseHeadersWithArray(const PRUnichar * aLine, PRUnichar *** aEmailAddresses,
113                                                        PRUnichar *** aNames, PRUnichar *** aFullNames, PRUint32 * aNumAddresses)
114 {
115   char * names = nsnull;
116   char * addresses = nsnull;
117   PRUint32 numAddresses = 0;
118   nsresult rv = NS_OK;
119 
120   // need to convert unicode to UTF-8...
121   nsAutoString tempString (aLine);
122   char * utf8String = ToNewUTF8String(tempString);
123 
124   rv = ParseHeaderAddresses(utf8String, &names, &addresses, &numAddresses);
125   NS_Free(utf8String);
126   if (NS_SUCCEEDED(rv) && numAddresses)
127   {
128     // allocate space for our arrays....
129     *aEmailAddresses = (PRUnichar **) PR_MALLOC(sizeof(PRUnichar *) * numAddresses);
130     *aNames = (PRUnichar **) PR_MALLOC(sizeof(PRUnichar *) * numAddresses);
131     *aFullNames = (PRUnichar **) PR_MALLOC(sizeof(PRUnichar *) * numAddresses);
132 

names is always allocated here.
Comment 56 Hiroyuki Ikezoe (:hiro) 2012-06-13 16:00:38 PDT
I found another risk there.

163 NS_IMETHODIMP
164 nsMsgHeaderParser::ParseHeaderAddresses(const char *aLine, char **aNames,
165                                         char **aAddresses,
166                                         PRUint32 *aNumAddresses)
167 {
168   NS_ENSURE_ARG_POINTER(aNumAddresses);
169   *aNumAddresses = msg_parse_Header_addresses(aLine, aNames, aAddresses);
170   return NS_OK;
171 }

ParseHeaderAddresses always returns NS_OK even if msg_parse_Header_addresses returns negative value (it means error), and unfortunately *aNumAddresses is an unsigned value...

We also need to fix it.
Comment 57 Hiroyuki Ikezoe (:hiro) 2012-06-13 16:05:53 PDT
(In reply to Hiroyuki Ikezoe (:hiro) from comment #56)

> ParseHeaderAddresses always returns NS_OK even if msg_parse_Header_addresses
> returns negative value (it means error), and unfortunately *aNumAddresses is
> an unsigned value...
> 
> We also need to fix it.

I've opened bug 764637.
Comment 58 Karsten Düsterloh 2012-06-17 11:30:36 PDT
Comment on attachment 632193 [details] [diff] [review]
Fix for SM

r/moa=me
Comment 59 Ryan VanderMeulen [:RyanVM] 2012-06-18 15:51:14 PDT
https://hg.mozilla.org/comm-central/rev/59d9b916d310
Comment 60 Mark Banner (:standard8) 2012-07-16 09:19:29 PDT
Comment on attachment 632193 [details] [diff] [review]
Fix for SM

a+=Callek via irc.
Comment 61 Philip Chee 2012-07-16 09:37:55 PDT
> a+=Callek via irc.
> Attachment #632193 [details] [diff] - Flags: approval-comm-aurora? → approval-comm-aurora+

Pushed to comm-central:
http://hg.mozilla.org/releases/comm-aurora/rev/d269e7c0fb80

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