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)
MailNews Core
Composition
Tracking
(thunderbird15+ fixed, seamonkey2.12 fixed, seamonkey2.13 fixed)
RESOLVED
FIXED
Thunderbird 15.0
People
(Reporter: u331436, Assigned: hiro)
References
Details
(Keywords: regression)
Attachments
(3 files, 2 obsolete files)
1.12 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
1.32 KB,
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
1.85 KB,
patch
|
mnyromyr
:
review+
mnyromyr
:
superreview+
standard8
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
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
Comment 3•12 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.
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
Comment 7•12 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
Comment 9•12 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•12 years ago
|
||
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•12 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. ;)
Updated•12 years ago
|
tracking-seamonkey2.12:
--- → ?
Comment 12•12 years ago
|
||
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•12 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•12 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•12 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•12 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•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
The previous patch includes unrelated diff.
Attachment #625903 -
Attachment is obsolete: true
Updated•12 years ago
|
Component: MailNews: Composition → Composition
Product: SeaMonkey → MailNews Core
QA Contact: mailnews-composition → composition
Comment 19•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
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
Comment 23•12 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 → ---
Reporter | ||
Comment 25•12 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.
Comment 26•12 years ago
|
||
(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•12 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•12 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•12 years ago
|
||
As I wrote in comment #20, we should also check each value of array.
Attachment #628274 -
Flags: review?(dbienvenu)
Comment 30•12 years ago
|
||
(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•12 years ago
|
||
(In reply to Ludovic Hirlimann [:Usul] from comment #30) > I don't get error messages , latest trunk. Error console?
Comment 33•12 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.
Comment 34•12 years ago
|
||
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•12 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
Comment 37•12 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•12 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•12 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•12 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
Comment 40•12 years ago
|
||
chekin needed for attachment 628274 [details] [diff] [review]
Keywords: checkin-needed
Comment 41•12 years ago
|
||
Attachment 628274 [details] [diff] checked in to comm-central as https://hg.mozilla.org/comm-central/rev/7cbaee447a20
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Reporter | ||
Comment 43•12 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•12 years ago
|
||
No. Anything in Error console?
Assignee | ||
Comment 45•12 years ago
|
||
SM needs attachment 626237 [details] [diff] [review].
Comment 46•12 years ago
|
||
Is anybody looking at the SM patch? There are no review requests...
Comment 47•12 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•12 years ago
|
||
(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•12 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•12 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•12 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•12 years ago
|
Attachment #632193 -
Flags: review?(mnyromyr)
Comment 52•12 years ago
|
||
(In reply to Philip Chee from comment #51) > var splitNames = name ? name.match(/[^\s,]+/g) : null; name && name.match(/[^\s,]+/g) perhaps?
Comment 53•12 years ago
|
||
(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•12 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•12 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•12 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•12 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•12 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+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 59•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/59d9b916d310
Assignee: dbienvenu → hiikezoe
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•12 years ago
|
status-seamonkey2.12:
--- → affected
status-seamonkey2.13:
--- → fixed
Updated•12 years ago
|
Attachment #632193 -
Flags: approval-comm-aurora?
Comment 60•12 years ago
|
||
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•12 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
tracking-seamonkey2.12:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•