Closed Bug 1378861 Opened 7 years ago Closed 7 years ago

stylo: Error columns reported by Stylo differ from Gecko's parser

Categories

(Core :: CSS Parsing and Computation, defect, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 - wontfix
firefox58 --- fixed

People

(Reporter: jdm, Assigned: SimonSapin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

CSS Messages Toggle Button
661.16 KB, image/png
Details
Bug 1378861 - Update test expectations for Servo PR #18808
59 bytes, text/x-review-board-request
Details 
Bug 1352669 includes some test changes to accommodate for different column numbers reported in error messages. As far as I can tell, this is because one engine reports the start of the error range while the other reports the end, but I haven't tried to verify this properly.
Priority: -- → P3
So one thing I'm seeing is on something like this:

  display: invalid;

Gecko will report the column of the "i" in "invalid" while stylo will report the column of the "d" in "display".

In this case it's not a big problem, but for more complicated values, pointing to the right place in the value, instead of to the property name part of the declaration, is much better.  Even as it was, I was confused about what the problem was with the "display" part here.  :(
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
This bug was not about off-by-one errors and is still relevant.
Ok, sorry about that.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Yeah, this bug is about the fact that for errors in property values stylo's column number is useless: it points to the start of the property _name_, not the actual error.
So I've been doing more of my work with stylo enabled the last few days, and this bug is a real PITA for sorting out CSS errors.  What are the chances of us fixing it before we ship 57?
Flags: needinfo?(josh)
Simon, since Josh is out, do you have time to take a look?
Flags: needinfo?(josh) → needinfo?(simon.sapin)
Priority: P3 → P2
I guess this was not caught by Bobby's radar sine the title was not stylo: prefixed.
Summary: Error columns reported by Stylo differ from Gecko's parser → sylo: Error columns reported by Stylo differ from Gecko's parser
Summary: sylo: Error columns reported by Stylo differ from Gecko's parser → stylo: Error columns reported by Stylo differ from Gecko's parser
Thanks Hiro!

[Tracking Requested - why for this release]:

Stylo P2.
tracking-firefox57: --- → ?
The quick fix we discussed of using the "current location" of the parser at the time of reporting didn’t turn out much better: for property declarations, that turns out to be the semi-colon or whatever else marks the end of a declaration, because after an error while parsing a "normal" value we keep looking for var() function.

I’ll work on recording the location before looking for var() functions.
Assignee: nobody → simon.sapin
Simon tells me this only affects error messages in the console that aren't enabled by default, so this doesn't seem worth tracking.
tracking-firefox57: ? → -
As far as I can tell, this only affects console messages that are only visible when the devtools.webconsole.filter.css pref is set to true. It defaults to false, and I couldn’t find any UI (other than about:config) that changes it.
(In reply to Simon Sapin (:SimonSapin) from comment #12)
> As far as I can tell, this only affects console messages that are only
> visible when the devtools.webconsole.filter.css pref is set to true. It
> defaults to false, and I couldn’t find any UI (other than about:config) that
> changes it.

You are correct that it defaults to false / messages hidden, but there is a UI control for this.  There is a "CSS" button in the console.  Clicking this will show the messages and set the pref to true.
Ah! It turns out the funnel icon next to the "Filter output" text field (which is a text filter) is not an illustration for "Filter output", it is a separate "Toggle filter bar" button. The filter bar is hidden by default. So there is UI, but it’s not easy to find. I didn’t find it by grepping for "devtools.webconsole.filter.css" in the tree.

I maintain that few people will ever see these CSS parsing error messages.
Boris, opinions about whether we should track/uplift?
Flags: needinfo?(bzbarsky)
From my point of view:

1) This is one of the differentiators for our devtools from the Chrome ones (which has no option to show CSS errors at all).

2) I use CSS error messages this all the time when debuggins sites while working on layout bugs.

I can't speak to how many web developers actually turn this on, since devtools has gone to such lengths to hide it (it used to be much more reachable in the old devtools ui), nor whether it affects other Gecko developers...  

For my personal layout developer productivity, it would be best to have this fixed, but obviously I usually debug on nightly, not release, so uplifting or not doesn't affect me personally.  As far as web developers go, I can't tell you how many of them end up enabling these, and what they would think of the output with this bug present...

I know that's not very helpful.
Flags: needinfo?(bzbarsky)
It would be nice to have usage metrics to add some objective data to the discussion, but we don't appear to capture that today for this part of DevTools. 

I filed bug 1402459 to record console sessions that have CSS messages enabled, so hopefully we can have it for the future, at least.
Seems like we should try to fix this regardless, assuming we can do so without performance penalty. If the fix turns out to be low-risk, we can uplift it.
Status update:

    error: aborting due to 267 previous errors
    error: Could not compile `style`.

I’ve rewritten cssparser’s error types so that they include a SourceLocation field, and I’m now porting the style system to this new API. It’s not gonna be a small patch. It also makes many return types larger by 8 bytes (two u32’s for line and column numbers), I’ll measure perf impact once I have something that compiles.
(In reply to Simon Sapin (:SimonSapin) from comment #19)
> it’s not gonna be a small patch.

Given this, I think it's unlikely we'd uplift this at this point. Moving to the bucket of 58+ work.
Priority: P2 → P4
I have something at https://github.com/servo/servo/pull/18585 that compiles and does improve the locations in most cases. Unfortunately…

> It also makes many return types larger by 8 bytes (two u32’s for line and column numbers)

Before:

  33.801 ±  0.195 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
  29.147 ±  0.039 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 262.219 ±  0.017 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 262.949 ±  0.006 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench
  99.183 ±  0.075 ms    Stylo.Servo_DeclarationBlock_GetPropertyById_Bench

After:

  38.519 ±  0.391 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
  28.816 ±  0.039 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 312.534 ±  0.412 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 313.117 ±  0.046 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench
 119.972 ±  0.022 ms    Stylo.Servo_DeclarationBlock_GetPropertyById_Bench

… this makes parsing 10~20% slower.

So we might not want to land this without finding some other way to shrink the result types.

Before going with this API I tried merging the BasicParseError and ParseError enums (in this PR renamed to BasicParseErrorKind and ParseErrorKind) but couldn’t implement the necessary conversion with the From trait to make this ergonomic because of a conflicting impl in the standard library.
Flags: needinfo?(simon.sapin)
Never mind, my "after" tree did not have --enable-release…

After:

  34.625 ±  0.180 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
  28.845 ±  0.057 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 272.367 ±  0.014 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 273.149 ±  0.012 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench
  98.441 ±  0.060 ms    Stylo.Servo_DeclarationBlock_GetPropertyById_Bench

So the slow down is 2~4%.
And in particular, the biggest regression is in the property setters; presumably the same code as inline style setters would invoke?
Indeed, that’s the same function that ends up called by JS code like `some_element.style.top = "10px";`.

I’m now trying another approach for compacting the error types, we’ll see if it goes anywhere this time.
I tried to recover from that 2~4% regression by shrinking the error types by moving things around in nested enums. style_traits::ParseError is 96 bytes in current master/central (the "before" case). In the code measured in comment 22 it was 104 bytes. In my branch with further changes it is now 72 bytes. However this is no measurable impact on the parsing microbenchmarks:

  34.397 ±  0.220 ms    Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench
  29.585 ±  0.083 ms    Stylo.Gecko_nsCSSParser_ParseSheet_Bench
 272.322 ±  0.020 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_Bench
 273.085 ±  0.005 ms    Stylo.Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench
  97.920 ±  0.008 ms    Stylo.Servo_DeclarationBlock_GetPropertyById_Bench

(GetPropertyById seems to be ~1% faster than central although it doesn’t involve any code that has been modified. I’m attributing this to "spooky LTO action at a distance".)

So it looks like the parsing perf regression is not related to the size of error types, but (I’m totally guessing) might be due to computing a column number (which is `((self.position: usize) - (self.current_line_start_position: usize) + 1) as u32`) and moving SourceLocation (which is `(u32, u32)`) around more often. But this is what this bug is all about.

I’m tempted to accept the regression and land this, but I’m biased having spent time making this branch work :) Boris, what do you think?
Flags: needinfo?(bzbarsky)
That doesn't seem unreasonable, though a followup to look into why there's still the 3% regression might be worth it.  That seems somewhat high to be at first glance, but I may just be miscalibrating how cheap a token is.  I'm assuming that we basically have one extra SourceLocation move per token...
Flags: needinfo?(bzbarsky)
https://github.com/servo/servo/pull/18585 is in the queue, attachment 8916941 [details] should be landed at the same time.
Comment on attachment 8916941 [details]
Bug 1378861 - Update test expectations for Servo PR #18808

https://reviewboard.mozilla.org/r/187976/#review193126

::: devtools/client/webconsole/new-console-output/test/fixtures/stubs/cssMessage.js:94
(Diff revision 1)
>      "errorMessage": "Error in parsing value for ‘padding-top’.  Declaration dropped.",
>      "errorMessageName": "",
>      "sourceName": "http://example.com/browser/devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/test-css-message.html",
>      "lineText": "",
>      "lineNumber": 3,
> -    "columnNumber": 3,
> +    "columnNumber": 16,

Are these stylo-only? Do they need conditional compilation?
Attachment #8916941 - Flags: review+
Comment on attachment 8916941 [details]
Bug 1378861 - Update test expectations for Servo PR #18808

https://reviewboard.mozilla.org/r/187976/#review193126

> Are these stylo-only? Do they need conditional compilation?

The test that checks these is Stylo only currently[1] (as a way avoid having two versions), so this patch seems fine as-is.

[1]: http://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser.ini#14
Comment on attachment 8916941 [details]
Bug 1378861 - Update test expectations for Servo PR #18808

https://reviewboard.mozilla.org/r/187976/#review193126

> The test that checks these is Stylo only currently[1] (as a way avoid having two versions), so this patch seems fine as-is.
> 
> [1]: http://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser.ini#14

This test is listed as:

    [browser_webconsole_check_stubs_css_message.js]
    skip-if = !stylo # Stubs updated for Stylo, won't match old Gecko style system
Pushed by simon.sapin@exyr.org:
https://hg.mozilla.org/integration/autoland/rev/7af19bab721c
Update test expectations for Servo PR #18808 r=emilio
https://hg.mozilla.org/mozilla-central/rev/7af19bab721c
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assuming this is 57:wontfix based on the P4/tracking- status.
status-firefox57: affected → wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: