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: