Closed Bug 1541861 Opened 8 months ago Closed 4 months ago

CSSStyleSheet.prototype.deleteRule confusing error message

Categories

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

66 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: mikes.ekb, Assigned: pabloarubi, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++])

Attachments

(1 file)

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.

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.

Status: UNCONFIRMED → NEW
Component: Untriaged → Console
Ever confirmed: true
Product: Firefox → DevTools

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!

Flags: needinfo?(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.

Flags: needinfo?(emilio) → needinfo?(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.

Flags: needinfo?(bzbarsky)
Component: Console → DOM: CSS Object Model
Product: DevTools → Core

(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.

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

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

  1. 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.

(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

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

Mentor: emilio
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [lang=c++]

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!

Flags: needinfo?(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?

Assignee: nobody → pabloarubi
Flags: needinfo?(emilio)

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

Type: defect → enhancement

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

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!

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.

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

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13a4951a9f46
Improve exception message for CSSStyleSheet.prototype.deleteRule r=emilio
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

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