1601074 - Hit MOZ_CRASH(There should be room for registering the new object) at js/src/vm/TypeInference.cpp:3494

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision d1001fea6e4c (build with --enable-debug --disable-optimize, run with --fuzzing-safe --no-threads --no-baseline --no-ion --no-ggc):

for (let y of [
    { x: 1 },
    { x: 2 },
    { x: 3 },
    { x: 4 },
    { x: 5 },
    { x: 6 },
    { x: 7 },
    { x: 8 },
    { x: 9 },
    { x: 10 },
    { x: 11 },
]) {}

Backtrace:

#0  0x0000561deba5b0b1 in js::PreliminaryObjectArray::registerNewObject (this=0x7fa67da1fdf0, res=0x2de229e7c580) at js/src/vm/TypeInference.cpp:3494
#1  0x0000561deb3b0f0e in js::NewObjectOperation (cx=0x7fa67dc27000, script=..., pc=0x7fa67d4418c4 "\362\v", newKind=js::GenericObject) at js/src/vm/Interpreter.cpp:5266
#2  0x0000561deb3a0325 in Interpret (cx=0x7fa67dc27000, state=...) at js/src/vm/Interpreter.cpp:3801
#3  0x0000561deb38dffa in js::RunScript (cx=0x7fa67dc27000, state=...) at js/src/vm/Interpreter.cpp:424
#4  0x0000561deb3a8939 in js::ExecuteKernel (cx=0x7fa67dc27000, script=..., envChainArg=..., newTargetValue=..., evalInFrame=..., result=0x0) at js/src/vm/Interpreter.cpp:811
#5  0x0000561deb3a8d0d in js::Execute (cx=0x7fa67dc27000, script=..., envChainArg=..., rval=0x0) at js/src/vm/Interpreter.cpp:844
#6  0x0000561deb65bbd3 in ExecuteScript (cx=0x7fa67dc27000, scope=..., script=..., rval=0x0) at js/src/vm/CompilationAndEvaluation.cpp:453
/snip

For detailed crash information, see attachment.

Setting s-s as a start because opt ASan builds show an error too, log coming up.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

Attachment: opt ASan stack

Comment 3

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

Setting [fuzzblocker] as this is happening very often.

Comment 4

[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/7bd0784e7a6f
user: Chris Fallin
date: Wed Nov 27 22:49:33 2019 +0000
summary: Bug 1598347, part 2: pass "inner singleton" to NEWOBJECT group logic. r=djvj,iain

Chris, is bug 1598347 a likely regressor?

Comment 5

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

Pernosco link:

https://pernos.co/debug/MV7LbNapmOux3voOGhlQlg/index.html

Comment 6

[Chris Fallin [:cfallin]]

Yes, this is almost certainly due to the recent change for bug 1598347. Looking into it!

Comment 7

[Chris Fallin [:cfallin]]

Attachment: Bug 1601074: Fix preliminary-objects-set issue wrt NEWOBJECT_WITHGROUP. r=iain,djvj

The recent addition of the JSOP_NEWOBJECT_WITHGROUP opcode for bug
1598347 (itself a regression fix for 1580246) has led to an issue when
more than a certain number of array elements with the same group are
created within an array literal. In particular, when too many objects
are created, the preliminary-objects-set for the ObjectGroup becomes
full and hits a MOZ_CRASH. This patch checks whether the set is full
before adding to it.

Comment 8

[Andrew McCreight [:mccr8]]

Is this a security issue, or is this just going to be a MOZ_CRASH() any time we get in this bad state? Thanks.

Comment 9

[Chris Fallin [:cfallin]]

This is just a MOZ_CRASH, not a security issue -- the PreliminaryObjectArray will not overflow its bounds. (See https://searchfox.org/mozilla-central/source/js/src/vm/TypeInference.cpp#3481 for its implementation.) The commit that caused this issue had changed some of the conditions under which objects are added to this array, or which array they are added to, hence the possibility of trying to add to an already-full array. (It's perfectly fine to not include objects, as they only improve analysis precision, so the fix is just to check if full first and do nothing.)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4c1a6acc2b1
Fix preliminary-objects-set issue wrt NEWOBJECT_WITHGROUP. r=iain

Comment 11

[Andreea Pavel [:apavel]]

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

Comment 12

[Chris Fallin [:cfallin]]

Comment on attachment 9113328 [details]
Bug 1601074: Fix preliminary-objects-set issue wrt NEWOBJECT_WITHGROUP. r=iain,djvj

Beta/Release Uplift Approval Request

• User impact if declined: Slight regression in performance (fewer JS functions are inlined) in some cases.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1598347
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): (This is meant to go along with 1598347, so is the same text reproduced below:)

Changes to the guts of the JS engine are always slightly risky, but this has been on m-c for a few days, and the one issue that arose (1601074) was caught quickly by the fuzzing team and fixed. Given that no further issues have arisen, and given that the engine is well-covered by tests, we believe that this risk is not unacceptably high.

• String changes made/needed:

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9113328 [details]
Bug 1601074: Fix preliminary-objects-set issue wrt NEWOBJECT_WITHGROUP. r=iain,djvj

companion fix for bug 1598347, approved for 72.0b4

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a37ac55dc781