Closed Bug 79929 Opened 24 years ago Closed 23 years ago

IMG tags with a space in the src= path are not loaded

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.2

People

(Reporter: mgregan, Assigned: attinasi)

References

(
URL
)

Details

(Whiteboard: PDT+, critical for 0.9.2, have patch, r and sr; a=drivers for image only fix on 0.9.2 branch)

Attachments

(5 files)

Simple testcase
160 bytes, text/html
Details
Patch
1001 bytes, patch
Details | Diff | Splinter Review
New Patch that handles dynamic changes to src too
1.14 KB, patch
Details | Diff | Splinter Review
Patch to content sink: strips whitespace for all attributes that are not in a special list (back to old behavior without regressing the option value fix)
1.43 KB, patch
Details | Diff | Splinter Review
patch in netlib [one more fly in the ointment... :) ]
1.88 KB, patch
Details | Diff | Splinter Review
At the above URL, there are two IMG tags that have a space in the path to the image e.g. SRC=" /someimage.gif". This causes Mozilla not to load the images in question. Removing those spaces with the editor and looking in the preview shows the images load and display correctly.
I'm seeing this too in Build 2001050921 in Linux. Changing OS to All. Only happens when the url is not absolute. I don't know what the Html standards say on this.
OS: Windows 2000 → All
updating component
Assignee: asa → clayton
Status: UNCONFIRMED → NEW
Component: Browser-General → HTML Element
Ever confirmed: true
QA Contact: doronr → bsharma
*** Bug 84313 has been marked as a duplicate of this bug. ***
Whiteboard: want for mozilla 0.9.2
Whiteboard: want for mozilla 0.9.2 → critical for 0.9.2
wonder how much content really looks like this...
I believe we should remove trailing and leading whitespace (but not ws in between). Clayton is not the correct owner, HTML Element component should be removed from Bugzilla. Reassigning to layout.
Assignee: clayton → karnaze
Component: HTML Element → Layout
QA Contact: bsharma → petersen
marc, can you have a look?
Assignee: karnaze → attinasi
Whiteboard: critical for 0.9.2 → PDT+, critical for 0.9.2
I'll look deeper, but removing the leading (and possibly trailing) spaces shoul dbe pretty easy, although I believe that this should probably be avoided for standard mode (I need to check the HTML specs to see if the attribute is somehow special). Just curious: why is this critical for 0.9.2?
Status: NEW → ASSIGNED
See also bug 24346 - same problem, resolved FIXED.
Changing QA contact
QA Contact: petersen → tpreston
CC'ing Harish - ideas?
This space is no longer stripped out as of 21-April, rev 3.339 of that file. If I remember correctly, that was to fix a problem with: <select> <option value=" foo"> In that case we *do* need to retain the space. Perhaps the space should be stripped out in the image frame, or in the url parser?
I think it is dangerous to change this at the Content sink since it has no clue about attribute semantics, or even the URLParser level where it cannot understand the calling-context. If I strip out the leading whitespace (and trailing I guess) in the ImageFrame, then the DOM is not messed up. Of course, it is not entirely clear that we are better off having a DOM value for the attribute and using something else to actually load the image, but at least the DOM is consistent with the source. My proposed fix, which I am coding now and testing, is to do the trimming of the src and lowsrc attribute values in the nsImageFrame::LoadImage method, for QuirksMode only. I really think that we should NOT trim the whitespace in standard mode, but I have yet to find anything in the specs to back me up...
Attached file Simple testcase —
Attached patch Patch — — Splinter Review
should this go in nsImageFrame::LoadImage so that it catches the case when someone changes the source dynamically?
Should a similar patch go in for: <form action=> <body background=> <blockquote/q/del/ins cite=> <object classid=> <object/applit codebase=> <object data=> <a/area/link/base href=> <img/frame/iframe longdesc=> <head profile=> <script/input/frame/iframe/img src=> <img/input/object usemap=> (There are the attributes with type URI at: http://www.w3.org/TR/html4/index/attributes.html) Is it ever legal to have whitespace at the beginning of a URI? Could all this be done in one place?
According to RFC 2396, the space character is forbidden everywhere within any URI. So Moz should just do whatever it is Moz usually does with a bogus attribute value. Stripping the whitespace out is probably the wrong thing, as it may hide the problem from a page author using Mozilla to test his work. Most validating tools won't catch this, because they don't include URI parsers. Regarding the original problem page: when did <image> get in the HTML spec? Mozilla seems to understand <image> = <img>, but I don't know why it should.
Specs aside, there are existing pages that expect this behavior. All other browsers on the market (including Mozilla up to 21-April) do strip leading whitespace. Let's not make this bug a repeat of bug 57968 and ship a browser, then run around telling webmasters why their images are mysteriously not showing up. (If we must do this for strict mode, fine...) Also, this has been our behavior up until 21-April-2001, so there is good support for the theory that we won't encounter too many problems if we do strip the whitespace for all URLs...
But as I said, do not strip whitespace in the middle, unless we were doing that previously. We can (or at least used to) handle spaces in URIs. There are websites that actually use spaces in directory and file names... (in the middle, not in the beginning or end that I know of). Even Microsoft's BizTalk website had them when it originally appeared, but they changed it fast :)
First, I strongly oppose putting this change *outside* of Quirks mode. Thewhitespace is illegal, and the only reason we want to handle it at all is because legacy browsers did, hence QuirksMode. In Standard mode, we want to adhere to the specifications as closely as possibly, regardless of legacy behavior (God save the web!). Furthermore, I can see no reason to muck with whitespace inside the URL at all, just leading and trailing. Re: pavlov's comment: Yea, maybe it should be done in LoadImage for the reasons you stated and because it is actually easier there (less code). I'll redo tha patch.
I've attached a patch to trim the leading and trailing whitespace in the LoadImage method (in QuirksMode only). This will handle dynamic changes to the src attribute as well, but there is the additional cost of a string-copy of the src attribute values now. Please review patchID 39521 and lemme know what y'all think. Also, I still have not heard why this is critical to 0.9.2 and RTM+ - it mystifies me since it is marked severity:minor and there are no top50 URLs noted. Anyway, it is easy enough, so marking target milestone 0.9.2
Priority: -- → P3
Target Milestone: --- → mozilla0.9.2
There is a typo in the comment, and you do not need the quirksmode variable since you only test once. Otherwise looks ok to me.
Thanks Heikki, I fixed the typo and removed the quirksMode variable. Also, I moved the whitespace static array into the block where it is used (could not see any reason to remove it though).
Who is the sr= person for this bug?
Whiteboard: PDT+, critical for 0.9.2 → PDT+, r=heikki, need sr=
Good question - trying shaver (for no good reason)
Keywords: patch
Summary: IMG tags with a space in the src= path are not loaded → [PATCH] IMG tags with a space in the src= path are not loaded
To reiterate, the checkin on 21-April that broke this one case (<img src=>) also broke up to 20 other cases, listed above - every other case where a URL is specified as an attribute. This patch only addresses one of them. Should we open up a separate bug for the others? I've only tested a few, but, our solution seems to be the least consistent of any browser out there. Some strip whitespace, some do not. This makes our behavior probably all the more confusing/frustrating/misleading for web designers. <form action=" foo.cgi"> IE, Nav 4.x, and Mozilla now all handle this differently Netscape 6 handles it like IE. <body background=" foo.gif"> IE, Nav 4.x, and Netscape 6 all show this image, we do not. <script/input/img src=> IE, Nav 4.x, and Netscape 6 all handle the script src=, input type=image src=, and img src= cases, we only handle the img src= case For example, take a look at this page. Nav 4.x and IE render it one way (arguably incorrectly), and we go a different route: http://eric.302easyst.net/mozilla/bugs/attrws.html My feeling is that if we fix img src= we should fix the other cases too...
Keywords: patch
Summary: [PATCH] IMG tags with a space in the src= path are not loaded → IMG tags with a space in the src= path are not loaded
Whiteboard: PDT+, r=heikki, need sr= → PDT+, critical for 0.9.2
Eric, I think your reasoning is sound. I'd like to look at the problem at the sink level and see what I can come up with. Of course, this bug is marked PDT+ critical for some reason, so maybe I'll need to get this in before adressing the more fundamental problem. In that case, this hack should be removed if/when the more basic issue is resolved. Regarding the April change that regressed this: if the sink was stripping whitespace when it set attributes, then is it true that the dynamic case was not handled? Is that OK? Also, how much semantic knowledge do we want the sink to have about attribute values?
True, well... as this is PDT+, I think the patch above is definitely better than not taking any action at all, as it brings us one step closer to compatability!
I agree that whitespace should not be stripped except in quirks mode. The danger here is that whitepace might be signifigant in the case of an image that is actually generated by a cgi. sr=blizzard
After prodding from Mike. Can you guys make sure that you open bugs to deal with and investigate the other isses that Eric has brought up here? Making sure that we pick something to be consistent with and making a better long term solution? THanks.
Chofmann didn't say it when he added PDT+, but the reason to want this badly is that we have so many pages out there where we don't "just work" as other browsers do (or at least as well as 4.x ;-) I assume this class of quirks (including the spinoff bugs suggested by Eric) is a significant contributor to the problem. If this patch isn't going to introduce a bunch of risk, then I agree it's good to get what we have and work on doing better. If the patch might cause a bunch of regressions, I'd rather wait (unless current behavior is *worse* than what we shipped in N6.) What's the prognosis on fixing the problem in the general case to maximize compatibility with IE and N4.x across the full range of URIs with whitespace?
Whiteboard: PDT+, critical for 0.9.2 → PDT+, critical for 0.9.2, have patch, r and sr
Fixing the general problem is going to be more risky. I think the risk can be mitigated, however, by getting enough eyes on it. The change I am proposing is pretty simple, but it adds some semantic knowledge to the ContentSink that I don't think it should have. That said, there are many of examples of the Sink already doing this kind of element-specific / attribute-specific processing, mostly for quirks. I'll put a patch together that takes care of stripping leading and trailing whitespace from attributes while not regression the option values (the reason this was changed and regressed in the first place). Once the patch is there, people can look at it and decide if the risk is reasonable and the benefits desirable...
I attached a patch to the content sink that makes it strip out whitespace for attributes EXCEPT those that are in a special table of non-strippable attributes. Currently, the table is only built up of attribute names, though in theory it might need tag names too (since attributes in different contexts might have different legacy behavior). Currently, the table only has the 'value' attribute so it does not regress the option value bug (gets us back to where we were when bug 24346 was fixed, more or less). The benefit is that all of the cases that Eric Pollman enumerated are handled. The drawbacks to this approach are: cannot distinguish between Quirks and Standard mode, and a slight performance hit as each attribute has to be looked up in a small table. Reviews? Thoughts? Concerns? Come on, lemme have it!
Sounds backwards. We know what attributes have a URI type, so why not strip whitespace for only those attributes. Actually it always seemed to me that whitespace stripping should be done by whatever code is responsible for initiating the HTTP request, or resolving the relative URI. That code knows it is working on a URI, and that neither leading nor trailing whitespace belong there. That would definitely have less of a performance impact as well. Consulting a lookup table for every attribute can add up on pages like LXR, or W3C recs, others, but stripping whitespace need only happen when navigating to another page or loading style sheets, scripts, objects, and images.
Jeffrey, I agree with you, in fact the whole idea of stripping whitespace on attributes stinks if you ask me. I was trying to minimize the impact, and since we used to strip spaces for ALL attributes, and then changed it for the 'value' attribute bug, I coded up a patch that is closest to what we had been doing for some time. Clearly we were not discriminating between URL attributes and non-URL attributes, so why should we now? It all seems so wrong, really, but then what do I know? Bear in mind that we are EVEN NOW stripping whitespace on all attributes, just not the 'space' character. The current code has the Trim in there but no space char in the set. So, the impact of the patch is just to add a check against the set of attributes...
I would suggest that we'd move the whitespace stripping code to HTMLContentSink::AddAttributes() where we can do the name compares by simply comparing atom pointers and not having to do a ASCII and Unicode string comparison for all attributes, we already have the atom in HTMLContentSink::AddAttributes() so we might as well use it. PS. Since we're changing this code, we might as well replace Truncate() and Append() with one call to Assign() in HTMLContentSink::GetAttributeValueAt() too.
Please do not penalize standards mode performance to be compatible with illegal html. Lowest common denominator is not what standards mode is all about. Leave compatibility for quirks and Netscape.
Bug 83603 is caused by spaces in a script src tag... So if this one is fixed generically, close that one as well.
This is a 0.9.2 bug. Can someone summarize what's going on with this bug? Thanks.
Summary: we used to strip leading whitespace from all attributes in the ContentSink. We still do this, but we do not include the 'space' character in the set of whitespace chars to strip. So, now if a URL has a leading space, it is not handled properly. If it is a src attribute for an image, the image is not displayed. If it is a src attribute for a script tag, the script is not loaded. My initial stab at fixing this was to handle images only, and in quirks mode only. Then it was pointed out that lots of attributes need the same processing, so I made a hack into the content sink to basically revert it back to what it was doing back in April (i.e. always stripping whitespace, including spaces, except I excluded the 'value' attribute since that is the one that it was changed for). The big problem is, this is non-standard behavior - whitespace in the URL is illegal, and by stripping it we are not conforming to spec. In the content sink it is not really possible (I think) to distinguish between Standard and Quirks mode. There is also a small performance penalty in stripping whitespace from every attribute... Another proposed solution is to handle it in the URL parser - I was looking at stripping the whitespace in the NS_NewURI call, but I am not very familiar with that code, and not sure if it can be condition on Std. vs. Quirks mode. There is a patch attached that handles <IMG> only. The question is about the general case...
Adding dougt since he can give us advice on the URL parsing bits.
I was looking at the NS_NewURI methods in nsNetUtil and it looks like we could do the stripping there, but I cannot tell if the better place would be in the IOService itself. The NS_NewURI method is inline, which seems quite strange since it is called so often: if it is really being inlined then it seems like a cause of potential bloat, if it is not being inlined then it is probably screwing up code locality (anybody know if it is inlined on the various platforms?). I can take a stab at it, but I keep thinking about johnhall's plea to NOT penalize standards mode performance by stripping illegal whitespace - and I agree with him. Stripping at the URI parsing would cause this penalty.
But stripping at URI parsing is going to happen a lot less frequently than stripping in the attribute parsing.
PRessure is mounting: should I just checkin the hack in patch 39521 to fix the image problem, and then we can work on the general URL-stripping problem for 0.9.3? Please advise - I have r and sr but need approval from drivers...
This patch also fixes the problem, and might address some of the performance concerns. I copied the code that was already in nsStdUrl::SetSpec to strip leading whitespace from absolute url's, to nsStdUrl::Resolve. It may, in fact, only need to be in one of the two places. ??? Unfortunately, I don't really know this code all that well... As for performance: We are already doing the exact same logic for every URL that gets created in SetSpec(). In the normal case (no whitespace), the logic reduces to a few instructions. As for how often this code is called, in loading the testcase I posted above: http://eric.302easyst.net/mozilla/bugs/attrws.html SetSpec is called 109 times (no change here) Resove is called 16 times (this patch would add a few extra instructions per call) I am not advocating this as the correct solution, because I really, Really don't know what the ramifications would be, but I'm posting it here as yet another solution to consider, if needed.
Whiteboard: PDT+, critical for 0.9.2, have patch, r and sr → PDT+, critical for 0.9.2, have patch, r and sr; need a decision on what patch to use from drivers and people in bug
at this late stage in the game I'd rather take the lower risk hack (for images only) on the 0.9.2 branch and put the real fix on the trunk. a= asa@mozilla.org (on behalf of drivers) for images only fix on the 0.9.2 branch.
Whiteboard: PDT+, critical for 0.9.2, have patch, r and sr; need a decision on what patch to use from drivers and people in bug → PDT+, critical for 0.9.2, have patch, r and sr; a=drivers for image only fix on 0.9.2 branch
Patch for images only checked into trunk. The remaining, more general, issue is being dealt with both here and in bug 87298 - I'm going to open a new bug to cover the URL problem specifically, and reference it in both bugs (this and 87298).
Bug 87894 opened to cover remaining, general issue with whitespace in URLs. Marking this one FIXED
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Re-opening, can't see the spinning logo on https://webmail.jadedirect.com, or that cute little baby in the test case
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Terri, did you use a BRANCH build? This is BRANCH only...
Sorry, my comment on Additional Comments From Marc Attinasi 2001-06-26 14:47 was wrong, it should have said BRANCH, not trunk. This hacked fix is for the BRANCH only.
Alrighty, FIXED ON BRANCH ;-)
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Verified fixed w2k branch build 2001062803 Verified fixed linux branch build 2001062706 Verified fixed mac branch build 2001062809
Status: RESOLVED → VERIFIED
This has only been fixed on the 0.9.2 branch, so nothing regressed, the issue is still there.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: