browser_all_files_referenced.js should report files only referenced by unreferenced files

RESOLVED FIXED in Firefox 59

Status

enhancement
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on 5 bugs)

unspecified
mozilla59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

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

Updated

2 years ago
Depends on: 1356031
Assignee

Updated

2 years ago
Depends on: 1356032
Assignee

Updated

2 years ago
Depends on: 1356033
Assignee

Updated

2 years ago
Depends on: 1356036
Assignee

Updated

2 years ago
Depends on: 1356037
Assignee

Updated

2 years ago
Depends on: 1356043
Assignee

Updated

2 years ago
Depends on: 1356045
Assignee

Comment 1

2 years ago
Posted patch Patch (obsolete) — Splinter Review
Attachment #8857704 - Flags: review?(gijskruitbosch+bugs)

Comment 3

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

Comment 4

a year ago
Posted patch Patch v2Splinter Review
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.
Attachment #8939629 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

a year ago
Attachment #8857704 - Attachment is obsolete: true

Comment 5

a year ago
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.
Attachment #8939629 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Comment 6

a year ago
(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

a year ago
Pushed by florian@queze.net:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c8b8afedeb21
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 59 → mozilla59
You need to log in before you can comment on or make changes to this bug.