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

RESOLVED FIXED in mozilla1.5beta

Status

()

Core
CSS Parsing and Computation
P2
minor
RESOLVED FIXED
16 years ago
15 years ago

People

(Reporter: Andreas M. "Clarence" Schneider, Assigned: bz)

Tracking

({css1})

Trunk
mozilla1.5beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CSS1-6.4], URL)

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
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.
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.
Assignee: dbaron → pierre
Keywords: css1

Updated

16 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.9
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

Comment 4

16 years ago
cc'ing myself
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
Status: ASSIGNED → NEW

Comment 6

16 years ago
Reconfirmed using FizzillaCFM/2002070913. Setting All/All.
OS: Windows NT → All
Hardware: PC → All
Whiteboard: [CSS1-6.4]
Created attachment 127666 [details] [diff] [review]
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
Attachment #127666 - Flags: superreview?(dbaron)
Attachment #127666 - Flags: review?(dbaron)
Taking
Assignee: dbaron → bzbarsky
Priority: P1 → P2
Summary: Relative URL in stylesheet resolved wrong if stylesheet has been redirected → [FIX]Relative URL in stylesheet resolved wrong if stylesheet has been redirected
Target Milestone: Future → mozilla1.5beta
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.)
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 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.
Attachment #127666 - Flags: superreview?(dbaron)
Attachment #127666 - Flags: superreview+
Attachment #127666 - Flags: review?(dbaron)
Attachment #127666 - Flags: review+
(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...
> 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.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.