1657554 - Assertion failure: !obj->as<WeakRefObject>().target(), at builtin/WeakRefObject.cpp:127

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200806-6e35e01646d7 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

oomTest(() => eval("new WeakRef({});"));

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555557246280 in js::WeakRefObject::finalize(JSFreeOp*, JSObject*) ()
#1  0x00005555575cd58a in JSObject::finalize(JSFreeOp*) ()
#2  0x00005555575ccc4f in unsigned long js::gc::Arena::finalize<JSObject>(JSFreeOp*, js::gc::AllocKind, unsigned long) ()
#3  0x00005555575cc90a in bool FinalizeTypedArenas<JSObject>(JSFreeOp*, js::gc::Arena**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&) ()
#4  0x000055555758e97c in FinalizeArenas(JSFreeOp*, js::gc::Arena**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&) ()
#5  0x00005555575ab319 in js::gc::ArenaLists::foregroundFinalize(JSFreeOp*, js::gc::AllocKind, js::SliceBudget&, js::gc::SortedArenaList&) ()
#6  0x00005555575ad042 in js::gc::GCRuntime::finalizeAllocKind(JSFreeOp*, js::SliceBudget&) ()
#7  0x00005555575dc694 in sweepaction::SweepActionForEach<ContainerIter<mozilla::EnumSet<js::gc::AllocKind, unsigned long> >, mozilla::EnumSet<js::gc::AllocKind, unsigned long> >::run(js::gc::SweepAction::Args&) ()
#8  0x00005555575ecd11 in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) ()
#9  0x00005555575dc06c in sweepaction::SweepActionForEach<js::gc::SweepGroupZonesIter, JSRuntime*>::run(js::gc::SweepAction::Args&) ()
#10 0x00005555575ecd11 in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) ()
#11 0x00005555575db7c7 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) ()
#12 0x00005555575ad9a3 in js::gc::GCRuntime::performSweepActions(js::SliceBudget&) ()
#13 0x00005555575b2afa in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason, js::gc::AutoGCSession&) ()
#14 0x00005555575b5a24 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#15 0x00005555575b760d in js::gc::GCRuntime::collect(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#16 0x000055555756f50d in js::gc::GCRuntime::gc(JSGCInvocationKind, JS::GCReason) ()
#17 0x0000555557092445 in JSRuntime::destroyRuntime() ()
#18 0x0000555556fa18c2 in js::DestroyContext(JSContext*) ()
#19 0x0000555556b2713e in main ()
rax	0x5555558e7676	93824995980918
rbx	0x5555583c7410	93825040938000
rcx	0x555558473a80	93825041644160
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffd130	140737488343344
rsp	0x7fffffffd130	140737488343344
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f9ddc0	140737353735616
r10	0x58	88
r11	0x7ffff6dac7a0	140737334921120
r12	0x1d00825a51c0	31888024162752
r13	0x1d00825a51c0	31888024162752
r14	0x7fffffffd510	140737488344336
r15	0x1d00825a51c0	31888024162752
rip	0x555557246280 <js::WeakRefObject::finalize(JSFreeOp*, JSObject*)+80>
=> 0x555557246280 <_ZN2js13WeakRefObject8finalizeEP8JSFreeOpP8JSObject+80>:	movl   $0x7f,0x0
   0x55555724628b <_ZN2js13WeakRefObject8finalizeEP8JSFreeOpP8JSObject+91>:	callq  0x555556bb5e3e <abort>

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200806033456-6e35e01646d7.
The bug appears to have been introduced in the following build range:
> Start: ede1c973aa858b334a495870fd9bbc019b0e94f9 (20200614092422)
> End: 75b4198a731db7dcbfc6280bf25639b083e36cca (20200614070503)
> Pushlog: https://hg.mozilla.org/mozilla-unified/pushloghtml?fromchange=ede1c973aa858b334a495870fd9bbc019b0e94f9&tochange=75b4198a731db7dcbfc6280bf25639b083e36cca

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1657554 - Don't set WeakRef target until we know that construction has succeeded r?sfink

This seems like a nice assertion to have so arrange that the weakref target is not set if registration fails. Also I fixed places where we didn't report out of memory on failure.

Depends on D86185

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67cdcf5da6a6
Don't set WeakRef target until we know that construction has succeeded r=sfink

Comment 5

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 67cdcf5da6a6 (bug 1657554) for causing SM bustages at js/src/jit-test/tests/gc/bug-1657554.js.
https://hg.mozilla.org/integration/autoland/rev/68c28339b590bd28cdd37a38e347c04c33334f17

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=67cdcf5da6a608ff6db2238294dbad0db6443ab8&selectedTaskRun=S-u13ZBrSGmshkxEoDwhQg.0

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2989be7adc88
Don't set WeakRef target until we know that construction has succeeded r=sfink

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2989be7adc88

Comment 8

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200807152823-5860b7b7c7a4.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jonco, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Jon Coppeard (:jonco)]

I don't think the bug will cause problems. But the fix is super simple so let's uplift just in case.

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 9168463 [details]
Bug 1657554 - Don't set WeakRef target until we know that construction has succeeded r?sfink

Beta/Release Uplift Approval Request

• User impact if declined: Probably no impact. The fix ensures WeakRef objects are correctly initialized if there's OOM and I'm requesting uplift just to be on the safe side.
• 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: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is very simple moves initialization of the WeakRef's target slot until we know the whole initialization process will succeed.
• String changes made/needed: None.

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9168463 [details]
Bug 1657554 - Don't set WeakRef target until we know that construction has succeeded r?sfink

approved for 80.0b7

Comment 13

[Julien Cristau [:jcristau]]

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