Closed Bug 112644 Opened 23 years ago Closed 22 years ago

[FIX]Ignores 404 error while loading style sheets

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Future

People

(Reporter: u39892, Assigned: bzbarsky)

References

(
URL
)

Details

Attachments

(2 files, 1 obsolete file)

Example of the bug
7.17 KB, application/octet-stream
Details
Same, with better error-checking
2.07 KB, patch
bbaetz
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
From Bugzilla Helper: User-Agent: Mozilla/4.78 [en] (X11; U; Linux 2.4.9-13custom i686; Nav) BuildID: 2001112012 When loading a page that includes a tag such as: <link href="style/default.css" rel="stylesheet" type="text/css" /> Mozilla (correctly) tries to load the file "style/default.css". However, if this file does not exist (a 404 error is returned), but the 404 error page does include inline style information, then that inline style information is used to render the original page. Reproducible: Always Steps to Reproduce: 1. Create a page with a broken link to a seperate style sheet. 2. Retreive that page from an Apache Jakarta Tomcat server (I'm using version 4.0.1 for these tests). Actual Results: The page renders using the style sheet information of the web server's error page. Expected Results: The browser should return an error of some form (not necessarily actually displaying the 404 page itself - a popup requester would probably suffice). In particular it should not use style sheet information returned in a page with an error status code, in other pages. There are more obvious example pages, but none I can provide easily. However, there are cases (for example) of the text background being changed to blue (the color defined in the Tomcat error page).
Attached file Example of the bug —
Very simple example, just untar/gzip this and view "index.html". This page doesn't demonstrate the problem brilliantly, but if you look at the font (it should render as Times by default, but comes up as Helvetica) for an example of the problem.
That url does not have any inline style in the error page.. This problem should also only occur in quirks mode now.
taking
Assignee: dbaron → pierre
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → mozilla0.9.9
Re: Error page not containing inline style sheet Sorry, had to move the example between servers, and didn't check Tomcat verions. Server updated and the correct error page (with inline style sheet) is now returned by it.
Moving to mozilla1.1. Engineers are overloaded!
Target Milestone: mozilla0.9.9 → mozilla1.1
Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling work post Mozilla1.0.
Priority: P3 → P1
Target Milestone: mozilla1.1 → Future
I'm not sure we still want to fix this bug... We now (in strict mode) ignore non-text/css content that's served as a sheet. If a site has a text/css 404 page, should we go ahead and parse it? Ian? (adding qawanted keyword). If we decide we _do_ want to fix this, I can do it without too much trouble, I think.
Keywords: qawanted
cc'ing myself
In my opinion, even in that case we should try to use the CSS parser. The parser WILL return a parse error because <html> in the content is not a valid selector. So the whole stylesheet must be ignored not because it is reported as being text/html, but because it is invalid CSS which can't be parsed according to the syntax. Even in quirks mode, you should not recover from parse errors in CSS files. In strict mode, any parse error in the CSS file must have the whole stylesheet ignored. In both case, the browser can display an alarm to the user saying that the CSS file was invalid and ignored.
Please don't just spread the discussion to more bugs (bug 53112, bug 136529, bug 113399, and here). I already said in bug 136529 comment 8 that comment 9 above is incorrect.
This is a tough one. I don't think sending a stylesheet as the entity body of a 404 is even valid. A stylesheet is supporting material, not a document in itself, and a 404 entity body should be a document in itself. I think we should not use the body of any of the 4xx (or 3xx in the case of, e.g., 310 or 302, or 5xx) error pages as stylesheets. Basically, if we don't get a 200 or a cached version, then we shouldn't be trying to use it as a stylesheet. (If we ever get UI that reports the missing stylesheet as an error somewhere, then we should link to the error page and show it then.) So I think this bug is valid. Since bz's worked on this and pierre is unlikely to ever fix this, reassigning to bz.
Assignee: pierre → bzbarsky
Keywords: qawanted
So. Let me get the desired behavior straight. if (code is 2xx) parse sheet else report error somewhere Right? Darin? Does that seem reasonable? I'm not sure what if anything we should do about 1xx codes here... I suspect that this code is simply never called with a channel that has a 1xx status.
Assuming that 201 Created will never happen, and that 301 Moved Permanently, 302 Moved Temporarily, 303 See Other, 304 Not Modified, 305 Use Proxy, 401 Unauthorized, and 407 Proxy Authentication Required are handled transparently by the network layer, then that sounds perfect to me.
what hixie said, and 1xx responses should not be seen by the CSS loader.
Attached patch Possible fix (obsolete) — — Splinter Review
This works (the first two hunks are compile warning fixes that I figured I may as well include). I can't shake the feeling that the aStatus argument to OnStreamComplete should tell me _something_ useful, however. Some docs on that interface would be nice. ;)
Comment on attachment 87837 [details] [diff] [review] Possible fix The problem with the status arguments is that they just tell you that some 'thing' loaded corretly. This thing could be a proxy error page, a 404 page, or anything else. Tahts why we have the linkchecker thing - I lost the argument to have an extra status value for this sort of thing on nsIRequest. Necko will hide the redirect/revalidation 3xx response code from the end user, so that part should be ok.
Attachment #87837 - Flags: needs-work+
Comment on attachment 87837 [details] [diff] [review] Possible fix wrong box, oops
Attachment #87837 - Flags: needs-work+ → review+
Comment on attachment 87837 [details] [diff] [review] Possible fix >Index: html/style/src/nsCSSLoader.cpp >+ nsCOMPtr<nsIRequest> request; >+ aLoader->GetRequest(getter_AddRefs(request)); >+ // If it's an HTTP channel, we want to make sure this is not an >+ // error document we got. >+ PRBool realDocument = PR_TRUE; >+ nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(request)); >+ if (httpChannel) { >+ PRUint32 statusCode; >+ httpChannel->GetResponseStatus(&statusCode); you need to check the return value of GetResponseStatus for sure, and what about the call to GetRequest above? is that known to always set it's output param to NULL on failure?
Attachment #87837 - Flags: needs-work+
Attachment #87837 - Attachment is obsolete: true
Summary: Ignores 404 error while loading style sheets → [FIX]Ignores 404 error while loading style sheets
Comment on attachment 88007 [details] [diff] [review] Same, with better error-checking r=bbaetz GetResponseStatus can't fail at this point unless something is really really wrong, but...
Attachment #88007 - Flags: review+
Comment on attachment 88007 [details] [diff] [review] Same, with better error-checking sr=darin bbaetz: ...but the CSS module isn't supposed to know the details of the nsHttpChannel implementation.
Attachment #88007 - Flags: superreview+
Checked in on the trunk.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
verified as per the comments
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: