Last Comment Bug 397021 - Subject line <foo@bar> in the forward inline body not shown in html composition mode
: Subject line <foo@bar> in the forward inline body not shown in html compositi...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Jonathan Kamens
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-21 01:51 PDT by Jonathan Kamens
Modified: 2015-09-17 04:47 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (5.70 KB, patch)
2010-07-25 07:32 PDT, Jonathan Kamens
bugmail: review-
Details | Diff | Splinter Review
mozmill test (2.15 KB, patch)
2012-03-17 05:40 PDT, Magnus Melin
no flags Details | Diff | Splinter Review
Consolidated patch (16.22 KB, patch)
2012-03-27 09:43 PDT, Jonathan Kamens
no flags Details | Diff | Splinter Review
Consolidated patch without repetition (8.10 KB, patch)
2012-03-28 08:00 PDT, Jonathan Kamens
mozilla: review+
Details | Diff | Splinter Review
Consolidated patch that uses nsnull instead of NULL (8.10 KB, patch)
2012-03-28 10:09 PDT, Jonathan Kamens
jik: review+
Details | Diff | Splinter Review
Use MsgEscapeHTML (1.37 KB, patch)
2012-03-30 13:15 PDT, neil@parkwaycc.co.uk
mozilla: review+
Details | Diff | Splinter Review

Description Jonathan Kamens 2007-09-21 01:51:43 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.6) Gecko/20070911 Fedora/2.0.0.6-8.fc8 Firefox/2.0.0.6
Build Identifier: version 2.0.0.6 (20070911) from Fedora Rawhide

If I create a message containing subject"<foo@bar>" in the subject, send it to myself, and then try to forward it, the "<foo@bar>" is stripped from the subject.  It's just wrong for thunderbird to be mucking with subjects like that.


Reproducible: Always
Comment 1 Magnus Melin 2007-09-27 12:18:39 PDT
I assume you mean in the body where the --- Original Message --- stuff is. Confirming on linux/trunk.
Comment 2 Magnus Melin 2007-09-27 12:21:18 PDT
Or rather, it's just not shown (in html mode) as the < > are not encoded as &lt; and &gt; -- or so I assume as the plain text composition works fine.
Comment 3 Jonathan Kamens 2010-07-25 07:32:38 PDT
Created attachment 460147 [details] [diff] [review]
proposed fix

Thanks for fixing this bug so quickly. :-)
Patch attached.
Comment 4 Ludovic Hirlimann [:Usul] 2010-07-26 01:58:51 PDT
(In reply to comment #3)
> Created attachment 460147 [details] [diff] [review]
> proposed fix
> 
> Thanks for fixing this bug so quickly. :-)
> Patch attached.

You need to request reviews if you want you code to get in see https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements.
Comment 5 Andrew Sutherland [:asuth] 2010-07-26 02:35:53 PDT
Comment on attachment 460147 [details] [diff] [review]
proposed fix

Ludo, I'm not a super-reviewer although I've heard talk of trying to make that happen.

But I can chime in here as a reviewer and say that this definitely needs at least a unit test confirming the change in behaviour.  If there are not already unit tests covering the cases where the removed/modified logic was supposed to be doing something helpful, test cases should probably be added for that too.  (Please call out the existing tests in a buzilla comment if they already exist.)

Sorta helpful testing link:
https://developer.mozilla.org/en/MailNews_xpcshell-tests

bienvenu is likely the right person to ask for review from once tests are created or if help is required in creating tests.  I don't think super-review is required for such a patch, but if it is, bienvenu can give it.
Comment 6 Ludovic Hirlimann [:Usul] 2010-07-26 04:16:18 PDT
(In reply to comment #3)
> Created attachment 460147 [details] [diff] [review]
> proposed fix
> 
> Thanks for fixing this bug so quickly. :-)
> Patch attached.

Jonathan , you'll need to add some unit test (I forgot to look for these). If you need help with these please ask either here or on our mailing-lists or in #maildev on irc.mozilla.org.
Comment 7 Jonathan Kamens 2010-07-26 04:39:05 PDT
The learning curve for figuring out how to debug and fix Thunderbird issues was steep enough. Unfortunately, I do not have the additional time that would be necessary to get over the undoubtedly steep learning curve that would also be necessary to write a unit test for something like this.
One would hope that if a user of the software gave the time and energy to submit a fix, the developers of the software would meet the user halfway and write the unit test. Especially given that this bug has sat unfixed for almost three years. In short, can I please get a little help here?
I've owned free software projects for much longer than Mozilla has existed, and I've never asked a user who contributed a fix to write a unit test for it.
Comment 8 Jonathan Kamens 2010-10-07 19:29:09 PDT
Is there no one willing to meet me halfway here and write a unit test for this bug? I'd really appreciate the help.
Comment 9 Jonathan Kamens 2011-07-27 06:47:23 PDT
Since posting comment #7 and comment #8, I've learned how to build and patch TB and FF and how to work with the unit tests for both, so I'll write my own unit test for this bug when I get a chance. That may be a while, though -- too many all-nighters fixing other TB bugs have left me with a work deficit I need to address -- so if anyone else stumbles across this bug and wants to pitch in by writing the unit test, I wouldn't say no.
Comment 10 Magnus Melin 2012-03-17 05:40:37 PDT
Created attachment 606859 [details] [diff] [review]
mozmill test

Here's a mozmill test for it, you'd run it using 
make SOLO_TEST=composition/test-forwarded-content.js mozmill-one
Comment 11 Jonathan Kamens 2012-03-27 09:43:00 PDT
Created attachment 609755 [details] [diff] [review]
Consolidated patch

Thank you Magnus! Here is a single consolidated patch that incorporates my fix and Magnus's mozmill test. Review?
Comment 12 David :Bienvenu 2012-03-28 07:49:34 PDT
Comment on attachment 609755 [details] [diff] [review]
Consolidated patch

thx for the patch! The mime parts of the patch seem to have bit-rotted - can you attach a de-bitrotted patch and re-request review? thx!
Comment 13 Jonathan Kamens 2012-03-28 08:00:53 PDT
Created attachment 610139 [details] [diff] [review]
Consolidated patch without repetition

Apparently "hg export -o" appends instead of overwriting. D'oh! Updated patch attached.
Comment 14 David :Bienvenu 2012-03-28 08:44:29 PDT
Comment on attachment 610139 [details] [diff] [review]
Consolidated patch without repetition

thx for the patch, looks reasonable - can you use nsnull instead of NULL?
Comment 15 Jonathan Kamens 2012-03-28 10:09:14 PDT
Created attachment 610180 [details] [diff] [review]
Consolidated patch that uses nsnull instead of NULL

Same as attachment 610139 [details] [diff] [review] which is already review+. The only change is to replace NULL with nsnull as requested by the reviewer. I've confirmed that it builds with the nsnull change, so I'm carrying forward the review+.
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-03-28 17:35:19 PDT
http://hg.mozilla.org/comm-central/rev/30c2149ba887
Comment 17 neil@parkwaycc.co.uk 2012-03-30 13:08:44 PDT
Bah, you're supposed to use MsgEscapeHTML, as per line 1435... patch coming up.
Comment 18 neil@parkwaycc.co.uk 2012-03-30 13:15:07 PDT
Created attachment 610985 [details] [diff] [review]
Use MsgEscapeHTML
Comment 19 Jonathan Kamens 2012-03-30 13:47:22 PDT
Sorry, didn't know.

Why do we have two functions that do exactly the same thing?
Comment 20 David :Bienvenu 2012-03-30 14:35:40 PDT
Comment on attachment 610985 [details] [diff] [review]
Use MsgEscapeHTML

Neil's talking about supporting frozen linkage, which is a different way of building where only some interfaces are supported.
Comment 21 neil@parkwaycc.co.uk 2012-03-31 14:16:01 PDT
Comment on attachment 610985 [details] [diff] [review]
Use MsgEscapeHTML

Pushed changeset b1b2d61638f5 to comm-central.

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