1356029 - browser_all_files_referenced.js should report files only referenced by unreferenced files

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Florian Quèze [:florian]]

browser_all_files_referenced.js currently assume that if a file is referenced by any other file, then it's referenced. It should instead use a reference graph to report reference cycles.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

Comment 2

[Florian Quèze [:florian]]

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18d938bacd5f44ce0ce7eff3032800e85e94fd04

Comment 3

[:Gijs (out for now; he/him)]

Comment on attachment 8857704 [details] [diff] [review]
Patch

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

::: browser/base/content/test/static/browser_all_files_referenced.js
@@ +404,5 @@
> +  if (gReferencesFromCode.has(url)) {
> +    ref = gReferencesFromCode.get(url);
> +  if (ref === null)
> +    return;
> +  } else {

No else after return. Oh, except the indenting is wrong here, so it isn't actually an else after a return... please fix the indenting.

@@ +407,5 @@
> +    return;
> +  } else {
> +    // Assume that all 'features' bootstrap files are referenced.
> +    // The features folder is only in resource://app/ for non-packaged builds.
> +    if (/resource:\/\/app\/features\/[^/]+\/bootstrap\.js/.test(from)) {

But bootstrap.js files are treated as referenced anyway, so why is this necessary?

@@ +706,5 @@
> +      if (refs === null)
> +        return false;
> +      for (let ref of refs) {
> +        if (ref.endsWith("!/bootstrap.js") ||
> +            (file.startsWith("resource://gre/modules/") && hasDevtoolsPrefix(ref)) ||

So this says that... if a file is in the toolkit module directory (irrespective of what kind of file it is), and it's referenced from somewhere in devtools, we treat it as referenced? Why is this exception necessary?

@@ +709,5 @@
> +        if (ref.endsWith("!/bootstrap.js") ||
> +            (file.startsWith("resource://gre/modules/") && hasDevtoolsPrefix(ref)) ||
> +            (gReferencesFromCode.has(ref) &&
> +             (gReferencesFromCode.get(ref) === null ||
> +              typeof gReferencesFromCode.get(ref) == "string")))

So, this is very confusing if reading the patch top-to-bottom, because earlier you assume that the return value of the .get() call is always a Set or null.

It then seems that a little bit below here, you occasionally write a string to it instead (for whitelisted entries).

I would prefer if you added a comment about this here, and if you checked for the exact values you're writing (ie not a typeof, but a simple equals with "whitelist" and "whitelist-direct").

This if statement could also do with splitting up in order to reduce the brace nesting.

Comment 4

[Florian Quèze [:florian]]

Attachment: Patch v2

Updated patch. Unbitrotted and addressed comment 3 (better late than never ;-)).

This is green on try https://treeherder.mozilla.org/#/jobs?repo=try&revision=12b6c94684f27504d1f0c3acd2c48cd9abbdbd4b except for bug 1412343 failing TV on Windows 32.

(In reply to :Gijs from comment #3)

> @@ +407,5 @@
> > +    return;
> > +  } else {
> > +    // Assume that all 'features' bootstrap files are referenced.
> > +    // The features folder is only in resource://app/ for non-packaged builds.
> > +    if (/resource:\/\/app\/features\/[^/]+\/bootstrap\.js/.test(from)) {
> 
> But bootstrap.js files are treated as referenced anyway, so why is this
> necessary?

There's nothing marking all bootstrap.js files as referenced, the existing exception for bootstrap.js only silences the errors.

The exception here is for files referenced by bootstrap.js (ie. most of the add-on files). I rephrased the code comment to hopefully make this clearer.

> @@ +706,5 @@
> > +      if (refs === null)
> > +        return false;
> > +      for (let ref of refs) {
> > +        if (ref.endsWith("!/bootstrap.js") ||
> > +            (file.startsWith("resource://gre/modules/") && hasDevtoolsPrefix(ref)) ||
> 
> So this says that... if a file is in the toolkit module directory
> (irrespective of what kind of file it is), and it's referenced from
> somewhere in devtools, we treat it as referenced? Why is this exception
> necessary?

It's necessary because of bug 1351878, ie. we can't say reliably if a devtools resource file is referenced or not.

In this new version of the patch, I removed this broad exception and instead whitelisted the only 2 files that needed it (Promise.jsm and reflect.jsm).

> @@ +709,5 @@
> > +        if (ref.endsWith("!/bootstrap.js") ||
> > +            (file.startsWith("resource://gre/modules/") && hasDevtoolsPrefix(ref)) ||
> > +            (gReferencesFromCode.has(ref) &&
> > +             (gReferencesFromCode.get(ref) === null ||
> > +              typeof gReferencesFromCode.get(ref) == "string")))
> 
> So, this is very confusing [...]

Agreed. I rephrased the comment above the gReferencesFromCode declaration to mention the two string values. I split the if statement, added comments, removed the typeof and added equal checks for the 2 possible string values.

Comment 5

[:Gijs (out for now; he/him)]

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

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

I think this makes sense, though the reason for the info() statement referenced below is still a bit unclear to me.

::: browser/base/content/test/static/browser_all_files_referenced.js
@@ +719,5 @@
> +      ok(false, "unreferenced file: " + file);
> +    } else if (refs == "whitelist-direct") {
> +      continue;
> +    } else if (refs == "whitelist") {
> +      info("indirectly whitelisted file: " + file);

Still a bit confused... why do we get here (or to the whitelist-direct case) at all? Just before this loop, we check that length == 0, so on infra this is always true and these entries don't exist (and when it's not, stuff gets backed out).

Under what circumstances do these prints print stuff? And why is this an info(), even though in principle, this being here implies it would be possible for the only item in the `unreferencedFiles` array to be a whitelisted item?

I feel like I'm not being very clear myself here... to rephrase: I guess I expect that 'unreferencedFiles' at the end only contains items that are unreferenced and not whitelisted, and we both error because there is a non-0 amount of such items, and we display each of them as a separate mochitest error. The whitelist items will do the first, but not the second - they might be either silent or just info()'d but not ok(false, ...). That seems inconsistent in terms of whether the list ought to be empty and/or contain whitelisted items.

Comment 6

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #5)

> Under what circumstances do these prints print stuff?

Never, it's dead code, probably leftover from a previous wip version of this patch. Good catch! :-)

Reporting indirectly whitelisted files is useful, but already done with the info() call in the removeReferenced function, right before we set the value to "whitelisted" in the gReferencesFromCode map.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b8afedeb21
browser_all_files_referenced.js should report files only referenced by unreferenced files, r=Gijs.

Comment 8

[Andreea Pavel [:apavel]]

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