Closed Bug 1123062 Opened 5 years ago Closed 5 years ago

In answers to plain text news articles or emails (without format=flowed!?) the line breaks are removed from the quotations.

Categories

(Core :: DOM: Serializers, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox35 --- unaffected
firefox36 --- unaffected
firefox37 + fixed
firefox38 + fixed

People

(Reporter: infofrommozilla, Assigned: ehsan)

References

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

Attached file broken_quote.eml (obsolete) —
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150109190446

Steps to reproduce:

Answer to an plain text article or email.
Example in newsgroup mozilla.test: <54BB29AA.8060705@hfigge.myfqdn.de>


Actual results:

The quoting is mangled.
This is caused by the patches of Bug 1113238. If I remove them all is fine again.
Blocks: 1113238
SM-Trunk Linux x86_64 is affected also.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
Keywords: regression
Is this fixed by bug 1119503?
(In reply to Takanori MATSUURA from comment #3)
> Is this fixed by bug 1119503?

It should be retested after that bug is fixed, but according to comment 0, that seems unlikely. :(
Depends on: 1119503
(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #4)
> (In reply to Takanori MATSUURA from comment #3)
> > Is this fixed by bug 1119503?
> 
> It should be retested after that bug is fixed, but according to comment 0,
> that seems unlikely. :(

Err, make that comment 1.

Alfred, I assume reverting the 2nd patch on that bug doesn't change anything, and it's only the first one, right?  Any chance you can get me the source HTML for the message that you are replying to?

Also, are you comfortable with using a debugger such as gdb?  If yes, it would be *hugely* helpful if you could set a breakpoint on both nsPlainTextSerializer::IsElementPreformatted and nsXHTMLContentSerializer::IsElementPreformatted and see which one is called and get me some full backtraces.  If you need help with doing that, I'd be happy to.

Thanks!
Flags: needinfo?(infofrommozilla)
(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #5)

> Alfred, I assume reverting the 2nd patch on that bug doesn't change
> anything, and it's only the first one, right?

I think Hartmut has already tested that.

@Hartmut: It was the It was the second patch - right?

>  Any chance you can get me the
> source HTML for the message that you are replying to?

It was also a plain-text message. I can upload the source of it.
But, what do you mean with HTML?

> would be *hugely* helpful if you could set a breakpoint on both
> nsPlainTextSerializer::IsElementPreformatted and
> nsXHTMLContentSerializer::IsElementPreformatted and see which one is called
> and get me some full backtraces.

Ok, I'll do that. You'll get the results tomorrow.
Flags: needinfo?(infofrommozilla)
This is a shorter message.
Attachment #8550893 - Attachment is obsolete: true
(In reply to Alfred Peters from comment #6)

> @Hartmut: It was the It was the second patch - right?

Backout of 'Make our...' was not longer possible, so I tried a build with backout of 'Part 2'. The build succeeded, but the problem with the mangling was still there. So the culprit has to be 'Make our...'.

Further testing is not possible at the moment due to a bustage on SM-Trunk Linux x86_64.
Attached file stacktrace.txt
(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #5)
> and get me some full backtraces.

Here you got it.
The buildrevision is:

comm-central changeset 17370:46a88d5a7720
mozilla-central changeset 224444:6446c26b45f9
Thanks a lot, Alfred.  I have a possible idea for a fix, let me write a patch that you can try out...
Comment on attachment 8554012 [details] [diff] [review]
Fall back to looking at the tag for determining if an element is preformatted when there is no style information available

Alfred, can you please test this patch?  It it doesn't apply cleanly, please just apply the nsPlainTextSerializer.cpp hunk manually.  Thanks!
Attachment #8554012 - Flags: feedback?(infofrommozilla)
There is a problem applying this patch for SM-Trunk Linux x86_64.

hafi@i5_64 ~/hg-moz/src/mozilla $ patch --dry-run -p1 < ~/ztmp/p2/attachment.cgi\?id\=8554012 
patching file dom/base/nsPlainTextSerializer.cpp
Hunk #1 FAILED at 1792.
1 out of 1 hunk FAILED -- saving rejects to file dom/base/nsPlainTextSerializer.cpp.rej
patching file dom/base/test/TestPlainTextSerializer.cpp

It may be that there is currently a bustage also for TB, so Alfred may not be able to build.
(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #13)
> Comment on attachment 8554012 [details] [diff] [review]

> Alfred, can you please test this patch?

Yes, that looks good. It does the trick.

>  It it doesn't apply cleanly, please
> just apply the nsPlainTextSerializer.cpp hunk manually.  Thanks!

actually I build without testing.
Attachment #8554012 - Flags: feedback?(infofrommozilla)
(In reply to Alfred Peters from comment #15)
> Created attachment 8554103 [details]
> answer_with_ att8553469.txt

Wrong number in the name. "... att8554012.txt" would have been correct.
Comment on attachment 8554012 [details] [diff] [review]
Fall back to looking at the tag for determining if an element is preformatted when there is no style information available

r=me
Attachment #8554012 - Flags: review?(bzbarsky) → review+
(In reply to Alfred Peters from comment #15)
> (In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail,
> needinfo? me!) from comment #13)
> > Comment on attachment 8554012 [details] [diff] [review]

> Yes, that looks good. It does the trick.

Unfortunately, not completely.

If I reply only with selected text, then the quotations are still broken.
(In reply to Takanori MATSUURA from comment #3)
> Is this fixed by bug 1119503?

Just for the records: No, those patches have no effect on this bug.
Attached file stacktrace2.txt
> and get me some full backtraces.

This time: Reply with selected text

comm-central changeset 17370:46a88d5a7720
mozilla-central changeset 224444:6446c26b45f9
with attachment 8554012 [details] [diff] [review] implemented!
(In reply to Alfred Peters from comment #18)

> If I reply only with selected text, then the quotations are still broken.

So they are, when 'copy and paste' out of the edit window.

And still another variant:
'copy and paste' out of the message-pane doesn't remove the line-breaks, but instead removes the quoting signs.
Alfred, thanks a lot for testing this.  The issues you mentioned in comment 18 onwards are probably separate issues, do you mind please filing separate bugs for them with as much info as you have?  The stack trace in comment 20 seems very normal, and it's not examining any edge case as far as I can tell, so please include some information on what it is that you're selecting exactly, etc.  Thanks!
Assignee: nobody → ehsan
Component: Composition → Serializers
No longer depends on: 1119503
Product: MailNews Core → Core
(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #22)

> tell, so please include some information on what it is that you're selecting
> exactly,

Doesn't really matter. If I select almost everything from the original message
and reply or copy almost everything from the edit window, the result looks
exactly like attachment 8553459 [details].

>  The issues you mentioned in comment
> 18 onwards are probably separate issues,

I would bet, they are all coursed by Bug 1113238

>  do you mind please filing separate
> bugs for them with as much info as you have?

If you think so, I'll do that.
https://hg.mozilla.org/mozilla-central/rev/7141add28851
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(In reply to Alfred Peters from comment #24)
> >  The issues you mentioned in comment
> > 18 onwards are probably separate issues,
> 
> I would bet, they are all coursed by Bug 1113238

Yes, it could be, but the fixes are different than this one (given that this patch didn't fix those issues) so it's better for release tracking purposes to have each symptom on file separately so that we can track which ones are fixed for a given release.  If some of those separate issues end up being the same underlying problem, we can dupe the bugs later.

> >  do you mind please filing separate
> > bugs for them with as much info as you have?
> 
> If you think so, I'll do that.

Thank you!  Please make sure that I'm CCed on the bugs.  I will try to fix as many as I can before I go on vacation on Wednesday.
[Tracking Requested - why for this release]: This breaks Thunderbird as noted in comment 0.
(In reply to Alfred Peters from comment #24)
> (In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail,
> >  The issues you mentioned in comment
> > 18 onwards are probably separate issues,
> I would bet, they are all coursed by Bug 1113238

I would have lost that bet. In fact these issues are a bit older.
(In reply to Alfred Peters from comment #28)
> (In reply to Alfred Peters from comment #24)
> > (In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail,
> > >  The issues you mentioned in comment
> > > 18 onwards are probably separate issues,
> > I would bet, they are all coursed by Bug 1113238
> 
> I would have lost that bet. In fact these issues are a bit older.

Right. Just tested with a SM-Trunk of last December. There may already be an entry in bugzilla for that. Anyway, it should not be too difficult to find the regression range.

Best be done in a new bug or the potential old one, though. ;)
I've alrad(In reply to Hartmut Figge from comment #29)
> (In reply to Alfred Peters from comment #28)
> > I would have lost that bet. In fact these issues are a bit older.
> Right. Just tested with a SM-Trunk of last December. There may already be an
> Best be done in a new bug or the potential old one, though. ;)

JFTR: Bug 1125956
Comment on attachment 8554012 [details] [diff] [review]
Fall back to looking at the tag for determining if an element is preformatted when there is no style information available

Approval Request Comment
[Feature/regressing bug #]: bug 1119503

[User impact if declined]: missing linebreaks with copy/paste

[Describe test coverage new/current, TreeHerder]: new test added here for testing the fallback-to-tag behaviour

[Risks and why]: 
Ehsan also told me to use this as the risk analysis:

"The risk on this patch set is moderate.  We have tried to not change the existing behavior except for only the well known cases, but this code is really old and not very well understood.  I (ehsan) would like these fixes to be backported on Aurora as soon as we can to get broader testing, and will be on the hook for fixing any regressions found."

[String/UUID change made/needed]: None
Attachment #8554012 - Flags: approval-mozilla-aurora?
Comment on attachment 8554012 [details] [diff] [review]
Fall back to looking at the tag for determining if an element is preformatted when there is no style information available

Small fix required for bug 1119503. Aurora+
Attachment #8554012 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.