971096 - Create a mochitest to verify that all CSS we ship is parsable in Gecko

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[:Gijs (he/him)]

As in summary. Bonus points for (as far as JS is concerned) checking for stupid mistakes like assigning to undeclared variables, using undefined variables, else after return, etc. (but some of those are harder because you need to know what variables are around and what gets created by DOM, etc.)

Comment 1

[:Gijs (he/him)]

This would have prevented bug 999080. I have a WIP patch for the CSS case.

Comment 2

[:Gijs (he/him)]

Alright, let's see the damage.

remote:   https://tbpl.mozilla.org/?tree=Try&rev=1278e410d17c

Comment 3

[:Gijs (he/him)]

Attachment: add test to parse all our CSS and check for obvious issues,

Mike, want to have a look at how you feel about this test? :-)

Comment 4

[Matthew N. [:MattN]]

Awesome, I was just thinking about something like this the other day too :)

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> Alright, let's see the damage.
> 
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=1278e410d17c

Well, that was dumb. :-(

Comment 6

[:Gijs (he/him)]

So I went and filed & patched bug 1004410 and checked that this time the patch isn't busted (at least, it seems to work locally...), and repushed:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=92798d0cf31f


I fully expect to find more extant CSS problems in the try push, which I intend to add to the filter so we can land this ASAP and then eliminate issues one-by-one as appropriate.

Comment 7

[:Gijs (he/him)]

Filed bug 1004418 for the JS issue.

Comment 8

[:Gijs (he/him)]

Leaks everywhere, but no more CSS issues. Good, I guess. I pushed another patch that doesn't leak for me locally (when building debug - I'd already fixed the leaks found in an opt build, but debug builds have a different leakfinder and so I had to do some more fiddling...).

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7f7abc547fb9

I also changed the whitelist mechanism to allow being more specific about what to ignore, so we can still detect "new" issues in files even if there are "known" issues in the file.

Requesting review in a bit.

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> Leaks everywhere, but no more CSS issues. Good, I guess. I pushed another
> patch that doesn't leak for me locally (when building debug - I'd already
> fixed the leaks found in an opt build, but debug builds have a different
> leakfinder and so I had to do some more fiddling...).
> 
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7f7abc547fb9

Err, I meant: remote:   https://tbpl.mozilla.org/?tree=Try&rev=9193c3bb0221

Comment 10

[:Gijs (he/him)]

Attachment: add test to parse all our CSS and check for obvious issues,

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8415840 [details] [diff] [review]
add test to parse all our CSS and check for obvious issues,

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

I think this is both fine and super useful, providing:

1) We add documentation about what things are whitelisted and why
2) We add documentation on how each of these functions operate
3) We ensure that this does not incur a huge cost time-wise in terms of running mochitest-bc

Overall though, I think this is a splendid idea.

::: browser/base/content/test/general/browser_parsable_css.js
@@ +1,1 @@
> +const kWhitelist = [

Missing license header

@@ +1,5 @@
> +const kWhitelist = [
> +  {sourceName: /cleopatra.*(tree|ui)\.css/i},
> +  {sourceName: /codemirror\.css/i},
> +  {sourceName: /web\/viewer\.css/i, errorMessage: /Unknown pseudo-class.*(fullscreen|selection)/i },
> +  {sourceName: /downloads\.css/i},

Can we put some explanations in here about why these things are whitelisted?

@@ +153,5 @@
> +  doc.head.innerHTML = '';
> +  doc = null;
> +  iframe = null;
> +});
> +

Unnecessary newlines at the end of the file.

Comment 12

[:Gijs (he/him)]

Thanks for the super-quick review! Landed with a bunch of docs in the file, and the downloads.css whitelist removed because I fixed the relevant bits in bug 1004431.

remote:   https://hg.mozilla.org/integration/fx-team/rev/706bc42060aa
remote:   https://hg.mozilla.org/integration/fx-team/rev/a38146bf6d18


I'll post to m.d.platform and fx-dev about this so people are aware.

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #12)
> I'll post to m.d.platform and fx-dev about this so people are aware.

https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/HfjjSyPi5MU
https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/HfjjSyPi5MU

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c0ab8f376e6
https://hg.mozilla.org/mozilla-central/rev/a38146bf6d18

Comment 15

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #11)
> 3) We ensure that this does not incur a huge cost time-wise in terms of
> running mochitest-bc

Our slowest bc1 runs these days are Win7 and 10.6 (debug), and checking on win7, I'm seeing runtimes of 42-44minutes both before and after landing this, so I don't think it's having much of an impact. :-)