1352669 - stylo: route CSS parsing errors to the developer tools console

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Cameron McCormack (:heycam)]

Like nsCSSParser and mozilla::css::ErrorReporter, we need to send CSS parsing errors to a page's developer tools console or the browser console.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

These mochitest failures are for this: https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/layout/style/test/stylo-failures.md#82-84

Comment 2

[Josh Matthews [:jdm]]

I'm giving this a shot. I'll unassign myself if I'm not making progress.

Comment 3

[Bobby Holley (:bholley)]

Awesome, thanks jdm!

Comment 4

[Josh Matthews [:jdm]]

Attachment: WIP

I made a bunch of progress during a 14 hour plane ride! CSS parsing errors are now appearing in the console for operations like "element.style = 'foopy'"; I'm going to look into the automated tests next, as well as sort out how to get the stylesheet URL from stylo into the error reporter.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:jdm, any news here?  Will you be able to continue working on this?

Comment 6

[Josh Matthews [:jdm]]

Yes. I have been actively working on it; https://github.com/servo/rust-cssparser/pull/155 and https://github.com/servo/servo/pull/16752 are the prerequisite API changes to the CSS parser. After those merge it will be much easier to work on the Stylo-specific parts.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

This also affects some other mochitests like dom/base/test/test_bug513194.html

Comment 8

[Josh Matthews [:jdm]]

Attachment: Hook up Stylo CSS parser to Gecko error reporter

MozReview-Commit-ID: 3r5Z6KiPgRM

Comment 9

[Josh Matthews [:jdm]]

Attachment: Address failing CSS parser error tests

MozReview-Commit-ID: KfcpLYLIIve

Comment 10

[Josh Matthews [:jdm]]

The first patch sets up the infrastructure, but it's unlikely that any tests pass. The second patch requires https://github.com/servo/rust-cssparser/pull/168 and passes test_bug413958.html, test_parser_diagnostics_unprintables.html, and test_bug513194.html.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8883410 [details] [diff] [review]
Hook up Stylo CSS parser to Gecko error reporter

Review of attachment 8883410 [details] [diff] [review]:
-----------------------------------------------------------------

(Just drive-by reviewing, overall looks good)

::: dom/base/FragmentOrElement.cpp
@@ +508,5 @@
> +  // We are not going to support xml:base for stylo, but we want to
> +  // ensure we unship that support before we enabling stylo.
> +  MOZ_ASSERT(nsLayoutUtils::StyleAttrWithXMLBaseDisabled());
> +  // This also ignores the case that SVG inside XBL binding.
> +  // But it is probably fine.

This can be easily solved calling bindingParent->GetCSSLoaderForStyleAttribute above, right?

Also, since this really can't return null, I believe according to the style guide this should be called CSSLoaderForStyleAttr.

::: servo/components/style/stylist.rs
@@ +602,5 @@
>                                           pseudo: &PseudoElement,
>                                           parent: Option<&Arc<ComputedValues>>,
>                                           cascade_flags: CascadeFlags,
> +                                         font_metrics: &FontMetricsProvider,
> +                                         reporter: &ParseErrorReporter)

Please document (here and in the other functions) that the error reporter is needed only for the custom properties code?

(Actually, I guess some of the code calling this is going to be relatively hot and we'll need to avoid the heap allocation of the error reporter using placement new or something like that, but I guess that can be done as a followup).

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8883411 [details] [diff] [review]
Address failing CSS parser error tests

Review of attachment 8883411 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/gecko/error_reporter.rs
@@ +54,5 @@
> +        }
> +    }
> +}
> +
> +fn escape_css_ident(ident: &str) -> String {

We have a serialize_identifier function. Is this doing the same, or something different? If so, it'd be nice to comment on the differences.

@@ +118,5 @@
> +    escaped
> +}
> +
> +// https://drafts.csswg.org/cssom/#serialize-a-string
> +fn escape_css_string(s: &str) -> String {

We definitely have code for string serialization in tree, look at String::to_css, and such.

@@ +137,5 @@
> +    }
> +    escaped
> +}
> +
> +fn token_to_str<'a>(t: Token<'a>) -> String {

And I think this really is reinventing token.to_css_string?

Comment 13

[Josh Matthews [:jdm]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Comment on attachment 8883411 [details] [diff] [review]
> Address failing CSS parser error tests
> 
> Review of attachment 8883411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/gecko/error_reporter.rs
> @@ +54,5 @@
> > +        }
> > +    }
> > +}
> > +
> > +fn escape_css_ident(ident: &str) -> String {
> 
> We have a serialize_identifier function. Is this doing the same, or
> something different? If so, it'd be nice to comment on the differences.

They are almost identical, but escape_css_string uses numeric escapes for characters between 0x7f and 0xa0. Comment added.

> @@ +118,5 @@
> > +    escaped
> > +}
> > +
> > +// https://drafts.csswg.org/cssom/#serialize-a-string
> > +fn escape_css_string(s: &str) -> String {
> 
> We definitely have code for string serialization in tree, look at
> String::to_css, and such.

That 0x7f and 0xa0 character range behaves differently, unfortunately. Comment added.

> @@ +137,5 @@
> > +    }
> > +    escaped
> > +}
> > +
> > +fn token_to_str<'a>(t: Token<'a>) -> String {
> 
> And I think this really is reinventing token.to_css_string?

This would be, except for the earlier behaviour differences and some very specific Gecko choices around bad URLs and strings.

Comment 14

[Josh Matthews [:jdm]]

Attachment: Hook up Stylo CSS parser to Gecko error reporter

MozReview-Commit-ID: 3r5Z6KiPgRM

Comment 15

[Josh Matthews [:jdm]]

Attachment: Address failing CSS parser error tests

MozReview-Commit-ID: KfcpLYLIIve

Comment 16

[Josh Matthews [:jdm]]

Interdiff works on both patches.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8883739 [details] [diff] [review]
Hook up Stylo CSS parser to Gecko error reporter

Review of attachment 8883739 [details] [diff] [review]:
-----------------------------------------------------------------

Lots of nits and similar, but looks good.

r=me with all the followups filed, or addressing them here directly :)

::: dom/base/nsIContent.h
@@ +25,5 @@
>  namespace mozilla {
>  class EventChainPreVisitor;
>  struct URLExtraData;
> +namespace css {
> +class Loader;

Does this really need to be here? no other change references css::Loader in this file.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +2852,4 @@
>  {
>    MOZ_ASSERT(aPresShell->StyleSet()->IsServo());
>  
> +  nsIDocument* document = aPresShell->GetDocument();

(Perhaps these functions should also assert aPresShell->GetDocument())

::: dom/svg/nsSVGElement.cpp
@@ +1212,4 @@
>                                     already_AddRefed<nsIURI> aBaseURI,
>                                     nsSVGElement* aElement,
>                                     StyleBackendType aBackend)
> +: mParser(aLoader), mLoader(aLoader), mDocURI(aDocURI), mBaseURI(aBaseURI),

nit: I think Gecko style would be something like:

  : mParser(aLoader)
  , mLoader(aLoader)
  , mDocURI(aDocURI)
  , //...

::: layout/style/ErrorReporter.cpp
@@ +149,5 @@
>  }
>  
> +ErrorReporter::ErrorReporter(const StyleSheet* aSheet,
> +                             const Loader* aLoader)
> +: mScanner(nullptr), mSheet(aSheet), mLoader(aLoader), mURI(nullptr),

Similarly, I think the colon should be indented two spaces (you can run ./mach clang-format and it'll auto-format your changes).

@@ +244,4 @@
>  }
>  
>  void
> +ErrorReporter::OutputError(uint32_t aLineNumber, uint32_t aLineOffset, nsIURI* aURI, const nsACString& aSource)

nit: I think this is more than 80 chars, so please wrap.

@@ +244,5 @@
>  }
>  
>  void
> +ErrorReporter::OutputError(uint32_t aLineNumber, uint32_t aLineOffset, nsIURI* aURI, const nsACString& aSource)
> +{

Should this assert that we either didn't have an URI, or that if we did, it's the same than what we were given?

Also re. aSource? It'd be nice to have docs about what this function expects.

@@ +246,5 @@
>  void
> +ErrorReporter::OutputError(uint32_t aLineNumber, uint32_t aLineOffset, nsIURI* aURI, const nsACString& aSource)
> +{
> +  mURI = aURI;
> +  mErrorLine = NS_ConvertUTF8toUTF16(aSource);

The equivalent of this assignment is fallible in Gecko. It seems it's due to bug 1171970... It'd be nice to avoid that pitfall. Can we do it, or get a followup filed for it?

@@ +315,5 @@
>  void
> +ErrorReporter::ReportUnexpectedUnescaped(const char *aMessage,
> +                                         const nsAutoString& aParam)
> +{
> +  if (!ShouldReportErrors()) return;

nit: braces for the if, and move the return into the next line.

::: layout/style/ServoBindings.cpp
@@ +2615,5 @@
> +ErrorReporter*
> +Gecko_CreateCSSErrorReporter(ServoStyleSheet* sheet,
> +                             Loader* loader)
> +{
> +  return new ErrorReporter(sheet, loader);

Can you file a bug to avoid the heap allocation here using placement new in the same fashion we do for the StyleChildrenIterator, and reference it from here?

::: servo/components/style/gecko/data.rs
@@ +126,5 @@
>      /// Map for effective counter style rules.
>      pub counter_styles: FnvHashMap<Atom, Arc<Locked<CounterStyleRule>>>,
> +
> +    /// Loader for this document.
> +    pub loader: *mut Loader,

We can get this pretty much every time through the stylist's device. Any reason why not doing that instead? Keeping the raw pointer around isn't particularly great, though I guess the StyleSet can't outlive the doc... Would be nice to at least mention it, but I'd rather do an accessor that calls:

Gecko_GetDocumentLoader(self.stylist.device().pres_context())

(We could also get it without FFI I guess).

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8883740 [details] [diff] [review]
Address failing CSS parser error tests

Review of attachment 8883740 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_parser_diagnostics_unprintables.html
@@ +45,5 @@
>  ];
>  
> +const isStylo = SpecialPowers.getBoolPref('layout.css.servo.enabled', false);
> +
> +// Stylo's CSS parser only reports the 'url(' token, not the actual bad URL.

Is there a bug on file for this? Maybe it's fine to just drop it, but perhaps worth opening a bug blocking bug 1365771 (same for the different columns reported in error messages).

::: servo/components/selectors/parser.rs
@@ +1105,4 @@
>              let position = input.position();
>              match input.next_including_whitespace() {
>                  Ok(Token::Delim('|')) => {
> +                    let prefix = from_cow_str(value.clone().into());

It'd be nice to avoid the unconditional clone here... I guess we could parameterice SelectorParseError with Parser, and make ExpectedNamespace(P::NamespacePrefix) instead, but I can see why that's somewhat annoying... Maybe file a followup in cssparser? I think it'd be nice, but I think selectors with explicit namespaces are somewhat uncommon, so maybe it's fine.

::: servo/components/style/gecko/error_reporter.rs
@@ +46,5 @@
> +    UnexpectedToken(Token<'a>),
> +}
> +
> +impl<'a> ErrorString<'a> {
> +    fn as_str(self) -> String {

I think as_str doesn't follow the conventions of this, into_string instead?

@@ +57,5 @@
> +}
> +
> +// This is identical to the behaviour of cssparser::serialize_identifier, except that
> +// it uses numerical escapes for a larger set of characters.
> +fn escape_css_ident(ident: &str) -> String {

I guess it's ok... Though I don't _love_ having all this mostly-duplicated code. File a bug to consider using the rust-cssparser escaping when we have stylo-only and reference it from here?

::: third_party/rust/cssparser/src/serializer.rs
@@ +129,4 @@
>              Token::SquareBracketBlock => dest.write_str("[")?,
>              Token::CurlyBracketBlock => dest.write_str("{")?,
>  
> +            Token::BadUrl(_) => dest.write_str("url(<bad url>)")?,

Well, we could potentially do better here I guess... Though this has already been reviewed upstream I suppose.

::: third_party/rust/cssparser/src/tokenizer.rs
@@ +287,4 @@
>          self.source_location(position)
>      }
>  
> +    pub fn current_source_line(&self) -> &'a str {

Add docs?

Comment 19

[Josh Matthews [:jdm]]

I'm going to ignore some of the style comments, since they're either about preexisting code or disagree with many other instances in the same file.

Comment 20

[Josh Matthews [:jdm]]

Attachment: Part 1- Hook up Stylo CSS parser to Gecko error reporter

Final patch stripped of servo changes.

Comment 21

[Josh Matthews [:jdm]]

Attachment: Part 2 - Address failing CSS parser error tests

Comment 22

[Josh Matthews [:jdm]]

Attachment: Part 2 - Address failing CSS parser error tests

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f220d8060c14
Hook up Stylo CSS parser to Gecko error reporter.
https://hg.mozilla.org/integration/autoland/rev/40ac529f9ea0
Address failing CSS parser error tests.

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da8c70e7f313
Backed out changeset 40ac529f9ea0 for build bustage CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/a13e199c5e09
Backed out changeset f220d8060c14 for build bustage CLOSED TREE a=bustage

Comment 25

[Josh Matthews [:jdm]]

Attachment: Part 1 - Hook up Stylo CSS parser to Gecko error reporter

Comment 26

[Josh Matthews [:jdm]]

Attachment: Part 2 - Address failing CSS parser error tests

Comment 27

[Josh Matthews [:jdm]]

Attachment: Part 1 - Hook up Stylo CSS parser to Gecko error reporter

Comment 28

[Josh Matthews [:jdm]]

https://github.com/servo/servo/pull/17655

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28e303771cc0
Hook up Stylo CSS parser to Gecko error reporter. r=emilio
https://hg.mozilla.org/integration/autoland/rev/81adbdebbfe3
Address failing CSS parser error tests. r=emilio

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28e303771cc0
https://hg.mozilla.org/mozilla-central/rev/81adbdebbfe3