1351878 - Lots of resource://devtools/ files reported as unreferenced by browser_all_files_referenced.js

Status: NEW

(DevTools :: General, defect)

Description

[Florian Quèze [:florian]]

In bug 1349005 I'm going to make browser_all_files_referenced.js check files at resource:// URLs, but I have to keep this disabled for devtools (it'll log using info() instead of reporting failures) because the list of unreferenced files it currently reports is too long for me to figure out what's false positives and what needs bugs filed.

Here's the list I see reported when I run the test locally:

resource://app/modules/devtools/gDevTools.jsm
resource://devtools/client/debugger/content/globalActions.js
resource://devtools/client/debugger/content/views/event-listeners-view.js
resource://devtools/client/debugger/content/views/sources-view.js
resource://devtools/client/debugger/new/pretty-print-worker.js
resource://devtools/client/debugger/new/source-map-worker.js
resource://devtools/client/jsonview/css/main.css
resource://devtools/client/jsonview/css/search.svg
resource://devtools/client/jsonview/json-viewer.js
resource://devtools/client/jsonview/lib/require.js
resource://devtools/client/jsonview/viewer-config.js
resource://devtools/client/performance/test/helpers/actions.js
resource://devtools/client/performance/test/helpers/dom-utils.js
resource://devtools/client/performance/test/helpers/input-utils.js
resource://devtools/client/performance/test/helpers/panel-utils.js
resource://devtools/client/performance/test/helpers/prefs.js
resource://devtools/client/performance/test/helpers/profiler-mm-utils.js
resource://devtools/client/performance/test/helpers/recording-utils.js
resource://devtools/client/performance/test/helpers/synth-utils.js
resource://devtools/client/performance/test/helpers/urls.js
resource://devtools/client/shared/Jsbeautify.jsm
resource://devtools/client/shared/shim/Services.js
resource://devtools/client/shared/source-map/worker.js
resource://devtools/client/shared/vendor/react-dom-server.js
resource://devtools/client/shared/vendor/react-virtualized.js
resource://devtools/client/shared/webgl-utils.js
resource://devtools/client/shared/widgets/BarGraphWidget.js
resource://devtools/client/sourceeditor/tern/comment.js
resource://devtools/client/sourceeditor/tern/condense.js
resource://devtools/client/sourceeditor/tern/signal.js
resource://devtools/client/themes/toolbars.css
resource://devtools/client/webconsole/new-console-output/actions/filters.js
resource://devtools/client/webconsole/new-console-output/actions/ui.js
resource://devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js
resource://devtools/client/webconsole/new-console-output/test/fixtures/stubs/consoleApi.js
resource://devtools/client/webconsole/new-console-output/test/fixtures/stubs/cssMessage.js
resource://devtools/client/webconsole/new-console-output/test/fixtures/stubs/evaluationResult.js
resource://devtools/client/webconsole/new-console-output/test/fixtures/stubs/index.js
resource://devtools/client/webconsole/new-console-output/test/fixtures/stubs/networkEvent.js
resource://devtools/client/webconsole/new-console-output/test/fixtures/stubs/pageError.js
resource://devtools/server/actors/highlighters.css
resource://devtools/server/protocol.js
resource://devtools/shared/acorn/acorn_loose.js
resource://devtools/shared/fronts/actor-registry.js
resource://devtools/shared/fronts/addons.js
resource://devtools/shared/fronts/director-manager.js
resource://devtools/shared/fronts/director-registry.js
resource://devtools/shared/fronts/eventlooplag.js
resource://devtools/shared/fronts/framerate.js
resource://devtools/shared/fronts/performance-entries.js
resource://devtools/shared/fronts/promises.js
resource://devtools/shared/gcli/source/lib/gcli/commands/mocks.js
resource://devtools/shared/gcli/source/lib/gcli/commands/preflist.js
resource://devtools/shared/gcli/source/lib/gcli/commands/test.js
resource://devtools/shared/gcli/source/lib/gcli/converters/html.js
resource://devtools/shared/gcli/source/lib/gcli/languages/command.html
resource://devtools/shared/gcli/source/lib/gcli/ui/menu.css
resource://devtools/shared/gcli/source/lib/gcli/ui/menu.html
resource://devtools/shared/gcli/source/lib/gcli/util/legacy.js
resource://devtools/shared/jsbeautify/lib/urlencode_unpacker.js
resource://devtools/shared/jsbeautify/src/beautify-tests.js
resource://devtools/shared/platform/content/clipboard.js
resource://devtools/shared/platform/content/stack.js
resource://gre/modules/devtools/Console.jsm
resource://gre/modules/devtools/Loader.jsm
resource://gre/modules/devtools/dbg-client.jsm
resource://gre/modules/devtools/dbg-server.jsm
resource://gre/modules/devtools/event-emitter.js
resource://gre/modules/devtools/shared/Loader.jsm

Could someone please review this list and figure out the next steps? (file bugs; fix the test to have less false positives; or add whitelist entries mentionning either where the file is used, or the number of the bug filed to remove it)


The test also reports a list of files referenced by code but that don't exist in our builds. This currently has a lot more false positives, and doesn't cause test failures even for the non-devtools parts of the tree, but it's likely worth someone having a look at it too to identify potential missing files:

resource://devtools/shared/fronts/styleeditor.js
resource://devtools/content/message.js
resource://devtools/chrome/message.js
resource://devtools/responsive/frame-loader.js
resource://devtools/shared/gcli/source/lib/gcli/test/mockCommands.js
resource://devtools/shared/gcli/source/lib/gcli/test/mockFileCommands.js
resource://devtools/shared/gcli/source/lib/gcli/test/mockSettings.js
resource://devtools/client/shared/vendor/utils.js
resource://devtools/client/shared/vendor/support.js
resource://devtools/client/shared/vendor/external.js
resource://devtools/client/shared/vendor/stream/DataWorker.js
resource://devtools/client/shared/vendor/stream/DataLengthProbe.js
resource://devtools/client/shared/vendor/stream/Crc32Probe.js
resource://devtools/client/shared/vendor/stream/GenericWorker.js
resource://devtools/client/shared/vendor/flate.js
resource://devtools/client/shared/utils.js
resource://devtools/client/shared/stream/GenericWorker.js
resource://devtools/client/shared/utf8.js
resource://devtools/client/shared/crc32.js
resource://devtools/client/shared/signature.js
resource://devtools/client/shared/compressions.js
resource://devtools/client/shared/vendor/ZipFileWorker.js
resource://devtools/client/shared/vendor/object.js
resource://devtools/client/shared/vendor/load.js
resource://devtools/client/shared/vendor/defaults.js
resource://devtools/client/shared/vendor/utf8.js
resource://devtools/client/shared/vendor/zipEntries.js
resource://devtools/client/shared/vendor/nodejsUtils.js
resource://devtools/client/shared/vendor/stream/StreamHelper.js
resource://devtools/client/shared/vendor/compressedObject.js
resource://devtools/client/shared/vendor/zipObject.js
resource://devtools/client/shared/vendor/generate.js
resource://devtools/client/shared/vendor/nodejs/NodejsStreamInputAdapter.js
resource://devtools/client/shared/vendor/DataReader.js
resource://devtools/client/shared/vendor/Uint8ArrayReader.js
resource://devtools/client/shared/vendor/ArrayReader.js
resource://devtools/client/shared/support.js
resource://devtools/client/shared/vendor/StringReader.js
resource://devtools/client/shared/vendor/NodeBufferReader.js
resource://devtools/client/shared/vendor/GenericWorker.js
resource://devtools/client/shared/vendor/ConvertWorker.js
resource://devtools/client/shared/base64.js
resource://devtools/client/shared/external.js
resource://devtools/client/shared/nodejs/NodejsStreamOutputAdapter.js
resource://devtools/client/shared/vendor/base64.js
resource://devtools/client/shared/vendor/reader/readerFor.js
resource://devtools/client/shared/vendor/signature.js
resource://devtools/client/shared/vendor/zipEntry.js
resource://devtools/client/shared/vendor/crc32.js
resource://devtools/client/shared/vendor/compressions.js
resource://devtools/client/shared/modules/web.immediate.js
resource://devtools/client/shared/modules/_core.js
resource://devtools/client/shared/vendor/_is-object.js
resource://devtools/client/shared/vendor/_a-function.js
resource://devtools/client/shared/vendor/_fails.js
resource://devtools/client/shared/vendor/_global.js
resource://devtools/client/shared/vendor/_core.js
resource://devtools/client/shared/vendor/_ctx.js
resource://devtools/client/shared/vendor/_hide.js
resource://devtools/client/shared/vendor/_object-dp.js
resource://devtools/client/shared/vendor/_property-desc.js
resource://devtools/client/shared/vendor/_descriptors.js
resource://devtools/client/shared/vendor/_dom-create.js
resource://devtools/client/shared/vendor/_an-object.js
resource://devtools/client/shared/vendor/_ie8-dom-define.js
resource://devtools/client/shared/vendor/_to-primitive.js
resource://devtools/client/shared/vendor/_invoke.js
resource://devtools/client/shared/vendor/_html.js
resource://devtools/client/shared/vendor/_cof.js
resource://devtools/client/shared/vendor/_export.js
resource://devtools/client/shared/vendor/_task.js
resource://devtools/client/shared/vendor/lib/utils/common.js
resource://devtools/client/shared/vendor/lib/deflate.js
resource://devtools/client/shared/vendor/lib/inflate.js
resource://devtools/client/shared/vendor/lib/zlib/constants.js
resource://devtools/client/shared/vendor/zlib/deflate.js
resource://devtools/client/shared/vendor/utils/common.js
resource://devtools/client/shared/vendor/utils/strings.js
resource://devtools/client/shared/vendor/zlib/messages.js
resource://devtools/client/shared/vendor/zlib/zstream.js
resource://devtools/client/shared/vendor/zlib/inflate.js
resource://devtools/client/shared/vendor/zlib/constants.js
resource://devtools/client/shared/vendor/zlib/gzheader.js
resource://devtools/client/shared/vendor/common.js
resource://devtools/client/shared/utils/common.js
resource://devtools/client/shared/vendor/trees.js
resource://devtools/client/shared/vendor/adler32.js
resource://devtools/client/shared/vendor/messages.js
resource://devtools/client/shared/vendor/inffast.js
resource://devtools/client/shared/vendor/inftrees.js
resource://devtools/shared/device/device.js
resource://devtools/sourceeditor/editor.js
resource://devtools/client/shared/components/reps.css
resource://devtools/server/actors/utils/lib/timeline.js
resource://devtools/server/actors/utils/formulas.js
resource://devtools/server/actors/utils/event.js
resource://devtools/client/shared/components/reps/rep.js
resource://devtools/shared/gcli/source/lib/gcli/testharness/examiner.js
resource://devtools/shared/gcli/source/lib/gcli/testharness/status.js
resource://devtools/shared/gcli/source/lib/gcli/test/helpers.js
resource://devtools/shared/gcli/source/lib/gcli/test/suite.js
resource://devtools/shared/gcli/source/lib/gcli/test/automators/requisition.js
resource://devtools/client/locales.js
resource://devtools/shared/locales.js
resource://devtools/client/debugger/new/client/shared/components/splitter/SplitBox.css
resource://devtools/client/debugger/shared/security/socket.js
resource://devtools/client/debugger/shared/security/auth.js
resource://devtools/client/server/main.js
resource://devtools/client/shared/widgets/ViewHelpers.jsm
resource://devtools/client/debugger/new/lib/themes/dark-theme.css
resource://devtools/client/debugger/new/lib/themes/light-theme.css
resource://devtools/client/debugger/new/lib/themes/firebug-theme.css
resource://devtools/client/debugger/new/rep.js

Comment 1

[Brian Grinstead [:bgrins]]

I think this is due to how we load modules - for example: `resource://devtools/client/debugger/content/globalActions.js` is required as `./content/globalActions` with our loader (see https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/debugger-view.js#31).

The loader is created here: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/Loader.jsm#32

Given that AIUI we are actively working to convert the codebase to a system addon it probably doesn't make sense to spend the time to get this test to work out the paths that the loader is generating.  Alex, any thoughts?

Comment 2

[Florian Quèze [:florian]]

(In reply to Brian Grinstead [:bgrins] from comment #1)

> Given that AIUI we are actively working to convert the codebase to a system
> addon it probably doesn't make sense to spend the time to get this test to
> work out the paths that the loader is generating.  Alex, any thoughts?

The test already tried to handle (or rather workaround) that loader's paths: http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/browser/base/content/test/static/browser_all_files_referenced.js#424

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> Given that AIUI we are actively working to convert the codebase to a system
> addon it probably doesn't make sense to spend the time to get this test to
> work out the paths that the loader is generating.  Alex, any thoughts?

Yes, this test is going to contain too many hardcoded workarounds to handle the various way devtools loads its resources.
It is getting even worse with the new bundles used for now only in the debugger.html.
It would be great to have such test, but I imagine it would be done really differently to be maintainable.
For example, I imagine we could spot such unused modules via code coverage.

Would you be fine dropping these checks for devtools?

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)
>
> Would you be fine dropping these checks for devtools?

Comment 5

[Florian Quèze [:florian]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)

> It would be great to have such test, but I imagine it would be done really
> differently to be maintainable.
> For example, I imagine we could spot such unused modules via code coverage.
> 
> Would you be fine dropping these checks for devtools?

Can we ensure we have coverage in some way before dropping the existing checks?

Comment 6

[Alexandre Poirot [:ochameau]]

Actually the same question applies to browser_parsable_css.js, which has the same level of maintanability.

If we want to keep it, we will have to fork and copy them to the github repo.

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> (In reply to Alexandre Poirot [:ochameau] from comment #3)
> 
> > It would be great to have such test, but I imagine it would be done really
> > differently to be maintainable.
> > For example, I imagine we could spot such unused modules via code coverage.
> > 
> > Would you be fine dropping these checks for devtools?
> 
> Can we ensure we have coverage in some way before dropping the existing
> checks?

Unfortunately, no. I could manually check that once we switched to the addon model.
But I don't have the cycle to craft any such tooling. Maybe Brian have some plan and/or resources.
I'm stressing this out as we will have to get rid of this in order to move to github:
http://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser.ini#99-102

Comment 8

[Brian Grinstead [:bgrins]]

I'd vote to at least ignore the resource URIs for devtools (wontfix this bug). The plan right now is to have devtools out of m-c and into a system addon by June.  I think it'd make sense for that test to live with the code (either a forked copy of these tests, or some other test running through CI integrated with the github repo).  If that's the case, then once devtools is out of m-c we wouldn't be running either of those tests on the devtools subsuite, so any special casing for devtools could be removed from the tests.