1982134 - JS_RemoveExtraGCRootsTracer removes incorrect tracer

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Nicolas Auvray [:Itms]]

Steps to reproduce:

We embed SpiderMonkey ESR, and we have multiple instances of a class that contains JS::Heap<JSObject*> references. We use JS_AddExtraGCRootsTracer and JS_RemoveExtraGCRootsTracer to trace these JS objects.

Actual results:

When upgrading from ESR 115 to ESR 128, SpiderMonkey creates segfaults in our code by calling the Trace method of objects that are no longer in memory.

#1  IGUIObject::Trace (trc=0x5555561bd630, data=0x555574d560c0) at ../../../source/gui/ObjectBases/IGUIObject.h:498
#2  0x00007ffff765cb6c in js::gc::GCRuntime::traceEmbeddingBlackRoots (this=0x5555561e4e98, trc=0x5555561bd630)
    at libraries/source/spidermonkey/mozjs-128.13.0/js/src/gc/RootMarking.cpp:383
#3  js::gc::GCRuntime::traceRuntimeCommon (this=0x5555561e4e98, trc=0x5555561bd630, traceOrMark=js::gc::GCRuntime::MarkRuntime)
    at libraries/source/spidermonkey/mozjs-128.13.0/js/src/gc/RootMarking.cpp:367
#4  0x00007ffff765c7c8 in js::gc::GCRuntime::traceRuntimeForMajorGC (this=0x5555561e4e98, trc=0x5555561bd630, session=...)
    at libraries/source/spidermonkey/mozjs-128.13.0/js/src/gc/RootMarking.cpp:242
#5  0x00007ffff76299f9 in js::gc::GCRuntime::beginMarkPhase (this=0x5555561e4e98, session=...)
    at libraries/source/spidermonkey/mozjs-128.13.0/js/src/gc/GC.cpp:3019
#6  0x00007ffff762b667 in js::gc::GCRuntime::incrementalSlice (this=0x5555561e4e98, budget=..., reason=JS::GCReason::API, budgetWasIncreased=<optimized out>)
    at libraries/source/spidermonkey/mozjs-128.13.0/js/src/gc/GC.cpp:3802
#7  0x00007ffff762ca58 in js::gc::GCRuntime::gcCycle (this=0x5555561e4e98, nonincrementalByAPI=false, budgetArg=..., reason=1525896928)
    at libraries/source/spidermonkey/mozjs-128.13.0/js/src/gc/GC.cpp:4389
#8  0x00007ffff762d087 in js::gc::GCRuntime::collect (this=0x5555561e4e98, nonincrementalByAPI=<optimized out>, budget=..., reason=JS::GCReason::API)
    at libraries/source/spidermonkey/mozjs-128.13.0/js/src/gc/GC.cpp:4580

Expected results:

This issue is caused by GCRuntime::removeBlackRootsTracer which removes the callback for a wrong instance of our object. This is a regression introduced by Bug 1846835, in which the behavior of that method was modified during a refactoring.

Originally, the method compared the value of data in order to remove the callback for the correct object. Now, the method removes the first instance of the callback it finds.

Comment 1

[Nicolas Auvray [:Itms]]

Attachment: Bug 1982134 - Choose specific callback in removeBlackRootsTracer. r?jonco,sfink

This fixes a regression introduced in the refactoring at Bug 1846835 / D185307

Comment 2

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1846835

Comment 3

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

(This fix is landable and good, but I commented on the implied setup on the matrix channel)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/bf1994b05bae
https://hg.mozilla.org/integration/autoland/rev/2e6388013463
Choose specific callback in removeBlackRootsTracer. r=sfink

Comment 5

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/2e6388013463

Comment 6

[Nicolas Auvray [:Itms]]

Attachment: Bug 1982134 - Choose specific callback in removeBlackRootsTracer. r?jonco,sfink

This fixes a regression introduced in the refactoring at Bug 1846835 / D185307

Original Revision: https://phabricator.services.mozilla.com/D260541

Comment 7

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Embedders of SpiderMonkey ESR need to apply this patch in order to use the ExtraGCRootsTracer API without segfaulting.
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Not applicable
• Risk associated with taking this patch: None
• Explanation of risk level: This reverts the behavior of this code to the status of ESR115.
• String changes made/needed: No
• Is Android affected?: no

Comment 8

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/45a4336df6a8