1312050 - loadAndRegisterSheet should give a helpful error message if the CSS contains an unescaped #id

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

At least a few devs had wasted considerable amounts of time due to this, which would be preventable if we logged an error in the Browser Console about this.

See https://bugzilla.mozilla.org/show_bug.cgi?id=659650#c4 and https://bugzilla.mozilla.org/show_bug.cgi?id=659650#c5 as well as recent work that was done as part of bug 1311352.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Steps to Reproduce:
1. Start Nightly/7.0a1 with clean profile
2. Enable App Button (Windows7 default)
3. Open Error Console(Ctrl+Shift+j)
4. Evaluate the following code (Copy and Paste into textbox and click Evaluete)

var cssURL = "data:text/css,@namespace%20url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul);#appmenu-button{color:red!important;}";
var u = Components.classes["@mozilla.org/network/io-service;1"]
        .getService(Components.interfaces.nsIIOService)
        .newURI(cssURL, null, null);
Components.classes["@mozilla.org/content/style-sheet-service;1"]
          .getService(Components.interfaces.nsIStyleSheetService)
          .loadAndRegisterSheet(u,
                      Components.classes["@mozilla.org/content/style-sheet-service;1"]
                      .getService(Components.interfaces.nsIStyleSheetService)
                      .USER_SHEET);

Actual Results:  
CSS load fails
No error message in Browser Console

Expected Results:  
CSS load fails.
Browser Console includes messages saying that "Stylesheet URIs must not include an unescaped hash character."

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

Review commit: https://reviewboard.mozilla.org/r/87726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87726/

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

https://reviewboard.mozilla.org/r/87726/#review86754

::: layout/base/nsStyleSheetService.cpp:142
(Diff revision 1)
>                                            uint32_t aSheetType)
>  {
> -  nsresult rv = LoadAndRegisterSheetInternal(aSheetURI, aSheetType);
> +  nsAutoCString ref;
> +  nsresult rv = aSheetURI->GetRef(ref);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (aSheetURI && !ref.IsEmpty()) {

This might catch some of the cases, but not:

`LoadAndRegisterSheet(NewURI(".toggle { background: fuchsia; } #foo { color: green; }"))`

Which does keep `.toggle { background: fuchsia; }` but omits the rest. `ref.aEmpty()` will return `false` here.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

https://reviewboard.mozilla.org/r/87726/#review86754

> This might catch some of the cases, but not:
> 
> `LoadAndRegisterSheet(NewURI(".toggle { background: fuchsia; } #foo { color: green; }"))`
> 
> Which does keep `.toggle { background: fuchsia; }` but omits the rest. `ref.aEmpty()` will return `false` here.

This case is handled by the code, explicitly because ref is not empty and thus the error text is shown and the function returns NS_ERROR_ILLEGEAL_VALUE.

Browser Console output from running the code:
> var cssURL = ".toggle { background: fuchsia; } #foo { color: green; }";
> var u = Components.classes["@mozilla.org/network/io-service;1"]
>         .getService(Components.interfaces.nsIIOService)
>         .newURI(cssURL, null, null);
> Components.classes["@mozilla.org/content/style-sheet-service;1"]
>           .getService(Components.interfaces.nsIStyleSheetService)
>           .loadAndRegisterSheet(u,
>                       Components.classes["@mozilla.org/content/style-sheet-service;1"]
>                       .getService(Components.interfaces.nsIStyleSheetService)
>                       .USER_SHEET);
> [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 2"  data: no]
> 
> 
> 
> var cssURL = "data:text/css,@namespace%20url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul);.toggle { background: fuchsia; } #foo { color: green; }";
> var u = Components.classes["@mozilla.org/network/io-service;1"]
>         .getService(Components.interfaces.nsIIOService)
>         .newURI(cssURL, null, null);
> Components.classes["@mozilla.org/content/style-sheet-service;1"]
>           .getService(Components.interfaces.nsIStyleSheetService)
>           .loadAndRegisterSheet(u,
>                       Components.classes["@mozilla.org/content/style-sheet-service;1"]
>                       .getService(Components.interfaces.nsIStyleSheetService)
>                       .USER_SHEET);
> [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIStyleSheetService.loadAndRegisterSheet]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 5"  data: no]
> Stylesheet URIs must not include an unescaped hash character.

I'm not hard pressed on returning early here, but I thought it would also help devs if this is buried deep in their code since an exception will get thrown.

Comment 5

[Masatoshi Kimura [:emk]]

(In reply to [Limited availability until Oct 31] Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Expected Results:  
> CSS load fails.

Why? The truncate part makes up a valid (but empty) CSS.
We should load the sheet (as before) and log a warning message.

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

https://reviewboard.mozilla.org/r/87726/#review86776

Comment 7

[Masatoshi Kimura [:emk]]

(MozReview didn't add a bugzilla comment, so adding it myself)

(In reply to [Limited availability until Oct 31] Jared Wein [:jaws] (please needinfo? me) from comment #4)
> I'm not hard pressed on returning early here, but I thought it would also
> help devs if this is buried deep in their code since an exception will get
> thrown.

I don't think it is correct to fail early because a URL may contain a real hash part. (Don't forget this is exposed to web content.)

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87726/diff/1-2/

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

https://reviewboard.mozilla.org/r/87726/#review86778

::: layout/base/nsStyleSheetService.cpp:147
(Diff revisions 1 - 2)
>    if (aSheetURI && !ref.IsEmpty()) {
>      nsCOMPtr<nsIConsoleService> consoleService =
>        do_GetService(NS_CONSOLESERVICE_CONTRACTID);
>      NS_WARNING_ASSERTION(consoleService, "Failed to get console service!");
>      if (consoleService) {
> -      nsAutoString msg(NS_LITERAL_STRING("Stylesheet URIs must not include an unescaped hash character."));
> +      nsAutoString msg(NS_LITERAL_STRING("nsStyleSheetService::LoadAndRegisterSheet: Stylesheet URIs should not include an unescaped hash character."));

Is it OK that the message is non-localizable?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

I didn't localize this because I saw other developer-related messages sent to the console service that weren't localized, such as http://searchfox.org/mozilla-central/source/chrome/nsChromeRegistry.cpp#170,179

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87726/diff/2-3/

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to [Limited availability until Oct 31] Jared Wein [:jaws] (please needinfo? me) from comment #4)
> This case is handled by the code, explicitly because ref is not empty and
> thus the error text is shown and the function returns
> NS_ERROR_ILLEGEAL_VALUE.

Aah, ok cool. Thanks for checking!

Comment 13

[Masatoshi Kimura [:emk]]

LGTM, but a layout peer sign-off (I couldn't find a way to add an overall comment on MozReview...).

Comment 14

[Masatoshi Kimura [:emk]]

(By the way, I didn't mean to r- in the second round. MozReview didn't allow changing the review flag after that. MozReview really sucks.)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

https://reviewboard.mozilla.org/r/87726/#review87226

Semi-drive-by-review comments -- so, apologies if I'm missing some obvious pieces; haven't done much research at this point.  (I'm on vacation right now but will be back fully on Wednesday.)

Not granting r+ yet, because (per my comments below) I don't yet understand the purpose of this warning.  But, here are a few things to fix in the meantime (including adding a bit more explanation of the badness):

::: layout/base/nsStyleSheetService.cpp:139
(Diff revision 3)
> -  nsresult rv = LoadAndRegisterSheetInternal(aSheetURI, aSheetType);
> +  nsAutoCString ref;
> +  nsresult rv = aSheetURI->GetRef(ref);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (aSheetURI && !ref.IsEmpty()) {

I believe you can use nsIURI::HasRef() here.

(instead of having to request the ref as a string, and then check whether it's empty)

::: layout/base/nsStyleSheetService.cpp:147
(Diff revision 3)
> +      nsAutoString msg(NS_LITERAL_STRING("nsStyleSheetService::LoadAndRegisterSheet: Stylesheet URIs should not include an unescaped hash character."));
> +      consoleService->LogStringMessage(msg.get());

Three things:

(1) The code should be wrapped at 80 characters. For the long string, in C++, you can have
  "consecutive quoted strings "
  "like these ones "
which get concatenated by the compiler.

(2) No need to create nsAutoString msg here, AFAICT. Just do
  consoleService->LogStringMessage(
    NS_LITERAL_STRING("foo "
                      "bar.");

(3) Please include a code-comment somewhere here explaining *why* this is bad & worth warning about. It's not obvious to me, right away. I imagine if I dug into some of the linked bugs from comment 0, I might find out -- but future code-readers should benefit from a brief comment about why we care about this condition, too.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

https://reviewboard.mozilla.org/r/87726/#review88662

(r- for now, though this will likely be r+ once my earlier review feedback is addressed.)

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87726/diff/3-4/

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

https://reviewboard.mozilla.org/r/87726/#review89334

r=me, with two wording nits addressed:

::: layout/base/nsStyleSheetService.cpp:139
(Diff revision 4)
> -  nsresult rv = LoadAndRegisterSheetInternal(aSheetURI, aSheetType);
> +  // Warn developers if their sheet (defined via a data URI) has any styles
> +  // defined after a hash character (used as the ID selector) since those
> +  // styles will get discarded.

Thanks for adding this comment! It helps clarify what you're trying to address here.

However -- this comment puts the cart before the horse a bit.  It assumes/implies 4 unfounded things: (a) a hash character in the URL is a hash character in "their sheet", (b) aSheetURI is a data URI, (c) any text after the hash mark is definitely "styles", and (d) an unescaped hash mark is definitely an ID selector.

Let's rewrite this to avoid implying that we know all those things. How about something like this:

  // Warn developers if their stylesheet URL has a #ref at the end.
  // Stylesheet URIs don't benefit from having a #ref suffix -- and if the
  // sheet is a data URI, someone might've created this #ref by accident (and
  // truncated their data-URI stylesheet) by using an unescaped # character in
  // a #RRGGBB color or #foo{} ID-selector in their data-URI representation.

::: layout/base/nsStyleSheetService.cpp:150
(Diff revision 4)
> +      const char16_t* message = u"nsStyleSheetService::LoadAndRegisterSheet: "
> +        u"Stylesheet URIs should not include an unescaped hash character.";

As above, this message is a bit stronger than it should be. It's hand-wavingly saying "should not" -- who says they should not do this, and why? If I want to register "chrome://foo/my-styles.css#blah" as a stylesheet, who's to say I "should not" do that?

Please reword the second line of the message here to something clearer, like:
        u"URI contains unescaped hash character, which might be truncating"
        u"the sheet, if it's a data URI.";

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

https://reviewboard.mozilla.org/r/87726/#review89336

(Sorry, meant to use r+, not r-)

Also, a few more nits while I'm here, which I've just noticed for the commit message:
> Bug 1312050 - loadAndRegisterSheet should give a helpful error message if the CSS contains an unescaped #id. r?dholbert

(1) "the CSS" is incorrect here. You're not checking "the CSS" -- you're checking the URI.  (The URI *might* contain the CSS, if it's a data URI; but it usually does not.)

(2) The wording with "should" is vague -- it expresses a hope, but doesn't quite say what's changing.  Commit messages need to be phrased so as to describe the change, rather than describing the problem/request, per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment  (I've observed that frontend commits don't always follow this & sometimes just copypaste bug titles into commit messages, but platform code is more strict/consistent about following this guideline.)

So, to address both of these, how about the following wording:
  Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains an potentially-truncating unescaped #id. r=dholbert

r=me with something like that.

Comment 20

[Daniel Holbert [:dholbert]]

(er, s/an/a/ in that suggested commit message text, of course. :))

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8803468 [details]
Bug 1312050 - Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87726/diff/4-5/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3f5d4fc5241
Make loadAndRegisterSheet log a helpful error message if the URI contains a potentially-truncating unescaped #id. r=dholbert

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a3f5d4fc5241