144728 - Midair collision doubles line feeds

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Jouni Heikniemi]

When submitting comment changes after midair collision confirmation, linefeeds
are doubled.

Steps:

1) Pick a bug and make some modifications, submit them
2) Hit back
3) Add a comment with linefeeds
4) Submit. Midair collision screen should pop up. 
5) Submit the changes anyway. 

Now go back to the bug and see what happened: the linefeeds are doubled.

I managed to repeat this on an almost fresh CVS pull running under Win2k, but
not on landfill's bugzilla-tip. I will investigate further soon, but marking
WinNT for now.

Comment 1

[Jouni Heikniemi]

Attachment: Patch proposal v1

Add some lf filters before processing the midair template. Comments follow.

Comment 2

[Jouni Heikniemi]

Ok, I think I understand this. When a comment is sent to database, it's passed
through "AppendComment" in bugs.pm. That sub has a couple of regexp replacements
which kill Windows and Mac line feeds. Those filters are not applied when a
comment is written out to the midair template, so the html source ends up
containing double linefeeds at least on Windows/Windows (client/server)
configurations. 

My patch adds the mentioned replacement regexps to the midair context as well.
The fix sucks, but it worked fine on my installation. Ccing Myk and Gerv for
reviews and comments. 

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Perhaps some code from Doctor can be of use here:

http://lxr.mozilla.org/mozilla/source/webtools/doctor/doctor.cgi#60
http://lxr.mozilla.org/mozilla/source/webtools/doctor/doctor.cgi#272

Comment 4

[Jouni Heikniemi]

Looks good, that could be done. Do you think it would be wise to make another
patch using the code from the Doctor? Or would it be better not to take risks
and introduce the new lf algoritms after 2.16 is out?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

I think we should wait until after 2.16 is released before doing anything with
this bug.  The problem is minor and the fixes, while relatively low-risk, aren't
trivial and could have unintended consequences that we shouldn't risk
introducing at this stage in the release cycle.

Comment 6

[Dave Miller [:justdave]]

Comment on attachment 83879 [details] [diff] [review]
Patch proposal v1

if I understand that code from doctor correctly, that can only be used when
you're passing it as a hidden form field, correct?  Right now, Bugzilla makes
no distinction between coming from the midair collision page and coming from
the normal submit page.  All it does is update the timestamp on the page so it
doesn't fail the midair test again.  This means the processing code would have
no idea if it was processing data generated by a textarea or by a hidden field,
and we can't very well be putting \n and \r in a textarea. :-)

So in this case, adding a check for windows linefeeds is probably the best
route to take, and it's pretty trivial.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Sounds like an ok plan for now, but I think we want to implement Doctor's
approach in the long run (perhaps with a hidden field that flags the form as
coming from the mid-air page).  The HTML spec calls for line breaks in tag
attributes to be silently removed by the client, so counting on the client to
retain those breaks is unreliable.

In fact, I'm not sure why it's even working here.  It didn't work for me when I
tried it in Doctor (hence the encoding).  cc:ing hixie, who may be able to shed
some light on the subject.

Comment 8

[Bradley Baetz (:bbaetz)]

We've had this discussion before, haven't we? See bug 97980 comment 10 and
surrounds.

Is this a dupe, or just really really related?

Comment 9

[Jouni Heikniemi]

I'd go for the "really really related" - rewriting all the linefeed stuff would
help, but this could be fixed separately without hassling with standards
compliance and all those nasty topics covered in the other wrapping bugs. 

Comment 10

[Bradley Baetz (:bbaetz)]

So, is this a regression? If so, are we going to want this for 2.16 anyway?

Comment 11

[Dave Miller [:justdave]]

Comment on attachment 83879 [details] [diff] [review]
Patch proposal v1

2xr= justdave
let's get this in.

This is obviously the right fix (for now) because it's what we already do in
other places (as Jouni mentioned).

Bug 97980/bug 11901 are the places to finish this up the correct way.

Comment 12

[Dave Miller [:justdave]]

Jouni, do you have checkin privs?

Comment 13

[Jouni Heikniemi]

Nope, somebody please check this in.

Comment 14

[Gervase Markham [:gerv]]

If we are fixing this correctly later, does it go on the trunk?

Gerv

Comment 15

[Jouni Heikniemi]

I think it should be in the trunk as well, because the fix doesn't make doing
the right thing more difficult, and it may take a while before the proper
solution is ready - it wouldn't be cool to have this regress in the first
developer release of 2.17 series where we still might not have the linewrap
policy remade.

Comment 16

[Gervase Markham [:gerv]]

Checked in on trunk and branch.

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.125.2.1; previous revision: 1.125
done

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.126; previous revision: 1.125
done

Gerv