unable to modify message title and/or to add/remove recipients when replying or replying to all (regression)

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Composition
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: u331436, Assigned: hiro)

Tracking

({regression})

Trunk
Thunderbird 15.0
regression

Thunderbird Tracking Flags

(thunderbird15+ fixed, seamonkey2.12 fixed, seamonkey2.13 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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
(Reporter)

Comment 2

5 years ago
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

5 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
(Reporter)

Comment 4

5 years ago
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

5 years ago
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 ;)
(Reporter)

Comment 6

5 years ago
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 7

5 years ago
Pushlog:
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2012-05-10&enddate=2012-05-18

I wonder if it's this:
http://hg.mozilla.org/comm-central/rev/83a7ea3d40c2
Bug 684865 - Do not return 0-length strings. r=mbanner

This patch touches parseHeadersWithArray which addRecipientsToIgnoreList() uses.

http://hg.mozilla.org/comm-central/annotate/58044311b637/mail/components/compose/content/MsgComposeCommands.js#l3051

http://hg.mozilla.org/comm-central/annotate/58044311b637/suite/mailnews/compose/MsgComposeCommands.js#l2098

Someone really ought to audit all callers of parseHeadersWithArray()
http://mxr.mozilla.org/comm-central/ident?i=parseHeadersWithArray

Updated

5 years ago
Blocks: 684865

Comment 8

5 years ago
It is in TB too, so some patch that touched both apps.

Comment 9

5 years ago
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

5 years ago
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.
Assignee: nobody → dbienvenu
Attachment #625675 - Flags: review?(mbanner)

Comment 11

5 years ago
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. ;)
tracking-seamonkey2.12: --- → ?
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.
(Assignee)

Comment 13

5 years ago
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);
(Assignee)

Comment 14

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
mail/base/content/msgHdrViewOverlay.js, mail/base/content/selectionsummaries.js and mail/base/content/mailWindowOverlay.js do expect null too.
(Assignee)

Comment 17

5 years ago
Created attachment 625903 [details] [diff] [review]
Fix for SM
(Assignee)

Comment 18

5 years ago
Created attachment 626237 [details] [diff] [review]
Fix for SM

The previous patch includes unrelated diff.
Attachment #625903 - Attachment is obsolete: true
Component: MailNews: Composition → Composition
Product: SeaMonkey → MailNews Core
QA Contact: mailnews-composition → composition
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.
Attachment #625675 - Flags: review?(mbanner) → review+
(Assignee)

Comment 20

5 years ago
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

5 years ago
(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

5 years ago
fixed on trunk for TB - http://hg.mozilla.org/comm-central/rev/3b21da7e9eff
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0

Comment 23

5 years ago
actually, I'm going to leave this open to make sure we handle the gloda case, if nothing else.
Status: RESOLVED → REOPENED
status-thunderbird15: --- → affected
tracking-thunderbird15: --- → +
Resolution: FIXED → ---
Duplicate of this bug: 758555
(Reporter)

Comment 25

5 years ago
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.
(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

5 years ago
> 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

5 years ago
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).
(Assignee)

Comment 29

5 years ago
Created attachment 628274 [details] [diff] [review]
Fix

As I wrote in comment #20, we should also check each value of array.
Attachment #628274 - Flags: review?(dbienvenu)
(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

5 years ago
(In reply to Ludovic Hirlimann [:Usul] from comment #30)
> I don't get error messages , latest trunk.

Error console?
Duplicate of this bug: 759728

Comment 33

5 years ago
> 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.
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

5 years ago
(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

Updated

5 years ago
Duplicate of this bug: 760186

Comment 37

5 years ago
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

5 years ago
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...
Attachment #628274 - Flags: review?(dbienvenu) → review+

Comment 39

5 years ago
> 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.
Status: REOPENED → ASSIGNED

Updated

5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
chekin needed for attachment 628274 [details] [diff] [review]
Keywords: checkin-needed
Attachment 628274 [details] [diff] checked in to comm-central as https://hg.mozilla.org/comm-central/rev/7cbaee447a20
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

5 years ago
Duplicate of this bug: 761190
(Reporter)

Comment 43

5 years ago
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 ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 44

5 years ago
No. Anything in Error console?
(Assignee)

Comment 45

5 years ago
SM needs attachment 626237 [details] [diff] [review].

Comment 46

5 years ago
Is anybody looking at the SM patch? There are no review requests...

Comment 47

5 years ago
> 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.
(Assignee)

Comment 48

5 years ago
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.
Attachment #626237 - Attachment is obsolete: true

Comment 49

5 years ago
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?
(Reporter)

Comment 50

5 years ago
(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

5 years ago
> 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);
    }

Updated

5 years ago
Attachment #632193 - Flags: review?(mnyromyr)
(In reply to Philip Chee from comment #51)
>       var splitNames = name ? name.match(/[^\s,]+/g) : null;
name && name.match(/[^\s,]+/g) perhaps?
(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

5 years ago
> 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]
(Assignee)

Comment 55

5 years ago
(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.
(Assignee)

Comment 56

5 years ago
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.
(Assignee)

Comment 57

5 years ago
(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

5 years ago
Comment on attachment 632193 [details] [diff] [review]
Fix for SM

r/moa=me
Attachment #632193 - Flags: superreview+
Attachment #632193 - Flags: review?(mnyromyr)
Attachment #632193 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/59d9b916d310
Assignee: dbienvenu → hiikezoe
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
status-seamonkey2.12: --- → affected
status-seamonkey2.13: --- → fixed
status-thunderbird15: affected → fixed
Attachment #632193 - Flags: approval-comm-aurora?
Comment on attachment 632193 [details] [diff] [review]
Fix for SM

a+=Callek via irc.
Attachment #632193 - Flags: approval-comm-aurora? → approval-comm-aurora+

Comment 61

5 years ago
> 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
status-seamonkey2.12: affected → fixed
tracking-seamonkey2.12: ? → ---
You need to log in before you can comment on or make changes to this bug.