22983 - extra newlines added to hidden input during form submission

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Dawn Endico]

I tried to edit the bugzilla operating parameters using mozilla.
I changed one field, "nummilestones", and when i hit submit, 10
others changed as well. The ones that bugzilla claimed it changed
were all textareas although some textareas were left alone.

Comment 1

[Dawn Endico]

Saving new parameters
This is Bugzilla: the Mozilla bug system. For more
information about what Bugzilla is and what it can do, see mozilla.org's bug pages.

Changed bannerhtml.
Changed blurbhtml.
Changed warnbannerhtml.
Changed
warnfooterhtml.
Changed changedmail.
Changed whinemail.
Changed curmilestone.
Changed entryheaderhtml.
Changed emailregexpdesc.
Changed voteremovedmail.
Changed browserbugmessage.
OK, done.


These are the fields that bugzilla claimed to have changed.
These fields seem to have the right values tho. Terry do
you agree?

Comment 2

[Terry Weissman]

It turns out that Dawn was using a recent Mozilla build to do these changes.  It
ended up adding newlines to textareas that had long entries in them.  Seems like
a bug in forms.

Comment 3

[Dawn Endico]

I was using the Jan 3, 2000 daily build.

Comment 4

[troy]

Re-assigning to pollmann for form problem

Comment 5

[Dawn Endico]

Attachment: http://bugzilla.mozilla.org/editparams.cgi

Comment 6

[Dawn Endico]

Attaching the editparams.cgi page as a test case since access of this url
is limited.

Comment 7

[Dawn Endico]

changed summary to something more useful



Marked as dogfood since this makes

mozilla useless for editing bugzilla

parameters. The added newlines horked

things overnight so that all

bugzilla mail was sent out with no bug summary in

the subject.

Comment 8

[Dawn Endico]

Note how each line in my last comment has an extra newline.
This is no doubt the same as the initial problem which would
make this bug a dup.

Comment 9

[Eric Pollmann]

Updating summary.

Comment 10

[Ben Bucksch (:BenB)]

Readding dogfood. Any progress on this bug?

Does not happen on Windows.

Comment 11

[Dawn Endico]

is anyone looking at  the dogfood keyword any more? I think "beta 2" would be
more appropriate if that exists yet. 

Comment 12

[Eric Pollmann]

I think this may have been fixed with Simon's latest linefeed converter fixes,
but I'll try it out to verify when my build is done.

Comment 13

[Eric Pollmann]

Assuming this comment comes out looking okay, as it does in my tests, this bug
looks to be fixed.  However, I noticed that the textarea is *appending* a CR/LF
to the end of a string, and each time session history is used to return to a
page, another CR/LF is added to the end.  I'll try to trace this down as the two
problems are closely related.

Posting this comment with 3/15 Linux build

Comment 14

[leger]

Putting on PDT+ radar for beta1 for the extra intermediate new line fix.  
Marking Resolved/Fixed - spoke with pollman, he ok'd.  

Please open a new bug for the trailing new lines problem.

Comment 15

[Eric Pollmann]

The new bug for extra trailing CR/LF's is bug 31699, thanks!

Comment 16

[Ben Bucksch (:BenB)]

> ------- Additional Comments From [e-mail] 2000-03-13 15:27 ----

> Posting this comment with 3/15 Linux build



? :-)



Seriously, which fixes are you speaking about? The latest changes from sfraser I

can see in bonsai are from 2000-03-09, and my build from Sun Mar 12 04:42:14 CET

2000 shows the bug (posting with this build).

Comment 17

[leger]

Updating QA Contact.

Comment 18

[Eric Pollmann]

[e-mail], which platform are you posting from, Win Mac or Linux?

Comment 19

[Ben Bucksch (:BenB)]

Linux, RH6.0 updated with 6.1 packages. The Mozilla instance I'm using is an
optimized non-debug build of my own.

- Ben Bucksch (posting with 4.7 now)

Comment 20

[Eric Pollmann]

(test post, please ignore)

This is a really long line which I did not press Enter while typing in.  Since
the textarea has WRAP=HARD, it should insert one CR/LF in the middle between the
words "Since" and "the" and another between "the" and "words".

This is a really long line which I did press Enter while typing
in.  It should also have one CR/LF in the middle between "typing" and
"in" and another between "and" and ""in"".

Entered using 15-Mar Linux build pulled and built this morning.

Comment 21

[Eric Pollmann]

I'm not having any problems with extra CR/LF's in the middle of the string, only
one at the end.  CC'ing Simon as maybe he can provide more info on linefeed
conversion.  However, I can't reproduce the bug, so I will have to leave it
RESOLVED FIXED for now.

Comment 22

[Eric Pollmann]

Um... Yes, today is 13-Mar not 15-Mar, the digits on the phone are too small to
read or else I need my eyes checked.  :)  The build I am using is form 13-Mar
morning

Comment 23

[Eric Pollmann]

See also bug 20207 - akkana fixed a bug today which sounds surprisingly like
this one.

Comment 24

[Ben Bucksch (:BenB)]

The extra newlines only appear, if I have to login after submission.

Reproduce:
1. Logout (or change you IP). Make sure, not to login before step 5.
2. Open a bug
3. Add a comment
4. Submit
5. You'll see a login page. Use that page to login. You're comment will be
submitted after you did that.
6. Go back to the bug.

REOPENing

Comment 25

[Ben Bucksch (:BenB)]

s/You're/Your
/me goes away ashamed

Comment 26

[Eric Pollmann]

Adding comments with a build that had had Akkana's fix for bug 20207 removed
from it.  I logged out first, so extra newlines should appear.

Comment 27

[Eric Pollmann]

Adding more comments to another build with the fix removed and myself logged

out.  This is on Solaris.

Comment 28

[Eric Pollmann]

Ah ha!  Well the problem is this:

<INPUT TYPE=HIDDEN NAME="foo" VALUE="Short&#013;&#010;Line">

When this value is submitted, it will be submitted as:

foo=Short%0D%0D%0ALine

This causes an extra CR to be inserted.  It was *really* helpful to know that
you had to be logged out of bugzilla - because only in this case will your
comments be cached in a hidden input before being submitted.

I don't think that this is a very common scenario, and am dubious that it
deserves to be PDT+.  I'll look at what it takes to fix this one.

Comment 29

[Akkana Peck]

We should not be able to insert CR into the dom, even if the user/content author
mistakenly says we should.  It's an illegal character and the editor doesn't
deal well with it.

nsGfxTextControlFrame should be stripping out the CRs; it would be useful to
know how the CR is being inserted, because perhaps one of the form control
classes needs to be modified in a similar way.

Comment 30

[Eric Pollmann]

The editor is not getting a CR in the case of this bug.

The hidden input is implemented by nsGfxButtonControlFrame, which doesn't use
the editor.

I'm not sure that:

1) The bug is a serious one

This will only cause a problem if the website intentionally puts an illegal
value (
) in a form element.  I've not seen any similar bugs in other
websites except bugzilla. (Correct me if I'm wrong!)

2) The fix would be small

If we want to make this fix consistent for all form elements, we'd have to put
CR stripping code similar to that used in bug 20207 in each of the form frame
classes.

Since the schedule has been pushed up to the 21st, and this bug does not have a
known fix (may cause regressions that won't be caught in a few days) I'd suggest
moving it to beta2.  Removing PDT+ and re-adding beta1 for reconsideration.

Comment 31

[Akkana Peck]

I agree with all of Eric's points.  We should fix bugzilla not to put the
illegal character in, and we should hold off on the form element fixes
'til after beta1.

Comment 32

[Ben Bucksch (:BenB)]

Filed bug #32000 about the bugzilla change.

Comment 33

[leger]

Putting on the PDT- radar for beta1.

Comment 34

[Jesse Ruderman]

How is this bug related to bug 22707?

Comment 35

[Eric Pollmann]

In both of this bug and bug 20207, a CR character is being added to the content
model.  For bug 20207, but element involved is a textarea.  For this bug it is a
hidden input.

The problem is that a CR is an illegal character as far as the content model is
concerned.  The solution that Akkana implemented for bug 20207 was to remove the
CR's from the string before setting it in the content model.  We should probably
implement a similar fix for this bug.

Comment 36

[Eric Pollmann]

Or if you're note dyslexic like me...  Bug 22707 is also related to this bug,
but is not the same.  Bug 22707 contains something like this:

<INPUT TYPE=HIDDEN NAME="foo" VALUE="Short <-----------------(CR LF here)
Line">

Whereas this bug contains this:

<INPUT TYPE=HIDDEN NAME="foo" VALUE="Short&#013;&#010;Line">

Note that bug 22707 is using the actual CR and LF characters whereas in this bug
we are using the HTML entities representing the CR and LF characters.  The bugs
are different because in the case of bug 22707, both of the characters are
stripped out before being sent to the content model, but in this bug neither
are.  Both should be addressed separately.

Comment 37

[Eric Pollmann]

Moving out to M17.

Comment 38

[Hixie (not reading bugmail)]

See bug 4928 for the background into why Bugzilla escapes newlines in the first
place. It's a long story...

Comment 39

[Eric Pollmann]

Moving out to M18 - schedule update.

Comment 40

[Eric Pollmann]

*** Bug 38826 has been marked as a duplicate of this bug. ***

Comment 41

[Eric Pollmann]

See comments on bug 31699, I'm merely transfering a wrongly reported bug's 
dogfood+ status to the right bug for the problem.

Comment 42

[Eric Pollmann]

Attachment: patch 1 - only convert linebreaks for textareas

Comment 43

[Eric Pollmann]

Perhaps this fix should be in addition to not allowing CR's into the content 
model.

The current behaviour induces platform specific behaviour into form controls 
that do not vary from platform to platform (hidden, checkbox, radio, buttons, 
submits, resets, ...)  The only control I can think of that would vary from 
platform to platform is the textarea, which may contain platform specific 
linebreaks.  Thus, only this control should have platform specific linebreaks 
converted into net linebreaks.

Comment 44

[Eric Pollmann]

The only place the value of a hidden input can be set is on the content side 
(nsHTMLInputElement::SetValue)  We'll have to filter out \r's there!

Comment 45

[Eric Pollmann]

Attachment: reduced test case

Comment 46

[Eric Pollmann]

Logged out and adding this comment to test my fix on Linux.
The reduced test case can also be used to verify the fix.
Load the reduced test case and click Submit
The Query string should be http://pollmann.net/echo.cgi?foo=bar%0D%0Abaz

Comment 47

[Braden]

Removed beta1 keyword. Perhaps this should be nominated for nsbeta2?

Comment 48

[Jim Roskind]

Updated status whiteboard with possible landing date, based on comments above.

Comment 49

[ckritzer (gone)]

Eric, pollman.net is down... could you boot that bad boy back up?  Thanks dude!
-Chris

Comment 50

[Eric Pollmann]

Simon, being quite Da Man, wrote ConvertLineBreaks to also take this param for 
source line breaks:

eLinebreakAny

I think this might be a better and more general solution.  I'll come up with a 
test case that includes all three types of line breaks and test on all three 
platforms - if this works, the patch becomes a one-liner and works for many more 
edge cases!

Comment 51

[Simon Fraser [no longer active]]

Yeah. My linebreak converters should also be used in 
nsFormControlHelper::PlatformToDOMLineBreaks().

Comment 52

[Jim Roskind]

Clarifying status whiteboard (dogfood+)

Comment 53

[Eric Pollmann]

Fix checked in.  I used the simple platform encoder change discussed above.

Comment 54

[ckritzer (gone)]

Eric, on the reduced test case (05/30/00 19:08), should the output of the test 
for Content Length look like this (where '•' = disc/bullet & '°' = circle/open 
bullet):

• foo
      ° bar
        baz

If this is the correct output, then it's fixed.  If not, then it's broken cross 
platform.

Comment 55

[Eric Pollmann]

Yes, that's right.  Sorry, I forgot to add verify instructions.  To verify 
(more thorough test case):

1) go to: http://blueviper/forms/hiddencrlf2.html
2) Click Submit Query
3) The URL that is submitted to should be:

http://pollmann.net/echo.cgi?foo=bar%0D%0Abaz&bar=baz%0D%0Afoo&baz=foo%0D0Abar

This means that no matter what type of line feeds are added to an input, they 
are always converted to net line feeds (%0D%0A) when they are submitted.

All three values (foo, bar, and baz) should have a line break in the middle of 
their value on the resulting page, like you observed before - single line break 
with no double-spacing!

• foo
      ° bar
        baz
• bar
      ° baz
        foo
• baz
      ° foo
        bar

Thanks!

Comment 56

[ckritzer (gone)]

Good testcase Eric...thanks!

Marking VERIFED FIXED on:
- MacOS9 2000-06-05-08-M16 Commercial Build
- Linux6 2000-06-05-08-M16 Commercial Build
- Win98  2000-06-05-08-M16 Commercial Build

Comment 57

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/70b44757a76e
[fenix] For https://github.com/mozilla-mobile/fenix/issues/22983 - Remove @color/destructive_button_text_color redirection