Closed Bug 1398869 Opened 7 years ago Closed 7 years ago

stylo: off-by-1 css rule line number when using stylo

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

If I apply the patch from bug 1388497 and then visit
https://tromey.github.io/source-map-examples/css-warning/index.html,
the resulting .scss line numbers are off by one in the console.

servo is using 0-based lines here but gecko seems to use 1-based.
Summary: off-by-1 css rule line number when using stylo → stylo: off-by-1 css rule line number when using stylo
I suspect that https://github.com/servo/rust-cssparser/commit/4d42b6a68e5a4c8806fcbf7e67ec74187a9b8001 is involved, since we've dealt with this problem in the past before.
There are other options too unfortunately, like 0- -vs- 1- base differences for css and js in platform,
or bad source maps, etc.
Logging shows maybe a fishy source map, since the original requests look reasonably sane.


Using gecko:

################ MAP {"url":"https://tromey.github.io/source-map-examples/css-warning/ta.css","line":1,"column":218,"promise":{},"callbacks":[null]} => https://tromey.github.io/source-map-examples/css-warning/ta.scss 4 10
################ MAP {"url":"https://tromey.github.io/source-map-examples/css-warning/ta.css","line":1,"column":237,"promise":{},"callbacks":[null]} => https://tromey.github.io/source-map-examples/css-warning/ta.scss 5 9


Using stylo:

################ MAP {"url":"https://tromey.github.io/source-map-examples/css-warning/ta.css","line":1,"column":212,"promise":{},"callbacks":[null]} => https://tromey.github.io/source-map-examples/css-warning/ta.scss 3 17
################ MAP {"url":"https://tromey.github.io/source-map-examples/css-warning/ta.css","line":1,"column":231,"promise":{},"callbacks":[null]} => https://tromey.github.io/source-map-examples/css-warning/ta.scss 4 23
It's ok for the original column numbers to differ there, because it's up to the parser to choose
the location at which to report an error.  servo's choice seems a bit better in that it points to
the property name, whereas gecko seems to point at the end of the name or maybe the ":".
My source map dumper uses 0-based lines and columns and says:

   column 211
   source #0
   orig line 2
   orig column 17

   column 230
   source #0
   orig line 3
   orig column 23

So maybe nothing is incorrect here.

Though I am wondering why some code in servo calls get_location_with_offset while other code does not.
Ok, that is the problem - there are paths that don't do this offsetting.
Going to move this to a servo PR.
Priority: -- → P2
Of course there was fallout.
Comment on attachment 8907276 [details]
Bug 1398869 - regenerate console css message fixture;

https://reviewboard.mozilla.org/r/178954/#review184032
Attachment #8907276 - Flags: review?(wkocher) → review+
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/04720d1e6c02
regenerate console css message fixture; r=KWierso CLOSED TREE
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/23bf7009f21d
Fix up test_bug413958.html, too a=bustage CLOSED TREE
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f4b0f3105c72
Now re-fix the non-stylo tests a=bustage CLOSED TREE
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
(In reply to Mozilla Autoland from comment #16)

I manually landed it earlier to work around autoland being closed. This is just the original autoland request failing because it already landed.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.