1591013 - Add a test / linter to make sure that DevTools server files are never reaching client files

Status: RESOLVED FIXED

(DevTools :: General, task, P3)

Description

[Julian Descottes [:jdescottes]]

We should be able to spot issues such as the one reported in Bug 1590932 with a simple test or linter.

I have several proposals to address this:

1/ create a test or a linting rule that will scan all the files in devtools/server and makes sure none of them requires "devtools/client" files.

2/ Another idea here would be to configure the Loader on the server to prevent loading any file in /client. I think this would also cover files from "devtools/shared" that are loaded on the server. devtools/shared is a bit messy and contains client only files such as the fronts... so we can't lint/test devtools/shared in the same way as devtools/server.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1591013 - Support lazyImporter in reject-some-requires

reject-some-requires only supports require/lazyRequireGetter at the moment
We still have a few call sites for lazyImporter, even though they probably should be
migrated to lazyRequireGetter IMO.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1591013 - Add an eslint rule to prevent loading client modules from devtools/server

This will help catch some issues related to server files importing client files.
Note that we still don't validate:

• devtools/shared: at the moment devtools/shared still contains client only files which
  naturally import other client files, so linting here is not an option before we clean
  this up
• require calls using variables eg require(MY_MODULE_URI). Maybe we should prevent this via
  another linting rule

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #0)

I have several proposals to address this:
...
2/ Another idea here would be to configure the Loader on the server to prevent loading any file in /client. I think this would also cover files from "devtools/shared" that are loaded on the server. devtools/shared is a bit messy and contains client only files such as the fronts... so we can't lint/test devtools/shared in the same way as devtools/server.

I don't think it will be easy to do that. I think we share the same loader between the client and the main process server. So we can't assume that a given Require will be only used for the server or for the client.

Another idea I was toying with is to look at stack for a "require" call, and if the stack contains a frame from devtools/server then throw.
This seems to work but I'd need to make sure it doesn't return false postives and that's also something we should not do in regular mode. Either only debug or only testing... I will try to put a patch up to see what others think.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d888aded0e70
Add an eslint rule to prevent loading client modules from devtools/server r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/5d53ab2f3152
Support lazyImporter in reject-some-requires r=Standard8,nchevobbe

Comment 5

[Raul Gurzau (:RaulG)]

Backed out 2 changesets (bug 1591013) for eslint failure at inspector/node.js on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/960977981144358e9ae12bcd78e6bee7389862ab

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=5d53ab2f3152c38db23fb341d309ab0094611ce0&selectedJob=273560714

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=273560714&repo=autoland&lineNumber=451

Log snippet:
[vcs 2019-10-29T22:28:18.931Z] 282947 files updated, 0 files merged, 0 files removed, 0 files unresolved
[vcs 2019-10-29T22:28:19.956Z] updated to 5d53ab2f3152c38db23fb341d309ab0094611ce0
[vcs 2019-10-29T22:28:19.960Z] PERFHERDER_DATA: {"framework": {"name": "vcs"}, "suites": [{"extraOptions": ["c3.2xlarge"], "lowerIsBetter": true, "name": "clone", "serverUrl": "hg.mozilla.org", "shouldAlert": false, "subtests": [], "value": 211.01004195213318}, {"extraOptions": ["c3.2xlarge"], "lowerIsBetter": true, "name": "update", "serverUrl": "hg.mozilla.org", "shouldAlert": false, "subtests": [], "value": 304.60786414146423}, {"extraOptions": ["c3.2xlarge"], "lowerIsBetter": true, "name": "overall", "serverUrl": "hg.mozilla.org", "shouldAlert": false, "subtests": [], "value": 516.3390369415283}, {"extraOptions": ["c3.2xlarge"], "lowerIsBetter": true, "name": "overall_clone", "serverUrl": "hg.mozilla.org", "shouldAlert": false, "subtests": [], "value": 516.3390369415283}, {"extraOptions": ["c3.2xlarge"], "lowerIsBetter": true, "name": "overall_clone_fullcheckout", "serverUrl": "hg.mozilla.org", "shouldAlert": false, "subtests": [], "value": 516.3390369415283}]}
[vcs 2019-10-29T22:28:20.253Z] TinderboxPrint:<a href=https://hg.mozilla.org/integration/autoland/rev/5d53ab2f3152c38db23fb341d309ab0094611ce0 title='Built from autoland revision 5d53ab2f3152c38db23fb341d309ab0094611ce0'>5d53ab2f3152c38db23fb341d309ab0094611ce0</a>
[setup 2019-10-29T22:28:20.253Z] GECKO_PATH is /builds/worker/checkouts/gecko
[task 2019-10-29T22:28:20.253Z] executing ['bash', '-cx', 'cp -r /build/node_modules_eslint node_modules && ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules && ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules && ./mach lint -l eslint -f treeherder --quiet -f json:/builds/worker/mozlint.json\n']in /builds/worker/checkouts/gecko
[task 2019-10-29T22:28:20.255Z] + cp -r /build/node_modules_eslint node_modules
[task 2019-10-29T22:28:20.684Z] + ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules
[task 2019-10-29T22:28:20.685Z] + ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules
[task 2019-10-29T22:28:20.686Z] + ./mach lint -l eslint -f treeherder --quiet -f json:/builds/worker/mozlint.json
[task 2019-10-29T22:28:21.950Z] Using base prefix '/usr'
[task 2019-10-29T22:28:21.950Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init_py3/bin/python3
[task 2019-10-29T22:28:21.950Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init_py3/bin/python
[task 2019-10-29T22:28:24.412Z] Installing setuptools, pip, wheel...done.
[task 2019-10-29T22:28:25.882Z] b"running build_ext\nbuilding 'psutil._psutil_linux' extension\ncreating build\ncreating build/temp.linux-x86_64-3.5\ncreating build/temp.linux-x86_64-3.5/psutil\nx86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python3.5m -I/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init_py3/include/python3.5m -c psutil/_psutil_common.c -o build/temp.linux-x86_64-3.5/psutil/_psutil_common.o\nx86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python3.5m -I/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init_py3/include/python3.5m -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-3.5/psutil/_psutil_posix.o\nx86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python3.5m -I/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init_py3/include/python3.5m -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-3.5/psutil/_psutil_linux.o\ncreating build/lib.linux-x86_64-3.5\ncreating build/lib.linux-x86_64-3.5/psutil\nx86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 build/temp.linux-x86_64-3.5/psutil/_psutil_common.o build/temp.linux-x86_64-3.5/psutil/_psutil_posix.o build/temp.linux-x86_64-3.5/psutil/_psutil_linux.o -o build/lib.linux-x86_64-3.5/psutil/_psutil_linux.cpython-35m-x86_64-linux-gnu.so\nbuilding 'psutil._psutil_posix' extension\nx86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python3.5m -I/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init_py3/include/python3.5m -c psutil/_psutil_common.c -o build/temp.linux-x86_64-3.5/psutil/_psutil_common.o\nx86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python3.5m -I/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init_py3/include/python3.5m -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-3.5/psutil/_psutil_posix.o\nx86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 build/temp.linux-x86_64-3.5/psutil/_psutil_common.o build/temp.linux-x86_64-3.5/psutil/_psutil_posix.o -o build/lib.linux-x86_64-3.5/psutil/_psutil_posix.cpython-35m-x86_64-linux-gnu.so\ncopying build/lib.linux-x86_64-3.5/psutil/_psutil_linux.cpython-35m-x86_64-linux-gnu.so -> psutil\ncopying build/lib.linux-x86_64-3.5/psutil/_psutil_posix.cpython-35m-x86_64-linux-gnu.so -> psutil\n"
[task 2019-10-29T22:28:25.882Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2019-10-29T22:44:04.532Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/server/actors/inspector/node.js:146:1 | require(resource://devtools/client/shared/DOMHelpers.jsm) is not allowed (mozilla/reject-some-requires)
[taskcluster 2019-10-29 22:44:05.205Z] === Task Finished ===
[taskcluster 2019-10-29 22:44:06.053Z] Unsuccessful task run with exit code: 1 completed in 1726.516 seconds

Comment 6

[Julian Descottes [:jdescottes]]

Sorry about that! I actually landed a violation of this new rule yesterday, I will fix it in Bug 1592513.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c19b26a1b39
Add an eslint rule to prevent loading client modules from devtools/server r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/33c3e07e19e7
Support lazyImporter in reject-some-requires r=Standard8,nchevobbe

Comment 8

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2c19b26a1b39
https://hg.mozilla.org/mozilla-central/rev/33c3e07e19e7

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c19b26a1b39
https://hg.mozilla.org/integration/mozilla-inbound/rev/33c3e07e19e7