1734392 - Add a read barrier when constructing JS::Heap<T>

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

Heap<T> has a read barrier when extracting the contained value, but this it doesn't when constructing one from another Heap<T> (in either the copy or move constructor). The current barriers are designed to unmark gray values that may be passed into the engine, and such a barrier is not required for that.

With incremental gray marking we need to ensure that we don't move gray roots behind the marking wavefront, which would allow live to things to go unmarked. It would be possible to do this by copying or moving a Heap<T>. For example, you could std::swap two of these between marking slices.

To close this loophole we can add a read barrier on construction. It doesn't have to do gray unmarking but just needs to do an incremental read barrier and mark the value black if we're currently marking the zone.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1734392 - Part 1: Inline black marking check in ExposeGCThingToActiveJS r?sfink

This already had code to check both black and gray bit since that's needed to
work out whether something is marked gray. We can do the black check first and
bail early which skips calling into the JS engine for black things.

Depends on D127671

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1734392 - Part 2: Add a read barrier on JS::Heap<T> construction r?sfink

Depends on D127672

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1734392 - Part 3: Add a test for Heap<T> constructor read barriers r?sfink

Depends on D127673

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd61ff016587
Part 1: Inline black marking check in ExposeGCThingToActiveJS r=sfink
https://hg.mozilla.org/integration/autoland/rev/c55d9ca8c869
Part 2: Add a read barrier on JS::Heap<T> construction r=sfink
https://hg.mozilla.org/integration/autoland/rev/53f7d8f6758a
Part 3: Add a test for Heap<T> constructor read barriers r=sfink

Comment 5

[Natalia Csoregi [:nataliaCs]]

Backed out along with Bug 1734362 for bustages on TestJSHolderMap.cpp - see comment

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8eee170a2362
Backed out 5 changesets (bug 1734392, bug 1734362) for bustages on TestJSHolderMap.cpp. CLOSED TREE

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a23cc095af5
Part 1: Inline black marking check in ExposeGCThingToActiveJS r=sfink
https://hg.mozilla.org/integration/autoland/rev/d1724fd8bf2b
Part 2: Add a read barrier on JS::Heap<T> construction r=sfink
https://hg.mozilla.org/integration/autoland/rev/d69187f5ce74
Part 3: Add a test for Heap<T> constructor read barriers r=sfink

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/4a23cc095af5
https://hg.mozilla.org/mozilla-central/rev/d1724fd8bf2b
https://hg.mozilla.org/mozilla-central/rev/d69187f5ce74