96858 - [FIX]Relative URL in stylesheet resolved wrong if stylesheet has been redirected

Status: RESOLVED FIXED

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

Description

[Andreas M. "Clarence" Schneider]

If an external stylesheet is redirected to another location by an HTTP 30x
redirection, relative URLs in the stylesheet are resolved relative to the
original location instead of the new location.
See testcase at http://clarence.de/tests/style-redirect.html .
2001-08-24-03, Win NT.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Pierre -- I think this looks like a bug in the CSS loader.  The way to fix it
might be to do what the nsXULDocument::OnStreamComplete does when it gives an
error for a script load:

5630 nsCOMPtr<nsIRequest> request;
5631 aLoader->GetRequest(getter_AddRefs(request));
5632 nsCOMPtr<nsIChannel> channel;
5633 channel = do_QueryInterface(request);
5634 if (channel) {
5635     nsCOMPtr<nsIURI> uri;
5636     channel->GetURI(getter_AddRefs(uri));

...but I'm not sure.  If that's the case, it probably requires an additional
parameter to CSSLoaderImpl::ParseSheet, since the SheetLoadData's URL is the
wrong one to use.  I'm not sure how we'd handle it for the synchronous case,
although hopefully we don't need to worry about it there.

Comment 2

[Kevin McCluskey (gone)]

Moving to mozilla1.1. Engineers are overloaded!

Comment 3

[Kevin McCluskey (gone)]

Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling
work post Mozilla1.0.

Comment 4

[Madhur Bhatia]

cc'ing myself

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Assigning pierre's remaining Style System-related bugs to myself.

Comment 6

[Greg K.]

Reconfirmed using FizzillaCFM/2002070913. Setting All/All.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

This does a few things:

1)  Waits to set the URI of the sheet till after we get OnStartRequest (for
    those sheets which will get such).
2)  Changes CSSStyleSheetImpl::ContainsStyleSheet to not crash when called on a

    sheet with a as-yet-null uri (but assert instead)
3)  Removes some bogus code in the CSS parser that caused the crash/assert in
    ContainsStyleSheet; this incidentally fixes the testcase at
    
http://web.mit.edu/bzbarsky/www/testcases/css-loading/multipleImportsWithSameURI.html

Comment 8

[Boris Zbarsky [:bzbarsky]]

Taking

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Have you run http://www.hixie.ch/tests/evil/css/import/ ?  Does this break our
behavior on the recursive importing tests?  This seems to remove the only caller
of ContainsStyleSheet that really needs to exist.  (Probably we should load the
quirks stylesheet separately in the pres shell and avoid that caller.)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, I tested on those.  The real recursion prevention is done in the CSSLoader
anyway; if you consider the code I removed, there is no way it can prevent any
sort of recursion except a sheet linking to itself (since all it does is look at
the _kids_).

And yes, ContainsStyleSheet should probably die.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 127666 [details] [diff] [review]
Fix

ContainsStyleSheet is recursive; it doesn't just check the children.

>+    rv = NS_NewCSSStyleSheet(aSheet);  // Don't init it here, init it
>+                                       // once we know the final URI

This comment doesn't  make much sense.	"it" is ambiguous.  If you make the
comment make more sense, r+sr=dbaron.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(OK, maybe it does make some sense, but I was thinking you meant the URL rather
than the sheet.  But now I see that you meant the sheet.)

Did you test relative URLs in both style attributes and style elements?  I
assume you've tested them in external sheets...

Comment 13

[Boris Zbarsky [:bzbarsky]]

> ContainsStyleSheet is recursive; it doesn't just check the children.

Yeah, but it recurses down the sheet tree, whereas to test for the case of "A
imports B, B imports A" you need to recurse up the tree and check the parents....

I have indeed tested relative URLs in inline style, <style> elements, and in
linked sheets of various  sorts.

Comment changed to:

  // Don't init the sheet here, but in
  // ParseSheet once we know the final URI

and patch checked in.