1572988 - Assertion failure: mallocedBuffers.has(buffer), at js/src/gc/Nursery.h:313

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision 3afb892abb74 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/jit-test/tests/debug/bug1375447.js
g = newGlobal({
    newCompartment: true
});
// Adapted from randomly chosen test: js/src/tests/non262/RegExp/replace-trace.js
function P() {
    return new Proxy([], {
        get(t, y) {
            x += "et" + y + "],";
        }
    })
}
x = e = [P(), null];
// jsfunfuzz-generated
oomTest(Promise.all);

Backtrace:

#0  js::Nursery::removeMallocedBuffer (this=0x7f11d3f29260, buffer=<optimized out>) at js/src/gc/Nursery.h:313
#1  0x000055d1bb583f3c in js::TenuringTracer::moveStringToTenured (this=0x7fffd7495778, dst=0x3b1905a37730, src=0x1852fc238c78, dstKind=<optimized out>) at js/src/gc/Marking.cpp:3279
#2  0x000055d1bb5820af in js::TenuringTracer::moveToTenured (this=0x7fffd7495778, src=0x1852fc238c78) at js/src/gc/Marking.cpp:3233
#3  0x000055d1bb581f1d in js::TenuringTracer::traverse<JSString> (this=0x7f11d5070680 <_IO_2_1_stderr_>, strp=0x3b1905a37708) at js/src/gc/Marking.cpp:2770
#4  0x000055d1bb57d12c in js::gc::TraceEdgeInternal<JSString*> (trc=0x7fffd7495778, thingp=0x7f11d50718b0 <_IO_stdfile_2_lock>, name=<optimized out>) at js/src/gc/Marking.cpp:590
#5  js::TraceManuallyBarrieredEdge<JSString*> (trc=0x7fffd7495778, thingp=0x7f11d50718b0 <_IO_stdfile_2_lock>, name=<optimized out>) at js/src/gc/Tracer.h:191
#6  JSRope::traceChildren (this=0x3b1905a37700, trc=0x7fffd7495778) at js/src/gc/Marking.cpp:1123
/snip

For detailed crash information, see attachment.

Setting s-s as a start as this is a GC assert.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/0372b25e5147
user: Jon Coppeard
date: Thu Jun 27 17:09:40 2019 +0100
summary: Bug 1558835 - Make Nursery::removeMallocedBuffer assert that its argument is valid and don't call it unless necessary r=allstarschh

Jon, is bug 1558835 a likely regressor?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I have a Pernosco session for this:

https://pernos.co/debug/3wmNhfKsStiffRjMUQ5KFA/index.html

Comment 4

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

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)

Jon, is bug 1558835 a likely regressor?

It exposed the problem, but it already existed.

What I'm seeing is strange -- the string's chars were inserted into mallocedBuffers during flattenInternal, then I didn't see any removals, but when the string is tenured and it looks up the same pointer, it isn't found. I don't know if this works, but my notebook at https://pernos.co/debug/3wmNhfKsStiffRjMUQ5KFA/index.html#f{m[OT8,B2k_,t[AQ,B90_,f{e[OTw,BQ0_,s{aVV4Pa4AA,bBA,oBFZc5A,uBDJ5gQ___ shows both points in time. I tried to break on relevant modifications in between, but didn't see anything. Unfortunately, the debug info is limited (and everything relevant is inlined away), and the Hashtable.h installed header is unavailable, so it's really hard to step through to see if the insertion is failing and returning true anyway, or what.

Comment 5

[Jon Coppeard (:jonco)]

I suspect the problem is the lack of error checking here:

https://searchfox.org/mozilla-central/source/js/src/vm/StringType.cpp#594

But I'm not sure how to fix this. Steve do you know if we can just bail out at this point?

Comment 6

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

(In reply to Jon Coppeard (:jonco) from comment #5)

I suspect the problem is the lack of error checking here:

https://searchfox.org/mozilla-central/source/js/src/vm/StringType.cpp#594

Oh wow, I'm blind. I didn't even see that registration; I got fixated on the other one on line 615 that handles failure correctly.

But I'm not sure how to fix this. Steve do you know if we can just bail out at this point?

No, we've already converted the string to a dependent string so we'd be leaking if we just returned false. Hopefully some minor reordering will fix it. I'll work on a patch.

Comment 7

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

Attachment: Bug 1572988 - error check registerMallocedBuffer r=jonco

Comment 8

[Daniel Veditz [:dveditz]]

if bug 1558835 is the regressor then Fx69 is affected as well

Comment 9

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

Not security sensitive. If this happens, it will leak memory when it's near OOM and do js_free(nullptr), which is benign.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f6027c1f8c3
error check registerMallocedBuffer r=jonco

Comment 11

[Narcis Beleuzu [:NarcisB]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Can we land a test for this? Also, is there a user impact which justifies backport consideration here or can this fix ride Fx70 to release?

Comment 13

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)

Can we land a test for this? Also, is there a user impact which justifies backport consideration here or can this fix ride Fx70 to release?

No user impact other than a trivial memory leak. The only reason to backport would be if it showed up as a fuzzblocker, but I doubt it's that common.

Comment 14

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

Attachment: Bug 1572988 - add a test case for registration error checking

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4d3fccf8ad4
add a test case for registration error checking

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset d4d3fccf8ad4 (Bug 1572988) for regress-1572988-nurseryRegisterCheck.js failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=d4d3fccf8ad47595c97a7e794729db9f23b1f11d&tochange=a1366c2086c6f2f280ec0eb8d7e1d62a4b81deb7&selectedJob=262579465

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

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=262579465&repo=autoland&lineNumber=48197

task 2019-08-21T00:35:49.808Z] TEST-PASS | non262/regress/regress-165201.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.0 s]
[task 2019-08-21T00:35:49.812Z] TEST-PASS | non262/regress/regress-174709.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.2 s]
[task 2019-08-21T00:35:49.813Z] TEST-PASS | non262/regress/regress-455758-01.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.1 s]
[task 2019-08-21T00:35:49.820Z] TEST-PASS | non262/regress/regress-344959.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.0 s]
[task 2019-08-21T00:35:49.827Z] TEST-PASS | non262/regress/regress-433279-01.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.0 s]
[task 2019-08-21T00:35:49.833Z] TEST-PASS | non262/regress/regress-465241.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.1 s]
[task 2019-08-21T00:35:49.838Z] TEST-PASS | non262/regress/regress-440926.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.0 s]
[task 2019-08-21T00:35:49.844Z] TEST-PASS | non262/regress/regress-167658.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.0 s]
[task 2019-08-21T00:35:49.845Z] ## non262/regress/regress-1572988-nurseryRegisterCheck.js: rc = 3, run time = 0.042224
[task 2019-08-21T00:35:49.845Z] /builds/worker/workspace/build/src/js/src/tests/non262/regress/regress-1572988-nurseryRegisterCheck.js:24:1 ReferenceError: oomTest is not defined
[task 2019-08-21T00:35:49.846Z] Stack:
[task 2019-08-21T00:35:49.846Z] @/builds/worker/workspace/build/src/js/src/tests/non262/regress/regress-1572988-nurseryRegisterCheck.js:24:1
[task 2019-08-21T00:35:49.847Z] TEST-UNEXPECTED-FAIL | non262/regress/regress-1572988-nurseryRegisterCheck.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.0 s]
[task 2019-08-21T00:35:49.858Z] TEST-PASS | non262/regress/regress-465688.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.1 s]
[task 2019-08-21T00:35:49.858Z] TEST-PASS | non262/regress/regress-592202-4.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.1 s]
[task 2019-08-21T00:35:49.859Z] TEST-PASS | non262/regress/regress-452498-030.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.0 s]
[task 2019-08-21T00:35:49.871Z] TEST-PASS | non262/regress/regress-244619.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.1 s]
[task 2019-08-21T00:35:49.872Z] TEST-PASS | non262/regress/regress-350312.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.1 s]
[task 2019-08-21T00:35:49.872Z] TEST-PASS | non262/regress/regress-646820-2.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.0 s]
[task 2019-08-21T00:35:49.872Z] TEST-PASS | non262/regress/regress-452498-051.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.0 s]
[task 2019-08-21T00:35:49.885Z] TEST-PASS | non262/regress/regress-614714.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.0 s]

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7a48c0db946
add a test case for registration error checking

Comment 18

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

Attachment: Bug 1572988 - test followup to prevent intermittent timeouts on arm64 simulator

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a20a1d7f1f3
test followup to prevent intermittent timeouts on arm64 simulator

Comment 20

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c7a48c0db946

Comment 21

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

(Fixed with the above followup patches.)

Comment 22

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/4a20a1d7f1f3