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

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Alfred Peters [:infofrommozilla]]

Attachment: broken_quote.eml

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: <[e-mail]>


Actual results:

The quoting is mangled.

Comment 1

[Alfred Peters [:infofrommozilla]]

This is caused by the patches of Bug 1113238. If I remove them all is fine again.

Comment 2

[Hartmut Figge]

SM-Trunk Linux x86_64 is affected also.

Comment 3

[Takanori MATSUURA]

Is this fixed by bug 1119503?

Comment 4

[(no longer active)]

(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. :(

Comment 5

[(no longer active)]

(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!

Comment 6

[Alfred Peters [: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.

Comment 7

[Alfred Peters [:infofrommozilla]]

Attachment: A normal correct plain text message.

Comment 8

[Alfred Peters [:infofrommozilla]]

Attachment: The mangled answer to the prior message.

This is a shorter message.

Comment 9

[Hartmut Figge]

(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.

Comment 10

[Alfred Peters [:infofrommozilla]]

Attachment: 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

Comment 11

[(no longer active)]

Thanks a lot, Alfred.  I have a possible idea for a fix, let me write a patch that you can try out...

Comment 12

[(no longer active)]

Attachment: Fall back to looking at the tag for determining if an element is preformatted when there is no style information available

Comment 13

[(no longer active)]

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!

Comment 14

[Hartmut Figge]

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.

Comment 15

[Alfred Peters [:infofrommozilla]]

Attachment: answer_with_ att8553469.txt

(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.

Comment 16

[Alfred Peters [: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 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Alfred Peters [:infofrommozilla]]

(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.

Comment 19

[Alfred Peters [:infofrommozilla]]

(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.

Comment 20

[Alfred Peters [:infofrommozilla]]

Attachment: 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!

Comment 21

[Alfred Peters [:infofrommozilla]]

(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.

Comment 22

[(no longer active)]

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!

Comment 23

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7141add28851

Comment 24

[Alfred Peters [:infofrommozilla]]

(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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7141add28851

Comment 26

[(no longer active)]

(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.

Comment 27

[(no longer active)]

[Tracking Requested - why for this release]: This breaks Thunderbird as noted in comment 0.

Comment 28

[Alfred Peters [:infofrommozilla]]

(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.

Comment 29

[Hartmut Figge]

(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. ;)

Comment 30

[Alfred Peters [:infofrommozilla]]

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 31

[Andrew Overholt [:overholt]]

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

Comment 32

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/074e4205c399