Closed Bug 756971 Opened 12 years ago Closed 12 years ago

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

Categories

(MailNews Core :: Composition, defect)

defect
Not set
normal

Tracking

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

RESOLVED FIXED
Thunderbird 15.0
Tracking Status
thunderbird15 + fixed
seamonkey2.12 --- fixed
seamonkey2.13 --- fixed

People

(Reporter: u331436, Assigned: hiro)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

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.
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
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
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
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 !
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 ;)
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
Blocks: 684865
It is in TB too, so some patch that touched both apps.
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.
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)
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. ;)
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.
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);
(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...
(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.
mail/base/content/msgHdrViewOverlay.js, mail/base/content/selectionsummaries.js and mail/base/content/mailWindowOverlay.js do expect null too.
Attached patch Fix for SM (obsolete) — Splinter Review
Attached patch Fix for SM (obsolete) — Splinter Review
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+
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?
(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.
fixed on trunk for TB - http://hg.mozilla.org/comm-central/rev/3b21da7e9eff
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
actually, I'm going to leave this open to make sure we handle the gloda case, if nothing else.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 ?
> 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 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).
Attached patch FixSplinter Review
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.
(In reply to Ludovic Hirlimann [:Usul] from comment #30)
> I don't get error messages , latest trunk.

Error console?
> 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
(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
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 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+
> 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
OS: Windows 7 → All
Hardware: x86_64 → All
Attachment 628274 [details] [diff] checked in to comm-central as https://hg.mozilla.org/comm-central/rev/7cbaee447a20
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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 → ---
No. Anything in Error console?
Is anybody looking at the SM patch? There are no review requests...
> 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.
Attached patch Fix for SMSplinter Review
(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
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?
(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
> 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);
    }
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?
> 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]
(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.
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.
(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 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+
https://hg.mozilla.org/comm-central/rev/59d9b916d310
Assignee: dbienvenu → hiikezoe
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → 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+
> 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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: