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

RESOLVED FIXED in Firefox 55

Status

enhancement
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on 4 bugs)

unspecified
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

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

Updated

2 years ago
Blocks: 1316187
Assignee

Comment 1

2 years ago
Posted patch Patch (obsolete) — Splinter Review
Assignee

Comment 2

2 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #8850550 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

2 years ago
Attachment #8850251 - Attachment is obsolete: true
Assignee

Comment 3

2 years ago
Posted patch Patch v3Splinter Review
Fixed the 3 eslint warnings of the previous try run.
Attachment #8850577 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

2 years ago
Attachment #8850550 - Attachment is obsolete: true
Attachment #8850550 - Flags: review?(gijskruitbosch+bugs)

Comment 4

2 years ago
(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.)
Assignee

Comment 5

2 years ago
(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

2 years ago
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.
Attachment #8850577 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Updated

2 years ago
Depends on: 1351070
Assignee

Updated

2 years ago
Depends on: 1351074
Assignee

Updated

2 years ago
Depends on: 1351078
Assignee

Updated

2 years ago
Depends on: 1351079
Assignee

Updated

2 years ago
Depends on: 1351089
Assignee

Updated

2 years ago
Depends on: 1351091
Assignee

Updated

2 years ago
Depends on: 1351093
Assignee

Updated

2 years ago
Depends on: 1351097
Assignee

Updated

2 years ago
Depends on: 1351099
Assignee

Updated

2 years ago
Depends on: 1351604
Assignee

Updated

2 years ago
Depends on: 1351637
Assignee

Updated

2 years ago
Depends on: 1351657
Assignee

Updated

2 years ago
Depends on: 1351658
Assignee

Updated

2 years ago
Depends on: 1351659
Assignee

Updated

2 years ago
Depends on: 1351669
Assignee

Updated

2 years ago
Depends on: 1351675
Assignee

Updated

2 years ago
Depends on: 1351682
Assignee

Updated

2 years ago
Depends on: 1351878
Assignee

Updated

2 years ago
Depends on: 1351892
Assignee

Comment 7

2 years ago
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).
Assignee

Updated

2 years ago
Depends on: 1351980

Comment 8

2 years ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e2edf43aad
verify that all the resource:// files we ship are actually referenced, r=Gijs.
Assignee

Comment 9

2 years ago
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c2e2edf43aad
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
No longer depends on: 1351079
Depends on: 543535
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.