Last Comment Bug 649134 - Blank link rel causes subsequent conditional CSS to be displayed
: Blank link rel causes subsequent conditional CSS to be displayed
Status: RESOLVED FIXED
: html5, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla6
Assigned To: Mats Palmgren (:mats)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 667518
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-11 14:31 PDT by cfrommann
Modified: 2011-06-27 10:39 PDT (History)
5 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (646 bytes, text/html)
2011-04-11 19:25 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #2 (648 bytes, text/html)
2011-04-12 08:33 PDT, Mats Palmgren (:mats)
no flags Details
Like so? (4.71 KB, patch)
2011-04-12 08:41 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Splinter Review
Testcase (549 bytes, text/html)
2011-04-12 09:34 PDT, Mats Palmgren (:mats)
no flags Details
part 1, fix <link> element (4.68 KB, patch)
2011-04-15 11:31 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 2, fix HTTP Link field for HTML documents (5.99 KB, patch)
2011-04-15 11:44 PDT, Mats Palmgren (:mats)
bzbarsky: review-
Details | Diff | Splinter Review
part 1, fix <link> element (4.02 KB, patch)
2011-04-21 12:20 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
part 2, fix HTTP Link field for HTML documents (7.78 KB, patch)
2011-04-21 12:20 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Splinter Review

Description cfrommann 2011-04-11 14:31:36 PDT
User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 6.0; SLCC1; .NET CLR 2.0.50727; InfoPath.2; .NET CLR 1.1.4322; .NET CLR 3.0.30729; .NET CLR 3.5.30729; .NET4.0C; MS-RTC LM 8)
Build Identifier: Mozilla/5.0 (Windows NT 6.0; rv:2.0) Gecko/20100101 Firefox/4.0

If there is a blank stylesheet link reference anywhere in a document, all subsequent CSS except the first selector (regardless of whether or not it's in a conditional comment) is rendered.

e.g.

<link rel="stylesheet" type="text/css" href="" />
<!--[if lte IE 8]>
	<style type="text/css">
		#foo {
			/* This doesn't get evaluated */
		}
		#ie {
			border: 5px solid red;
		}
		#ie {
			display: block;
		}
		#moz {
			display: none;
		}
	</style>
<![endif]-->

#foo is not rendered, subsequent rules are

Reproducible: Always

Steps to Reproduce:
1. Include a link rel with a blank href
2. Include subsequent css in a conditional comment
Actual Results:  
All but the first rule will be displayed, despite the conditional comment

Expected Results:  
The rules will only be rendered if the conditions are met
Comment 1 Mats Palmgren (:mats) 2011-04-11 19:25:59 PDT
Created attachment 525276 [details]
Testcase
Comment 2 Mats Palmgren (:mats) 2011-04-11 19:35:12 PDT
This is fun stuff :-)

The href="" is a reference to the document itself, so it loads that
as text/css and finds that everything leading up to the first {
is a bogus CSS selector and thus discards the first rule, then there's
three valid rules and then some junk at the end...

As far as I know this works as expected, unless we want to make an
exception here like we did in some other case... was it for images?
Comment 3 Henri Sivonen (:hsivonen) 2011-04-11 23:06:20 PDT
The spec changed the conformance requirements to require a non-empty URL there: http://www.whatwg.org/specs/web-apps/current-work/#attr-link-href

It's not clear to me if the spec was intended also to ignore empty URLs in processing. Currently, AFAICT, the spec doesn't have the right wording to say that if it was intended.
Comment 4 Mats Palmgren (:mats) 2011-04-12 08:33:49 PDT
Created attachment 525391 [details]
Testcase #2
Comment 5 Mats Palmgren (:mats) 2011-04-12 08:39:31 PDT
I think it's quite clear [that it should be ignored] in the steps to obtain the resource a few paragraphs down.  The first step is "If the href attribute's value is the empty string, then abort these steps.".  I think this implicitly
also applies to attribute values with only whitespace;  it follows from
"valid non-empty URL potentially surrounded by spaces" at the link you gave.

Google Chrome 12 loads it.  Opera 11, Firefox 3.6.x and IE9 ignores it.
Comment 6 Mats Palmgren (:mats) 2011-04-12 08:41:24 PDT
Created attachment 525394 [details] [diff] [review]
Like so?
Comment 7 Mats Palmgren (:mats) 2011-04-12 09:03:41 PDT
Actually, Firefox 3.6.x and IE9 also loads it - using the testcase
attachment links gave the wrong result since it has a query string in it.
So, it's only Opera 11 that ignores it.
Comment 8 Mats Palmgren (:mats) 2011-04-12 09:34:15 PDT
Created attachment 525405 [details]
Testcase

Bah, I have to correct myself again.  IE9 does in fact ignore the empty href.
Here's a better testcase that removes the <style> and IE-conditional markup.
(still have to save it to a local file)

So, Opera and IE9 ignores it, Chrome and Firefox 3.6.x loads it.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-04-12 22:44:55 PDT
Comment on attachment 525394 [details] [diff] [review]
Like so?

Do we need something similar for Link HTTP headers?

There's no point in doing the HasAttr check.  Just do the GetAttr call and then do the whitespace thing... and you probably need to file a bug on the spec about whitespace.

r=me with those nits.
Comment 10 Mats Palmgren (:mats) 2011-04-15 11:31:31 PDT
Created attachment 526320 [details] [diff] [review]
part 1, fix <link> element

Nit fixed.  Simplified the reftest somewhat.
Filed HTML spec bug: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12508
Comment 11 Mats Palmgren (:mats) 2011-04-15 11:44:28 PDT
Created attachment 526329 [details] [diff] [review]
part 2, fix HTTP Link field for HTML documents

> Do we need something similar for Link HTTP headers?

Maybe.  The HTTP 1.1 definition of space [1] is different from HTML5's -
it doesn't include \f.  So a space-only URI reference in a Link field
that includes \f would be loaded (see testcases in this patch).

[1] http://tools.ietf.org/html/rfc2616#section-2.2
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-04-21 09:26:57 PDT
Comment on attachment 526329 [details] [diff] [review]
part 2, fix HTTP Link field for HTML documents

This seems to be missing test_bug649134.html.  And why do you need the separate directory?  Just putting the new files in content/html/content/test should work, right?
Comment 13 Mats Palmgren (:mats) 2011-04-21 12:20:11 PDT
Created attachment 527614 [details] [diff] [review]
part 1, fix <link> element

Updated to fix merge conflicts in reftest.list
Comment 14 Mats Palmgren (:mats) 2011-04-21 12:20:49 PDT
Created attachment 527615 [details] [diff] [review]
part 2, fix HTTP Link field for HTML documents

(In reply to comment #12)
> This seems to be missing test_bug649134.html.

Sorry about that.

> And why do you need the separate directory? Just putting the new files
> in content/html/content/test should work, right?

It's needed for the http://mochi.test server to fail without the patch.
Here's how the test works - when the bug649134/file_bug649134-*.sjs
file is loaded, it responds with the white-space Link: header
which we will now ignore and not load, this part would work without
the extra directory.

Without the fix, we would load the directory path where the .sjs is
located http://mochi.test:8888/tests/content/html/content/test/
but the mochitest server handles such URLs internally and responds
with the list of tests to run, but there's a way to avoid that:
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js#2464
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-04-21 13:18:16 PDT
Comment on attachment 527615 [details] [diff] [review]
part 2, fix HTTP Link field for HTML documents

OK.  Please just do this for all documents (I don't see why the Link header handling should depend on the type of document), and add a comment in the file explaining why the extra subdirectory is needed, and r=me.
Comment 17 Mats Palmgren (:mats) 2011-06-27 10:39:42 PDT
Filed bug 667518, based on the "Editor's Response" in 
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12508#c2
which says we should load a non-empty white-space-only @href.

Note You need to log in before you can comment on or make changes to this bug.