1655110 - Provide isPrivateName method to replace JSID manipulations.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

In SpiderMonkey there's a lot of checks for private names that look like this:

JSID_IS_SYMBOL(id) && JSID_TO_SYMBOL(id)->isPrivateName();

It would be nice if these were collapsed into a method on JS::PropertyKey, such that you could instead write

id->isPrivateName()

Because id is often a HandleId (which is under the covers a JS::Handle<<JS::PropertyKey> we actually will also want a method on class WrappedPtrOperations<JS::PropertyKey, Wrapper> to forward that call for us.

Prerequisites

Before getting started, you'll want to

• Have a checkout of the Firefox source code
• Make sure you can build SpiderMonkey
• Understand the basics of using Mercurial
• Understand how to submit a patch
• Read this walkthrough about how development works in Firefox.

Success Criteria

• Checks of the above form instead can directly call id->isPrivateName().
• After the changes are made, the engine compiles, and passes the shell tests (run mach jit-test and mach jstests).

Questions

If you have questions, feel free to visit us on in the SpiderMonkey room on Matrix, or post questions here.

Tips and Tricks:

• SearchFox indexes the Firefox codebase, and allows you to easily find functions, definitions, and more. It's a powerful tool!
• If you're looking at some code in SpiderMonkey, and are trying to figure out how it gets triggered, one of the easiest ways to do this is to inject a crash by putting MOZ_RELEASE_ASSERT(false); into the code, then running the tests. They will fails when they trigger that line.

Comment 1

[makarmani]

Hi,
This is my first time here and i’d like to work on this bug.
I successfully checked out mozilla source code on windows 10 using mercurial.
I built SpiderMonkey as DEBUG and OPTIMIZED.

When Testing SpiderMonkey:
${BUILDDIR}/dist/bin/js Y.js
and
./jit-test/jit_test.py ${BUILDDIR}/dist/bin/js
were ran successfully.

But when running this one
./tests/jstests.py ${BUILDDIR}/dist/bin/js
i get:
$ python3 jstests.py ../build_OPT.OBJ/dist/bin/js.exe Traceback (most recent call last):
File "jstests.py", line 678, in <module>
sys.exit(main())
File "jstests.py", line 567, in main
test_count, test_gen = load_tests(options, requested_paths, excluded_paths)
File "jstests.py", line 488, in load_tests
excluded_paths)
File "jstests.py", line 367, in load_wpt_tests
import manifestupdate
File "c:\mozilla-source\mozilla-central\testing/web-platform\manifestupdate.py", line 19, in <module>
import manifestdownload
File "c:\mozilla-source\mozilla-central\testing/web-platform\manifestdownload.py", line 8, in <module>
import mozversioncontrol
ModuleNotFoundError: No module named 'mozversioncontrol'

I could not find any documentation on how to install mozversioncontrol.
Can you please help me out.
Thanks in advance.

Armand.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Thanks for stopping by! I'm happy to see you've gotten a little ways already.

Python setups are a bit peculiar; two thoughts:

1. I wonder if jstests.py is a bit sensitive to your current working directory. I'd try running it from the directory above: python3 tests/jstests.py build_OPT.OBJ/dist/bin/js.exe
2. I'd recommend investing a little time in setting up a MOZCONFIG as documented in the in-tree build docs; I know it's a little different, but the nice thing is all the pieces are well integrated. If you have that running such that you can build with ./mach build, then running the jstests.py script is a simple matter of ./mach jstests

Comment 3

[André Bargull [:anba]]

This error is related to the WPT tests run by jstests.py, so if you don't care about those, you can also use ./tests/jstests.py ${BUILDDIR}/dist/bin/js non262/ and ./tests/jstests.py ${BUILDDIR}/dist/bin/js test262/ to run the other two test suites from jstests.

Comment 4

[makarmani]

@Matthew

1. is not working

2. i created both variants of a MOZCONFIG:
   if i run ./mach build (preceded with export MOZCONFIG=$HOME/mozconfigs/debug) i get this error:
   0:34.16 c:/mozilla-source/mozilla-central/obj-x86_64-pc-mingw32/js/src\js-config.h(28,4): error: "SpiderMonkey was configured with --disable-debug, so DEBUG must be not defined when including this header"
   0:34.16 # error "SpiderMonkey was configured with --disable-debug, so DEBUG must be not defined when including this header"

if i then change in the DEBUG MOZCONFIG: ac_add_options --enable-debug to --enable-debug and rerun ./mach build, i get this error:
0:50.68 mfbt/tests
0:54.26 In file included from Unified_cpp_js_src_shell0.cpp:29:
0:54.26 c:/mozilla-source/mozilla-central/js/src/shell/js.cpp(64,12): fatal error: 'prerror.h' file not found
0:54.26 # include "prerror.h"
0:54.26 ^~~~~~~~~~~
0:55.18 1 error generated.
0:55.22 mozmake.EXE[4]: *** [c:/mozilla-source/mozilla-central/config/rules.mk;748: Unified_cpp_js_src_shell0.obj] Error 1
0:55.22 mozmake.EXE[3]: *** [c:/mozilla-source/mozilla-central/config/recurse.mk;72: js/src/shell/target-objects] Error 2
0:55.22 mozmake.EXE[3]: *** Waiting for unfinished jobs....
1:44.55 c:/mozilla-source/mozilla-central/intl/icu/source/common/unistr.cpp(1979,13): warning: unused function 'uprv_UnicodeStringDummy' [-Wunused-function]
1:44.55 static void uprv_UnicodeStringDummy(void) {
1:44.55 ^
1:44.67 1 warning generated.
1:50.48 mozmake.EXE[2]: *** [c:/mozilla-source/mozilla-central/config/recurse.mk;34: compile] Error 2
1:50.48 mozmake.EXE[1]: *** [c:/mozilla-source/mozilla-central/config/rules.mk;390: default] Error 2
1:50.48 mozmake.EXE: *** [client.mk;125: build] Error 2
1:50.51 209 compiler warnings present.

if i run ./mach build (preceded with export MOZCONFIG=$HOME/mozconfigs/optimized) it works.
But if i follow up with ./mach jstests, the tests seems not too work:
$ ./mach jstests
New python executable in c:\mozilla-source\mozilla-central\obj-opt-x86_64-pc-mingw32_virtualenvs\init\Scripts\python2.7.exe
Also creating executable in c:\mozilla-source\mozilla-central\obj-opt-x86_64-pc-mingw32_virtualenvs\init\Scripts\python.exe
Installing setuptools, pip, wheel...
done.
Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[ 472| 0| 0| 10] 1% > | 5.9s

non262\Date\time-zones-posix.js: rc = 3, run time = 0.186

c:\mozilla-source\mozilla-central\js\src\tests\non262\Date\shell.js:160:21 Error: Assertion failed: got "Nordamerikanische Ostk\xFCsten-Sommerzeit", expected "EDT"
Stack:
inTimeZone@c:\mozilla-source\mozilla-central\js\src\tests\non262\Date\shell.js:97:24
@c:\mozilla-source\mozilla-central\js\src\tests\non262\Date\time-zones-posix.js:46:11
REGRESSION - non262\Date\time-zones-posix.js
[ 474| 1| 0| 10] 1% > | 6.0s

non262\Date\time-zone-pst.js: rc = 3, run time = 0.189

c:\mozilla-source\mozilla-central\js\src\tests\non262\Date\time-zone-pst.js:14:13 Error: Assertion failed: got "Sun Nov 06 2016 00:00:00 GMT-0700 (Nordamerikanische Westk\xFCsten-Sommerzeit)", expected "Sun Nov 06 2016 00:00:00 GMT-0700 (PDT)"
Stack:
@c:\mozilla-source\mozilla-central\js\src\tests\non262\Date\time-zone-pst.js:42:21
REGRESSION - non262\Date\time-zone-pst.js
[ 476| 2| 0| 10] 1% > | 6.1s

non262\Date\time-zone-2038-pst.js: rc = 3, run time = 0.19

c:\mozilla-source\mozilla-central\js\src\tests\non262\Date\shell.js:160:21 Error: Assertion failed: got "Nordamerikanische Westk\xFCsten-Sommerzeit", expected "PDT"
Stack:
assertDateTime@c:\mozilla-source\mozilla-central\js\src\tests\non262\Date\shell.js:160:21
@c:\mozilla-source\mozilla-central\js\src\tests\non262\Date\time-zone-2038-pst.js:27:19
REGRESSION - non262\Date\time-zone-2038-pst.js
[ 1088| 3| 0| 19] 2% > | 15.7ss

non262\Intl\default-locale-shell.js: rc = 3, run time = 0.172

c:\mozilla-source\mozilla-central\js\src\tests\non262\Intl\default-locale-shell.js:7:9 Error: Assertion failed: got "de-DE", expected "en-US"
Stack:
@c:\mozilla-source\mozilla-central\js\src\tests\non262\Intl\default-locale-shell.js:7:9
REGRESSION - non262\Intl\default-locale-shell.js
[19821| 4| 0| 255] 48% =================> | 338.5s

test262\intl402\DateTimeFormat\prototype\resolvedOptions\order-dayPeriod.js: rc = 3, run time = 0.223

c:\mozilla-source\mozilla-central\js\src\tests\test262\shell.js:414:9 uncaught exception: Test262Error: Expected true but got false
Stack:
$ERROR@c:\mozilla-source\mozilla-central\js\src\tests\test262\shell.js:414:9
assert@c:\mozilla-source\mozilla-central\js\src\tests\test262\shell.js:20:9
@c:\mozilla-source\mozilla-central\js\src\tests\test262\intl402\DateTimeFormat\prototype\resolvedOptions\order-dayPeriod.js:34:7
REGRESSION - test262\intl402\DateTimeFormat\prototype\resolvedOptions\order-dayPeriod.js
[37699| 5| 0| 3963] 100% ======================================>| 444.2s
REGRESSIONS
non262\Date\time-zones-posix.js
non262\Date\time-zone-pst.js
non262\Date\time-zone-2038-pst.js
non262\Intl\default-locale-shell.js
test262\intl402\DateTimeFormat\prototype\resolvedOptions\order-dayPeriod.js
FAIL

That's what i'm facing now

Comment 5

[makarmani]

@André both ways failed

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Can you set your MOZCONFIG to your debug one, then run mach clobber then run mach build again? It seems like your object directly is in a weird state.

Comment 7

[makarmani]

@Matthew
Yes i can set MOZCONFIG to debug, then run ./mach clobber followed by ./mach build, without issues.
But still ONLY this one test is still failing:

$ python3 tests/jstests.py build_DBG.OBJ/dist/bin/js.exe Traceback (most recent call last):
File "tests/jstests.py", line 678, in <module>
sys.exit(main())
File "tests/jstests.py", line 567, in main
test_count, test_gen = load_tests(options, requested_paths, excluded_paths)
File "tests/jstests.py", line 488, in load_tests
excluded_paths)
File "tests/jstests.py", line 367, in load_wpt_tests
import manifestupdate
File "c:\mozilla-source\mozilla-central\testing/web-platform\manifestupdate.py", line 19, in <module>
import manifestdownload
File "c:\mozilla-source\mozilla-central\testing/web-platform\manifestdownload.py", line 8, in <module>
import mozversioncontrol
ModuleNotFoundError: No module named 'mozversioncontrol'

Can i somehow anyway proceed with fixing this bug?

Comment 8

[André Bargull [:anba]]

(In reply to makarmani from comment #7)

@Matthew
[...]
Can i somehow anyway proceed with fixing this bug?

Answering for Matthew: Yes, I think you can proceed here. This bug is about refactoring, so it shouldn't lead to any observable changes which may lead to jstests failures. And when the patch is ready, it'll get uploaded to the CI system, which will run jstests, so if anything goes wrong, it'll be detected at some point. So, as long as the build itself completes and the binary can be successfully compiled, you can start writing the patch. Thanks!

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

Agreed; you should still be able to work on this. I'm sorry you're having a poor experience like this.

I do notice in that last post that you're still doing $ python3 tests/jstests.py build_DBG.OBJ/dist/bin/js.exe rather than running $ mach jstests -- does mach jstests not work for you either?

Comment 10

[yozaam]

Does this apply to keyValue.isSymbol() && keyValue.toSymbol()->isPrivateName())

as well? those are HandleValue in js/src/jit/BaseLineIC.cpp and a few other places keyValue or idValue

Or only the places with JSID_IS_SYMBOL(id) && JSID_TO_SYMBOL(id)->isPrivateName());

Comment 11

[yozaam]

I had another doubt, instead of making it a method on JS::PropertyKey in js/public/Id.h

why didn't we just add a initial check for is "is symbol" inside the original isPrivateName?

over here -> https://searchfox.org/mozilla-central/source/js/src/vm/SymbolType.h#89

Comment 12

[yozaam]

Okay I answered my own doubt about JSID_IS_SYMBOL and HandleValues,

It's the same thing, sorry
js/public/Id.h
181 static MOZ_ALWAYS_INLINE bool JSID_IS_SYMBOL(jsid id) { return id.isSymbol(); }

Comment 13

[yozaam]

Attachment: Bug 1655110 added isPrivateName method to replace JSID manipulations r?mgaudet

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05d190d8303e
added isPrivateName method to replace JSID manipulations r=mgaudet

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/05d190d8303e