117410 - image not loaded if SRC URL has a leading SPACE character.

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[John Morrison]

Almost hate to file a bug like this, but this works in Nav4.x, IE and
worked in the 6.2 release. It stopped working between 11/15 and 11/20
(of builds that I had lying around). Given that the url rewrite landed
on 11/15, I'm guessing that this is related (although possibly not).

Anyways ... if you have a <IMG SRC="..."> and the URL in the SRC
attribute has a leading SPACE character, the image will not
load. Curiously, if it has a leading TAB character, the image will
load (i.e., a leading TAB is stripped, but a leading SPACE is not
stripped from the URL).

I'll attach a simple test case in one moment.

Comment 1

[John Morrison]

Attachment: simple test case; SPACE before URL in SRC of IMG == image doesn't load.

Comment 2

[John Morrison]

Attachment: simple test case; SPACE before URL in SRC of IMG == image doesn't load.

take two. (I accidentally expanded the tab in the first attachment when
cutting/pasting from a remote term window).

Comment 3

[Andreas Otte]

Hmmmm ... if I remember correctly stripping spaces was removed from resolving
relative urls some time ago, not just with the urlparser rewrite. AFAIK that was
moved to the customers of ::Resolve because some of them wanted the space
preserved, some not.

Comment 4

[Andreas Otte]

Take a look at the discussion in bug 50285 for more details.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

src="" is a URI which is defined as a CDATA element.
http://www.w3.org/TR/html401/sgml/dtd.html#URI

This is a likely dupe of Bug 87894  - "Leading and trailing whitespace should be
trimmed from CDATA attributes in quirks mode"

The fact that it worked on the branch was because of bug 79929 I'm sure...

Comment 6

[Darin Fisher]

-> badami

Comment 7

[Andreas Otte]

This is not a networking issue in my opinion. We should not strip
leading/trailing spaces on relative urls. This has to be done by the customer of
::Resolve in context of the situation and before the string is given to ::Resolve.

Comment 8

[Vinay Badami]

Attachment: trim uri for leading spaces

Comment 9

[Andreas Otte]

Okay, I was on the wrong trip here, thinking about relative urls. If it is an
absolute URL the spaces should be left/right trimmed from the url. But why not
move that trim into FilterString?
 

Comment 10

[Vinay Badami]

Attachment: remove FilterString and replace it by StripChars

Looks like StripChars does what the FilterString routine was doing.
Note that i need to pass the cleaned up spec to ParseURL and
BuildNormalizedSpec since they will be noting offsets into this string for the
various URI components.

Comment 11

[Darin Fisher]

Comment on attachment 71644 [details] [diff] [review]
remove FilterString and replace it by StripChars

vinay: FilterString does more than StripChars... it avoids a costly string copy
when there is nothing to filter.  let's not remove it.	instead, can you fix
FilterString to do the right thing?

Comment 12

[Vinay Badami]

Attachment: modify FilterString

1. Return with PR_FALSE on null string.
2. Strip leading spaces. 
3. Strip trailing spaces.

Comment 13

[Darin Fisher]

Comment on attachment 71853 [details] [diff] [review]
modify FilterString

Append null terminates, so there is no need to worry about null terminating
|p|... also no need to worry about restoring str to original state to preserve
constness.

Comment 14

[Vinay Badami]

Attachment: with darins comments

Comment 15

[Darin Fisher]

Comment on attachment 72011 [details] [diff] [review]
with darins comments

a few more comments (i should have caught these before)...

> nsStandardURL::FilterString(const char *str, nsCString &result)
> {
>+    if (!str) 
>+        return PR_FALSE;

this check should be moved to nsStandardURL::SetSpec after the call to Clear()


>+    PRBool firstNonWhiteSpace = PR_FALSE;

instead of checking this variable everytime through the loop we
should have a separate loop for removing leading whitespace.

like so:

for (; *p; ++p) {
  if (*p == ' ') {
    writing = PR_TRUE;
    str = p + 1;
  }
}

then follow with the existing loop to remove control characters:

for (; *p; ++p) {
  if (*p == '\t' || ... ) {
    // ...
  }
}

and finally, a third loop to strip trailing white space:

>+    // Remove trailing spaces if any
>+    const char* end = (char*)p;
>+    while (((p-1) >= str) && (*(p-1) == ' ')) {
>+        writing = PR_TRUE;
>+        p--;
>     }

doesn't look like |end| is needed.

i'm being especially picky here because SetSpec is called thousands of
times during startup and thousands of times during page load.

Comment 16

[Vinay Badami]

Attachment: with 3 loops

Comment 17

[Darin Fisher]

Comment on attachment 72349 [details] [diff] [review]
with 3 loops


>Index: nsStandardURL.cpp

>+    // Remove leading spaces
>+    while (*p && (*p == ' ')) {

while (*p == ' ')

is equivalent


>+    NS_PRECONDITION(spec, "null pointer");
>+    if(!spec)
>+        return NS_ERROR_INVALID_ARG;

sorry, i think i gave you misleading advice.  either it is valid
to pass in a NULL spec, meaning that the URL gets completely
cleared, or it is invalid to pass in a NULL spec, and nothing
should change.	this patch would not satisfy either approach.

i'd prefer to say that a NULL spec simply clears the URL.

so this NULL check (after the call to Clear()), should silently
return NS_OK.

Comment 18

[Vinay Badami]

Attachment: oen more

1. Changed while (*p && (*p == ' ')) to while (*p == ' ')
2. Return NS_OK if spec was null after clearing URL.

Comment 19

[Darin Fisher]

Comment on attachment 72553 [details] [diff] [review]
oen more

i meant that the NS_PRECONDITION should be removed since it seems to suggest
that the caller is in error... with that, sr=darin :-)

Comment 20

[Bradley Baetz (:bbaetz)]

Comment on attachment 72553 [details] [diff] [review]
oen more

r=bbaetz with the precondition removed.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't you filter out the control characters before stripping
leading/trailing whitespace? Otherwise stuff like "   \n   http://ocallahan.org"
won't work.

Comment 22

[Vinay Badami]

Attachment: removing leading tabs and CRLF too

Darin, Bbaetz
Can u folks r/srf this once more please ?

Comment 23

[Darin Fisher]

Comment on attachment 72952 [details] [diff] [review]
removing leading tabs and CRLF too

badami: you're going to hate this, but the old for loop in the code could be
collapsed into a while() loop which would avoid testing (*p != 0) :-)

with that sr=darin

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 72952 [details] [diff] [review]
removing leading tabs and CRLF too

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 25

[Vinay Badami]

fixed with checkin

D:\mozilla\netwerk\base\src>cvs commit
cvs commit: Examining .
Checking in nsStandardURL.cpp;
/cvsroot/mozilla/netwerk/base/src/nsStandardURL.cpp,v  <--  nsStandardURL.cpp
new revision: 1.17; previous revision: 1.16
done

Comment 26

[Vadim Berezniker]

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