1302570 - verify that all the files referenced from CSS we ship actually exist

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Florian Quèze [:florian]]

Bug 1301851 is something that IMHO should have been caught by automation. I'm filing this bug to investigate how difficult it would be to extend the browser_parsable_css.js mochitest to catch this kind of issues.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch v1

Comment 2

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecac296914f5

Comment 3

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=901497c4a860

Comment 4

[Florian Quèze [:florian]]

Here are the results from the try run:

Linux

missing chrome://browser/skin/toolbarbutton-dropdown-arrow.png referenced from chrome://browser/skin/customizableui/panelUI.css

missing chrome://global/skin/console/console.png referenced from chrome://browser/skin/browser.css

missing chrome://mozapps/skin/extensions/alerticon-info-negative.png referenced from chrome://browser/skin/browser.css

missing chrome://browser/skin/notification-pluginNormal.png referenced from chrome://browser/skin/browser.css

missing chrome://browser/skin/notification-pluginBlocked.png referenced from chrome://browser/skin/browser.css

missing chrome://browser/skin/customizableui/customize-titleBar-toggle.png referenced from chrome://browser/skin/browser.css

missing chrome://browser/skin/customizableui/[e-mail] referenced from chrome://browser/skin/browser.css

missing chrome://pocket/skin/toolbar-lunaSilver.png referenced from jar:file:///builds/slave/test/build/application/firefox/browser/features/[e-mail]!/chrome/skin/windows/pocket.css



Mac

missing chrome://browser/skin/toolbarbutton-dropdown-arrow.png referenced from chrome://browser/skin/customizableui/panelUI.css

missing chrome://mozapps/skin/extensions/alerticon-info-negative.png referenced from chrome://browser/skin/browser.css

missing chrome://browser/skin/notification-pluginNormal.png referenced from chrome://browser/skin/browser.css

missing chrome://browser/skin/notification-pluginBlocked.png referenced from chrome://browser/skin/browser.css

missing chrome://pocket/skin/toolbar-lunaSilver.png referenced from jar:file:///builds/slave/test/build/application/Nightly.app/Contents/Resources/browser/features/[e-mail]!/chrome/skin/windows/pocket.css



Windows

missing chrome://mozapps/skin/extensions/alerticon-info-negative.png referenced from chrome://browser/skin/browser.css

missing chrome://browser/skin/notification-pluginNormal.png referenced from chrome://browser/skin/browser.css

missing chrome://browser/skin/notification-pluginBlocked.png referenced from chrome://browser/skin/browser.css

missing chrome://pocket/skin/toolbar-lunaSilver.png referenced from chrome://pocket/skin/pocket.css

Comment 5

[:Gijs (he/him)]

Comment on attachment 8790997 [details] [diff] [review]
Patch v1

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

Definite f+ on doing this. I think there's enough useful feedback below to not have it be r+ yet.

We should probably disable this test on debug builds because it'll be too slow (or just requestLongerTimeout it to death and back).

::: browser/base/content/test/general/browser_parsable_css.js
@@ +162,5 @@
>    }
>    return false;
>  }
>  
> +let images = new Map();

This feels pretty generic... maybe "imageURIsToReferencesMap" or something else more descriptive?

@@ +176,5 @@
> +    if (!(rule instanceof CSSStyleRule))
> +      continue;
> +
> +    // Extract urls from the css text.
> +    let urls = rule.cssText.match(/url\("[^"]*"\)/g);

We often use url() without the quotes, or with single quotes, as all 3 are legal. Consider using:

let urls = rule.cssText.match(/url\((?:[^\)]*|"[^"]*"|'[^']*')\)/g);
// Remove 'url()' syntax and any potential containing quotes:
urls = urls.map(url => url.replace(/url\(["']?(.*)["']?\)/, "$1"));

(or do the latter in the loop if you prefer, I guess - it doesn't matter an awful lot, but the regexp does need updating slightly).

@@ +186,5 @@
> +      url = url.replace(/url\("(.*)"\)/, "$1");
> +
> +      // Make the url absolute and remove the ref.
> +      let baseURI = Services.io.newURI(rule.parentStyleSheet.href, null, null);
> +      url = Services.io.newURI(url, null, baseURI).specIgnoringRef;

So we won't catch elements that aren't in SVG files. Because we're switching more and more to SVG files, and because you're already writing stream-reading code, any chance of dealing with that and/or filing a followup for that?

@@ +193,5 @@
> +        continue;
> +
> +      // Store the image url along with the css file referencing it.
> +      if (!images.has(url)) {
> +        images.set(url, [baseURI.spec]);

Nit: images.set(url, new Set([baseURI.spec]))

@@ +196,5 @@
> +      if (!images.has(url)) {
> +        images.set(url, [baseURI.spec]);
> +      } else {
> +        let cssFilesArray = images.get(url);
> +        if (!cssFilesArray.includes(baseURI.spec))

.has

@@ +197,5 @@
> +        images.set(url, [baseURI.spec]);
> +      } else {
> +        let cssFilesArray = images.get(url);
> +        if (!cssFilesArray.includes(baseURI.spec))
> +          cssFilesArray.push(baseURI.spec);

.add

@@ +210,5 @@
> +    let channel = Services.io.newChannel2(aURI, null, null, null,
> +                                          Services.scriptSecurityManager.getSystemPrincipal(),
> +                                          null,
> +                                          Ci.nsILoadInfo.SEC_NORMAL,
> +                                          Ci.nsIContentPolicy.TYPE_OTHER);

Nit: NetUtil.newChannel() lets you pass less gobbledygook.

Some of the other code for this stuff uses XHRs. Was there a reason not to use those? They have the benefit of being async and not doing main thread IO.

@@ +212,5 @@
> +                                          null,
> +                                          Ci.nsILoadInfo.SEC_NORMAL,
> +                                          Ci.nsIContentPolicy.TYPE_OTHER);
> +    let stream = channel.open();
> +    let sstream = Components.classes["@mozilla.org/scriptableinputstream;1"]

You're using Ci, so might as well use Cc.

@@ +215,5 @@
> +    let stream = channel.open();
> +    let sstream = Components.classes["@mozilla.org/scriptableinputstream;1"]
> +                            .createInstance(Ci.nsIScriptableInputStream);
> +    sstream.init(stream);
> +    let available = sstream.available();

Nitty nit: declare 'available' outside the try block, assign 0 there, reassign here without declaring, return available > 0 at the end of the function instead of having two return statements.

@@ +298,5 @@
> +  // Check if all the files referenced from CSS actually exist.
> +  for (let [image, references] of images) {
> +    if (chromeFileExists(image))
> +      continue;
> +    ok(false, "missing " + image + " referenced from " + references);

references is an array, and with my changes it'll be a set (which serializes to [object Set]). Can we make the list of output slightly more readable with something like:

for (let ref of references) {
  ok(false, ... + ref);
}

so multiple URIs are more easily readable?

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> Nit: images.set(url, new Set([baseURI.spec]))
> 
> @@ +196,5 @@
> > +      if (!images.has(url)) {
> > +        images.set(url, [baseURI.spec]);
> > +      } else {
> > +        let cssFilesArray = images.get(url);
> > +        if (!cssFilesArray.includes(baseURI.spec))
> 
> .has
> 
> @@ +197,5 @@
> > +        images.set(url, [baseURI.spec]);
> > +      } else {
> > +        let cssFilesArray = images.get(url);
> > +        if (!cssFilesArray.includes(baseURI.spec))
> > +          cssFilesArray.push(baseURI.spec);
> 
> .add

my brain is only half functioning, clearly. The point of using a Set is that you don't worry about this:

...
} else {
  images.get(url).add(baseURI.spec);
}

and done.

Comment 7

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> Here are the results from the try run:
> 
> Linux
> 
> missing chrome://browser/skin/toolbarbutton-dropdown-arrow.png referenced
> from chrome://browser/skin/customizableui/panelUI.css

bug 1025967

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch v2

(In reply to :Gijs Kruitbosch from comment #5)

> We should probably disable this test on debug builds because it'll be too
> slow (or just requestLongerTimeout it to death and back).

I haven't done this yet as I'm curious to see how this will perform on try (it worked fine on debug in my previous run; and it surprised me a bit). But I see no use for this test to run on both opt and debug at every push, so disabling on debug makes sense to me.

> > +      // Make the url absolute and remove the ref.
> > +      let baseURI = Services.io.newURI(rule.parentStyleSheet.href, null, null);
> > +      url = Services.io.newURI(url, null, baseURI).specIgnoringRef;
>
> So we won't catch elements that aren't in SVG files. Because we're switching
> more and more to SVG files, and because you're already writing
> stream-reading code, any chance of dealing with that and/or filing a
> followup for that?

I can file a follow-up. It would require opening and parsing all the SVG files, so the test may end up even slower; may be a good idea to do this in a separate test file.

> Some of the other code for this stuff uses XHRs. Was there a reason not to
> use those? They have the benefit of being async and not doing main thread IO.

There are reasons why I didn't use the existing XHR stuff, but none of them is very solid:
- I wasn't sure how the existing function using XHR would react when looking at the responseText for files that are binary.
- the existing XHR stuff for the manifest parsing already causes plenty of noise in my terminal, as for some reason it attempts to parse the manifest files as XML and fails.
- I have some hope (but haven't verified) that the code I used which only checks the available length of the stream without reading it will just open/stat the files without actually reading them, and will use significantly less memory.
- tbh, I copy/pasted most of this function from some existing older code (hence the Components.classes ;-)).

I pondered whether this code being sync would be an issue, but for now it seems to work well enough. I think if I wanted to parse SVG files (and so need to look at the file contents), I would switch to XHR.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8791250 [details] [diff] [review]
Patch v2

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

r=me - but don't we still need the list of exceptions?

::: browser/base/content/test/general/browser_parsable_css.js
@@ +167,5 @@
> +
> +function processCSSRules(sheet) {
> +  let rules = sheet.cssRules;
> +  for (let i = 0; i < rules.length; ++i) {
> +    let rule = rules[i];

Nit:

for (let rule of sheet.cssRules) {
}

seems like it works.

Comment 10

[Florian Quèze [:florian]]

Attachment: Patch v3

Comment 11

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=167c5e0b04a6

Comment 12

[:Gijs (he/him)]

Comment on attachment 8791280 [details] [diff] [review]
Patch v3

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

rs=me

::: browser/base/content/test/general/browser_parsable_css.js
@@ +33,5 @@
>     isFromDevTools: true},
>  ];
>  
> +// Platform can be "linux", "macosx" or "win". If omitted, the exception applies to all platforms.
> +let fileWhitelist = [

Personally I try to avoid "whitelist" and "blacklist" terminology these days, so I would go with something like "allowedImageReferences", also to distinguish it from the other list we already have (which, today, I would also not call "whitelist" but that's a problem for another bug).

Up to you.

Comment 13

[Florian Quèze [:florian]]

Attachment: Patch v4

Comment 14

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e90d45b1f4b

Comment 15

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/4832e0d25e64bb4b61ef0815ae13412a9e876dd3
Bug 1302570 - verify that all the files referenced from CSS we ship actually exist, r=Gijs.

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/4832e0d25e64