1541861 - CSSStyleSheet.prototype.deleteRule confusing error message

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, enhancement, P3)

Description

[mikes.ekb]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

https://jsfiddle.net/3j0dnagp/embedded/

Try to call .deleteRule(0) for empty style sheet:

1. const styleEl = document.createElement('style');
2. document.head.appendChild(styleEl);
3. styleEl.sheet.deleteRule(0);

Actual results:

Error is thrown - that's ok, just by the standard, but the message itself is confusing:
"IndexSizeError: Index or size is negative or greater than the allowed amount"

Also, according to CSSOM (https://heycam.github.io/webidl/#indexsizeerror), error thrown should be instance of RangeError.

Comment 1

[Rares Doghi, Desktop QA]

Hi, I managed to reproduce this issue in our latest Firefox Nightly 68.0a1 (2019-04-07), Release 66.0.2 as well as Beta 67.0b8.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

I think the error comes from layout/style/StyleSheet.cpp#1155.

Emilio, I think you're the latest person that changed this line (even if it was just moving it from a Servo file).
Could you have a look at it, or ping someone who could look at it so the error makes more sense? Thanks!

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Also, according to CSSOM (https://heycam.github.io/webidl/#indexsizeerror), error thrown should be instance of RangeError.

The spec is here, and it says to throw an IndexSizeError (even though WebIDL says it's deprecated, which sounds plausible), and all other browsers also throw an IndexSizeError. So that's probably not going to change.

We could try to improve the error message for IndexSizeError, though, I guess... Boris, do you if we have any error which is formatted using some context (e.g, index given and maximum index in this case or such)? I'd be happy to try giving that a shot.

Also, I noticed that if you open: data:text/html,<style></style> and run: try { document.styleSheets[0].removeRule(0) } catch (e) { console.log(e.name) }, we log TypeError, but WebKit and Blink log IndexSizeError. Looking at https://heycam.github.io/webidl/#indexsizeerror I'd expect "IndexSizeError" to be logged. Do you know if that's tracked somewhere or if WebKit and Blink are wrong?

Thanks.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So a few things:

1. It's not clear to me why the error message is so confusing to people. The passed-in index is in fact "greater than the allowed amount". I agree that, being a very generic error message, it's not very specific to this situation.

2. It's easy to have a custom error message here instead of the default. Instead of using ErrorResult::Throw, use ErrorResult::ThrowDOMException, which takes two arguments: an nsresult that will affect what the .name of the resulting DOMException is (in this case NS_ERROR_DOM_INDEX_SIZE_ERR) and an nsACString for the DOMException's .message. You can use nsPrintfCString to create the latter and have it include whatever data you want (e.g. in this case aIndex and mRuleList->Length()).

3. For the removeRule testcase described at the end of comment 3, what's going on there is that the spec, and Gecko, do not have a removeRule method at all. It's called deleteRule in the spec and Gecko. So that testcase throws a TypeError in JS without ever getting into the DOM, with message "document.styleSheets[0].removeRule is not a function". It looks like WebKit and Blink have both removeRule method in addition to deleteRule. See https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/css_style_sheet.idl?l=37-40&rcl=6dc816e543555790648ec5af1ca827431a7d5807 for the various "non-standard APIs" they have on their CSSStyleSheet. The impl at https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/css_style_sheet.h?l=102&rcl=20c676afc838a18b5cbda662a462d1cee0946dc7 just calls deleteRule; we should probably just add that to Gecko (and the spec), or ask them to remove it.

Comment 5

[mikes.ekb]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)

1. It's not clear to me why the error message is so confusing to people. The passed-in index is in fact "greater than the allowed amount". I agree that, being a very generic error message, it's not very specific to this situation.

This message is confusing because it implies that there is some valid index that you can pass and get valid result. Even something simple like "Index is invalid" is better in this case.

Comment 6

[Boris Zbarsky [:bzbarsky]]

For the special case of the list length being 0, I guess....

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)

3. For the removeRule testcase described at the end of comment 3, what's going on there is that the spec, and Gecko, do not have a removeRule method at all. It's called deleteRule in the spec and Gecko. So that testcase throws a TypeError in JS without ever getting into the DOM, with message "document.styleSheets[0].removeRule is not a function". It looks like WebKit and Blink have both removeRule method in addition to deleteRule. See https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/css_style_sheet.idl?l=37-40&rcl=6dc816e543555790648ec5af1ca827431a7d5807 for the various "non-standard APIs" they have on their CSSStyleSheet. The impl at https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/css_style_sheet.h?l=102&rcl=20c676afc838a18b5cbda662a462d1cee0946dc7 just calls deleteRule; we should probably just add that to Gecko (and the spec), or ask them to remove it.

Blerg. Filed https://github.com/w3c/csswg-drafts/issues/3814.

Comment 8

[mikes.ekb]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

The spec is here, and it says to throw an IndexSizeError (even though WebIDL says it's deprecated, which sounds plausible), and all other browsers also throw an IndexSizeError. So that's probably not going to change.

Chromium switched to RangeError - https://chromium.googlesource.com/chromium/src.git/+/b2caae9ede888ede7d662c8760da9ab765904571

Comment 9

[Emilio Cobos Álvarez (:emilio)]

sigh

And that's wrong, commented on https://bugs.chromium.org/p/chromium/issues/detail?id=949523.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

We can have a custom error message, but switching to RangeError should be proposed and agreed on the CSS WG.

Comment 11

[Blito]

Hi! Can I take this bug? I've been looking forward to contributing and this seems like a low-hanging fruit.

I already built Firefox and identified the line that has to change (tested by changing the exception type and it is indeed the line): https://dxr.mozilla.org/mozilla-central/source/layout/style/StyleSheet.cpp#1201

I was thinking of going with Boris' approach outlined in comment #3.

Do we have a strong opinion on what the error message should say? Are there any error style guides that I should follow? (I'll check later just in case, I wanted to claim this before leaving for today).

Thanks!

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Sure, that sounds great, thank you for picking this up!

Yeah, what Boris said is the right thing to do (Boris is usually right ;)).

I personally don't have a strong opinion on the message. Just for my own reference, from a quick test, it seems that custom error messages don't prepend the generic error message (which makes sense). For example the following error is emitted when I run navigator.registerProtocolHandler("yadayada", "foo%s", "yada yada"), and it doesn't prepend The operation is insecure. (which is the message in here).

You can take a look at the other [callers of the method](https://searchfox.org/mozilla-central/search?q=ThrowDOMException(&case=false&regexp=false&path=) for reference, but I don't spot any particularly interesting pattern. Given the code is shared for both removeRule and deleteRule, including the name of the method may not be great.

So maybe just something like: "Cannot delete rule at index %u (length %u)" or something of that sort? How does that sound?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Also let me know if you have any questions or any issue comes up. Thanks!

Comment 14

[Boris Zbarsky [:bzbarsky]]

"Cannot delete rule at index %u because the number of rules is only %u" or so might work.

Comment 15

[Blito]

Ok, I've made the changes according to Boris' comment. I reproduced the bug manually and it's fixed.

From ./mach build output I see there's some testing being done. Should I assume there are no tests validating the exact string of this exception? Should I add one?

I'll read more about the review process tomorrow and submit my change for review (unless I have to add tests to check the new exception string).

Thanks for your help!

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Probably not, once you submit the patch we can send it to the try server to ensure it comes back green. There are a lot of tests, off-hand from a quick search there's no test for the exception string.

You can add one to layout/style/test/, like layout/style/test/test_group_insertRule.html (of course only one subtest is needed, probably).

Can be a separate patch as well if you prefer.

Comment 17

[Blito]

Attachment: Bug 1541861 - Improve exception message for CSSStyleSheet.prototype.deleteRule

CSSStyleSheet.prototype.deleteRule can throw an IndexSizeError. This change
makes it so that the exception thrown has the max valid index for that function.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13a4951a9f46
Improve exception message for CSSStyleSheet.prototype.deleteRule r=emilio

Comment 19

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/13a4951a9f46

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.