1593399 - WeakMap marking can fail to properly preserve gray keys with black delegates

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[Steve Fink [:sfink] [:s:]]

I found this while adding a test for some refactoring I was doing. It's kind of a niche case, but I wrote a test case that I think could happen in real browser operation. It's where you have a gray key with a black delegate, and in certain situations you could end up with the key never getting blackened. If it happens to be a member of a CC-collected gray cycle, then this could result in a WeakMap entry being discarded prematurely.

See the comment in the added jit-test gc/weak-marking-03.js test for details. This test fails in the currently landed version.

There is probably a more targeted fix that relies on the recently added ability to mark black during gray marking, but this modified marking logic for weakmaps is in my opinion much easier to follow and more likely to be correct.

Comment 1

[Steve Fink [:sfink] [:s:]]

Uh... what happened to my patch? Sorry about that.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1593399 - Rework how mark colors are handled in weakmap marking

Comment 3

[Steve Fink [:sfink] [:s:]]

Weird. That time it worked. Clearly I successfully create the phabricator revision before, since you saw it and reviewed it. It's probably from me using phabsend instead of moz-phab, but it worked this time.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de7a1a1b75f0
Rework how mark colors are handled in weakmap marking r=jonco

Comment 5

[Cosmin Sabou [:CosminS]]

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

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=275859005&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/9d2cd9e8b06dd503210799fad4474b49f9371819

[task 2019-11-12T20:18:06.576Z] 20:18:06 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -o Unified_cpp_js_src_debugger0.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DWASM_SUPPORTS_HUGE_MEMORY -DJS_CACHEIR_SPEW -DJS_STRUCTURED_SPEW -DJS_HAS_CTYPES -DFFI_BUILDING -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -I/builds/worker/workspace/build/src/js/src/debugger -I/builds/worker/workspace/build/src/obj-firefox/js/src/debugger -I/builds/worker/workspace/build/src/obj-firefox/js/src -I/builds/worker/workspace/build/src/js/src -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/js/src/js-confdefs.h -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-noexcept-type -Wno-unknown-warning-option -Wno-return-type-c-linkage -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -fno-strict-aliasing -Werror=format -Wno-shadow -MD -MP -MF .deps/Unified_cpp_js_src_debugger0.o.pp Unified_cpp_js_src_debugger0.cpp
[task 2019-11-12T20:18:06.576Z] 20:18:06 INFO - In file included from Unified_cpp_js_src_debugger0.cpp:11:
[task 2019-11-12T20:18:06.577Z] 20:18:06 INFO - In file included from /builds/worker/workspace/build/src/js/src/debugger/Debugger.cpp:120:
[task 2019-11-12T20:18:06.578Z] 20:18:06 ERROR - /builds/worker/workspace/build/src/js/src/gc/WeakMap-inl.h:115:13: error: unused variable 'oldKey' [-Werror,-Wunused-variable]
[task 2019-11-12T20:18:06.578Z] 20:18:06 INFO - gc::Cell* oldKey = gc::ToMarkable(p->key());
[task 2019-11-12T20:18:06.579Z] 20:18:06 INFO - ^
[task 2019-11-12T20:18:06.579Z] 20:18:06 INFO - 1 error generated.
[task 2019-11-12T20:18:06.580Z] 20:18:06 INFO - /builds/worker/workspace/build/src/config/rules.mk:787: recipe for target 'Unified_cpp_js_src_debugger0.o' failed
[task 2019-11-12T20:18:06.580Z] 20:18:06 ERROR - make[4]: *** [Unified_cpp_js_src_debugger0.o] Error 1
[task 2019-11-12T20:18:06.580Z] 20:18:06 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/js/src/debugger'
[task 2019-11-12T20:18:06.581Z] 20:18:06 INFO - /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'js/src/debugger/target-objects' failed
[task 2019-11-12T20:18:06.581Z] 20:18:06 ERROR - make[3]: *** [js/src/debugger/target-objects] Error 2
[task 2019-11-12T20:18:06.582Z] 20:18:06 INFO - make[3]: *** Waiting for unfinished jobs....
[task 2019-11-12T20:18:06.583Z] 20:18:06 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/js/src/jsapi-tests'
[task 2019-11-12T20:18:06.583Z] 20:18:06 INFO - js/src/jsapi-tests/Unified_cpp_js_src_jsapi-tests1.o

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8daa186bd18b
Rework how mark colors are handled in weakmap marking r=jonco

Comment 7

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=android%2C7.0%2Cx86-64%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-android-em-7.0-x86_64%2Fopt-geckoview-web-platform-tests-e10s-14%2Cw%28wpt14%29&fromchange=c5c7e5fc0307caa3a6e455a98ccfa7a5b9b4459a&group_state=expanded&tochange=5b26b747f3bd5be47e43b07531a88cc292dfdebb&selectedJob=275945134

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=275945134&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=275944958&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/7d9725c3f834323fa9b39cf1b1d2604ce9d5c35d

See Bug 1595992.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5decb743461a
Rework how mark colors are handled in weakmap marking r=jonco

Comment 9

[Steve Fink [:sfink] [:s:]]

Passed try, looking good so far on autoland. Fingers crossed.

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/5decb743461a