1349005 - verify that all the resource:// files we ship are actually referenced

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Florian Quèze [:florian]]

Similar to bug 1316187, but to extend the browser_all_files_referenced.js test landed there to also cover resource:// URLs.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch v2

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

Note: I fully intend to file bugs for all the places that currently have TODO comments, and include the bug numbers in the test file before landing.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch v3

Fixed the 3 eslint warnings of the previous try run.

Comment 4

[:Gijs (he/him)]

(apologies for the delay, I aim to get to this today (Sat 25) or Monday. FWIW, I am a bit concerned about this because of all the intermittents that have already appeared for this test. Feels like extending it is likely to exacerbate that, which would be unfortunate. Equally, not sure I have better ideas without looking into the patch in detail which I haven't found time to do.)

Comment 5

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #4)
> FWIW, I am a bit concerned about this because of all the intermittents that
> have already appeared for this test. Feels like extending it is likely to
> exacerbate that, which would be unfortunate. Equally, not sure I have better
> ideas without looking into the patch in detail which I haven't found time to
> do.)

The 4 intermittent failure bugs I see currently open for that test are one Linux asan timeout (the test completed in took 114165ms whereas the default timeout was 90s; seems we can just request a longer timeout), and 3 Linux32 opt oom crashes. I would just disable the test on linux32 (as we have coverage on linux64 and that should be enough).

Comment 6

[:Gijs (he/him)]

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

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

::: browser/base/content/test/static/browser_all_files_referenced.js
@@ +492,5 @@
> +      for (let res of gResourceMap) {
> +        if (fileUri.startsWith(res[1]))
> +          return fileUri.replace(res[1], "resource://" + res[0] + "/");
> +      }
> +      // Give up and return the original URL.

It seems to me like it'd be clearer if the resource conversion was a separate function and we didn't conflate it into a function named "convertToChromeURI". If that makes consumers overly complicated, perhaps this function just needs renaming.

@@ +519,2 @@
>        dump("Checking " + aURI + ": " + e + "\n");
>        Cu.reportError(e);

Sorry, remind me again why this isn't a test failure? Or is that an extant followup bug?

@@ +527,5 @@
>    // Find the 'c' character...
>    for (let index = 0;
>         (index = array.indexOf(prefix.charCodeAt(0), index)) != -1;
>         ++index) {
>      // Then ensure we actually have the whole chrome:// prefix.

This comment is out of date now.

@@ +551,5 @@
>        string += String.fromCharCode(array[index]);
>  
> +    // Only keep strings that look like real chrome or resource urls.
> +    if (/chrome:\/\/[a-zA-Z09 -]+\/(content|skin|locale)\//.test(string) ||
> +        /resource:\/\/gre.*\.[a-z]+/.test(string))

Seems like the resource check could just be a string.startsWith("resource://gre") check which would be cheaper.

@@ +622,1 @@
>          .filter(u => isDevtools == devtoolsPrefixes.some(prefix => u.startsWith(prefix)));

This would probably be faster as a for...of loop that just .push()d the right thing onto a chromeFiles array if/when applicable.

@@ +668,4 @@
>  
>    is(unreferencedFiles.length, 0, "there should be no unreferenced files");
>    for (let file of unreferencedFiles)
> +    ok(false, "unreferenced file: " + file);

Nit: I missed this last time, but please brace for loops.

Comment 7

[Florian Quèze [:florian]]

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

(In reply to :Gijs from comment #6)

> @@ +551,5 @@
> >        string += String.fromCharCode(array[index]);
> >  
> > +    // Only keep strings that look like real chrome or resource urls.
> > +    if (/chrome:\/\/[a-zA-Z09 -]+\/(content|skin|locale)\//.test(string) ||
> > +        /resource:\/\/gre.*\.[a-z]+/.test(string))
> 
> Seems like the resource check could just be a
> string.startsWith("resource://gre") check which would be cheaper.

That regexp is (poorly) checking that the string we found contains an extension (ie. a '.' followed by a few letters).

This test for the extension is useful, but unfortunately not done consistently throughout the file. I'll definitely need to revisit these checks for path validity when working on bug 1349010 to reduce the amount of garbage in the list of 'code references'.

> @@ +622,1 @@
> >          .filter(u => isDevtools == devtoolsPrefixes.some(prefix => u.startsWith(prefix)));
> 
> This would probably be faster as a for...of loop that just .push()d the
> right thing onto a chromeFiles array if/when applicable.

I reluctantly changed this to a for loop. The new code is longer, less readable, and I'm not convinced it's significantly faster (I would assume the amount of I/O and parsing done by this test far outweights any overhead map/filter functions may introduce).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e2edf43aad
verify that all the resource:// files we ship are actually referenced, r=Gijs.

Comment 9

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #6)

> @@ +519,2 @@
> >        dump("Checking " + aURI + ": " + e + "\n");
> >        Cu.reportError(e);
> 
> Sorry, remind me again why this isn't a test failure? Or is that an extant
> followup bug?

I changed it to a failure for my try push, and that failed on Windows on this URL, so I changed it to a todo() before pushing to inbound:

Failed to check if resource://testing-common/content-task.js%20line%2052%20%3E%20evalexists: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIChannel.open]" nsresult: "0x80004005 (NS_ERROR_FAILURE)"

This url comes from test files from the devtools/client/webconsole/new-console-output/test folder; it shouldn't be packaged, but that's bug 1351878...

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2e2edf43aad