57968 - Pages break if HREF / SRC attribute spans two lines

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P1)

Description

[Mike Pinkerton (not reading bugmail)]

http://www.calcbuilder.com/cgi-bin/calcs/HOM17.cgi/themotleyfool

There are lots of image form submit buttons on this page, and none of them show
up correctly.

All platforms, builds from 10/24/00.

Comment 1

[Mike Pinkerton (not reading bugmail)]

this may be a stop-ship.

Comment 2

[Boris Zbarsky [:bzbarsky]]

If you look at the page source:

<INPUT TYPE="IMAGE" NAME="Inputs" SRC="/fc_calcs/images/calc_buttons/
input_selected.gif" BORDER="0">

Yes, the line break in the middle of the SRC attribute is there. It is causing
the problem.  If that is removed, the page works beautifully.

This happens for all images, not just IMAGE inputs.  Is this a Mozilla bug, or
something to evangelize?

Comment 3

[Mike Pinkerton (not reading bugmail)]

resummarizing. probably a parser problem. cc'ing harish

Comment 4

[harishd]

According to bug 47535 parser should not strip of '\n'.  I guess this should get
handled in the layout land. CCing pollmann

Comment 5

[Richard Brodie]

I don't think it is up to the parser to strip whitespace from attributes;
however RFC2396 notes that URI may be line wrapped, although the remarks refer
more directly to text. One could argue they should be correct as is in HTML.

'In some cases, extra whitespace (spaces, linebreaks, tabs, etc.) may
need to be added to break long URI across lines. The whitespace
should be ignored when extracting the URI.'

This is essentially a dupe of bug 55925, BTW.

Comment 6

[Kevin McCluskey (gone)]

Marking [rtm need info]
reassigning to polmmann.

Comment 7

[Eric Pollmann]

Looking for advice on this one - should we always strip linebreaks from the path
portion of URLS before sending them to the server?  Does this change need to go
to general URL parsing, or should it be restricted to just IMG and INPUT
TYPE=IMAGE SRC=?  It seems like it would also apply to FRAME SRC=, A HREF=,
SCRIPT SRC=, and any of a myriad of other things.

Comment 8

[Eric Pollmann]

FRAME SRC=, LINK HREF= and A HREF= already strip out linebreaks
SCRIPT SRC=, IMG SRC=, and INPUT TYPE=IMAGE SRC= do not strip out linebreaks

Oh, for consistency...

Comment 9

[Eric Pollmann]

Harish, we used to strip *all* linebreaks out of *all* attributes in the parser,
right?  I'm asking to see what the chances of regression are by changing it to
strip linebreaks out of just these attributes.  It seems like if we always
stripped the linebreaks all along (until 15204 fix was checked in one month
ago), the change is pretty safe - one might even say Zaro Reesk!

Comment 10

[Hixie (not reading bugmail)]

Eric: per HTML4/SGML, spaces and newlines in attributes should be trimmed and
collapsed, so a single newline would become a space and then be escaped to %20
if it was an href or src attribute.

We don't do this for various reasons, mainly for backwards compatability and
such like. To be honest if people do stupid things to their attribute values...

Comment 11

[harishd]

I think it'll be much safer to deal with this problem in the layout than the
parser because in layout you will know exactly ( I think! )what element you're
dealing with and the constraints it imposes.

Comment 12

[Kevin McCluskey (gone)]

Marking rtm-. Assumming that line-breaks in src attribute does not occur very
often. If they are examples of top100 sites breaking because of this bug please
clear the rtm-.

Comment 13

[Eric Pollmann]

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

Comment 14

[Eric Pollmann]

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

Comment 15

[Eric Pollmann]

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

Comment 16

[Eric Pollmann]

I have a simplefix for this bug.

The Motley Fool test case is at:
www.fool.com->Personal Finance->Calculators->What home can I afford (under Home)

Based on the prominence of that test case, the number of dups, and the fact that
this is a regression within the last month, putting back on RTM radar.  Andreas,
can you review the patch?  Thanks!

Comment 17

[Eric Pollmann]

Attachment: patch to nsStdURL.cpp

Comment 18

[Eric Pollmann]

CC'ing Scott - would you be able to super review this bug or recommend someone
who could?

Comment 19

[Eric Pollmann]

Also CC'ing Jud - CVS Blame says you also might be a good reviewer here...  :)

Comment 20

[Judson Valeski]

defering to andreas; sorry (I don't like touching stdurl)

Comment 21

[Scott MacGregor]

Eric, your change looks okay to me. I"m worried about the risk though so we
should definetly get a lot of tip testing before landing on the branch. In
particular, the method you rewrote (SetSpec) is used for so many different types
of urls in the product including complex imap, mailbox, and mailto urls.

I'd just be worried about changing something all these urls use. For kicks, are
you able to read imap mail okay, try saving an attachment or two, etc.

sr=mscott

Comment 22

[Eric Pollmann]

I've been using this build for my daily work for a few days now.  IMAP works
great, I've tested sending and receiving attachments.  In fact, I've set a
breakpoint in my newly added code so I will see if we ever encounter linebreaks
in URLs, and the only place it has been fired so far is on the test cases
associated with this bug.

Gagan or Rick, would you be able to review this change so I can get it into the
trunk at least?  :) 

Comment 23

[Eric Pollmann]

About the only other thing I could think of where we might want a linebreak in a
URL was an ftp: URL with a filename that has a linebreak in it.  This is not
broken by my change either - any more than it already is anyway:

If this is an <A HREF="ftp://server/foo
file">get foo file</a> then we already strip the linebreak and it won't work.

If this is in the ftp: tree browser, it already gets the wrong name when listing
the directory and won't work.  I guess that's not a big deal because:

1) No other ftp client I tried could get the file either
2) It's pretty hard to create a filename with a linebreak in it, even on UNIX.
3) If this were to be fixed we could still pass an escaped linebreak through my
change.

Any other strange test cases people can think of?

As an additional note, we *already* strip linebreaks out of URLS for <A
HREF=..., <FRAME SRC=..., and <LINK SRC=... (probably some of the most common
cases).  With this change, we make our behaviour with all of the other possible
cases consistent with the spec as well, and can (later) remove the excess code
in these other places.

Comment 24

[Gagan]

for rtm this is a great fix. r=gagan

for trunk (and beyond) we should remove the now-redundant checks from elsewhere
that strip off the newlines.

Comment 25

[selmer (gone)]

rtm-, not stop ship.

Comment 26

[Andreas Otte]

Sorry for being so late, but I just returned home after several days. The fix
looks good to me. We should definitly try this on the trunk.

But there was a long debate (can't remember the bug) about where to put code to
remove spaces, newlines, control characters, etc. In the end it was put all over
the code to be more flexibel in dealing with it. I really don't know if it is
necessary to be flexibel in this way.

Comment 27

[Eric Pollmann]

Since this is not RTM, I fired off this email to [e-mail] and
[e-mail]:

  Hello all,

  I work at Netscape as a software developer.  This is just a quick note to let
  you know that your calculators (all of them I tried) will not work inside
  Netscape 6.  And Netscape 6 will be released very soon!

  The problem is very easy to fix.  On pages that contain images, you
  can not split the SRC= attribute across multiple lines for Netscape 6.

  Here's an example:

    http://www.calcbuilder.com/cgi-bin/calcs/HOM17.cgi/themotleyfool

  This page contains the following:

    <IMG SRC="/fc_calcs/images/calc_buttons/
    brand.gif" BORDER=0>

  This needs to be rewritten as:

    <IMG SRC="/fc_calcs/images/calc_buttons/brand.gif" BORDER=0>

  I thought that this might be a general problem with the calculators generated
  by www.calcbuilder.com, but I have tested the calculators on their main site,
  and they do not have this problem.  Please let me know if I can be of further
  assistance tracking down this problem.

  For reference, the Bugzilla bug that is associated with this problem is
  located here:

    http://bugzilla.mozilla.org/show_bug.cgi?id=57968

  Thanks,
  -Eric

Comment 28

[Eric Pollmann]

Checked the fix into the trunk.

To verify, go to this URL:
http://www.calcbuilder.com/cgi-bin/calcs/HOM17.cgi/themotleyfool

You should see the page as it appears in Navigator - that is, the Images for the 
tabs along the top of the calculator will appear, and clicking on them will take 
you to the right place.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

Comment 30

[Eric Pollmann]

Was contacted by a techie from fool.com today (Bijoy).  The tech was unable to
reproduce the problem.  I suspect it was because PR1,2,3, or a daily trunk build
was used.  I wrote back with the location of the latest trunk builds on
Mozilla's download site, and more explicit info on how to fix the problem.

Comment 31

[Eric Pollmann]

Reminded The Fool to fix their web pages, because it hasn't been done yet.

Comment 32

[Vladimir Ermakov]

Verifying windows 98 build 2001-03-29-04-Mtrunk