1731218 - Rooted implementation casts between unrelated types

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

The implementation of Rooted currently casts between different Rooted<T> classes that are not related by inheritance. Although this currently works everywhere, this is not guaranteed and it's generally pretty dubious.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1731218 - Part 0: Remove unnecessary template from Rooted constructor r?sfink

I don't think this does anything. The type is always present so we won't
restrict use of this constructor by SFINAE, although I may have misunderstood
the purpose of this.

I tried 'fixing' the SFINAE to what I though it should be by using enable_if_t
but it had the same effect as removing it. In both cases the code compiles, and
if I try to construct a rooted with an inappropriate argument it fails with a
sensible error.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1731218 - Part 1: Rename rooting API base classes to 'Operations' in prepartion for adding base classes r?sfink

These are really mixins used to add public methods based on the type and don't
need to the base of an inheritance tree.

Depends on D125949

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1731218 - Part 2: Give PersistentRooted a base class and use this refer to these roots when we don't know the type r?tcampbell

This removes the need for a bunch of reinterpret_casts.

Depends on D125950

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1731218 - Part 3: Give Rooted a base class and use this refer to these roots when we don't know the type r?tcampbell

The link pointers move to the base class in a similar way to PersistentRootedBase.
This lets us get rid of a bunch more reinterpret_casts.

Depends on D125951

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1731218 - Part 4: Add typed base classes for roots r?tcampbell

This adds typed base classes with trace() methods. RootedTraceable is removed
and we store T directly in Rooted.

Depends on D125952

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1731218 - Part 5: Update Rooted constructor test expectations r?sfink

I don't know exactly why this is happening but we're now directly constructing
MyContainer and not copying it. I confirmed that this is calling the first
Rooted consturctor so I updated the tests to expect this.

Depends on D125953

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1731218 - Part 6: Check that we don't increase the size of Rooted for GC thing types r?sfink

Depends on D125954

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06e7a1992de8
Part 0: Remove unnecessary template from Rooted constructor r=sfink
https://hg.mozilla.org/integration/autoland/rev/304f27af6b95
Part 1: Rename rooting API base classes to 'Operations' in prepartion for adding base classes r=sfink
https://hg.mozilla.org/integration/autoland/rev/3b264a4bc67e
Part 2: Give PersistentRooted a base class and use this refer to these roots when we don't know the type r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/57e60277e4ca
Part 3: Give Rooted a base class and use this refer to these roots when we don't know the type r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/3e98b832dcc6
Part 4: Add typed base classes for roots r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/e2c59b5af7ea
Part 5: Update Rooted constructor test expectations r=sfink
https://hg.mozilla.org/integration/autoland/rev/0dfcae852026
Part 6: Check that we don't increase the size of Rooted for GC thing types r=sfink

Comment 9

[Sandor Molnar[:smolnar]]

Backed out for causing spider-monkey bustages in RootingAPI

Backout link

Push with failures

Failure log

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c9145f71f55
Part 0: Remove unnecessary template from Rooted constructor r=sfink
https://hg.mozilla.org/integration/autoland/rev/b45dc61300b1
Part 1: Rename rooting API base classes to 'Operations' in prepartion for adding base classes r=sfink
https://hg.mozilla.org/integration/autoland/rev/bc5898ff5c3d
Part 2: Give PersistentRooted a base class and use this refer to these roots when we don't know the type r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/d7e74bfc08ef
Part 3: Give Rooted a base class and use this refer to these roots when we don't know the type r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/ac495f8eecd2
Part 4: Add typed base classes for roots r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/a9372964d060
Part 5: Update Rooted constructor test expectations r=sfink
https://hg.mozilla.org/integration/autoland/rev/f76391987517
Part 6: Check that we don't increase the size of Rooted for GC thing types r=sfink

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5c9145f71f55
https://hg.mozilla.org/mozilla-central/rev/b45dc61300b1
https://hg.mozilla.org/mozilla-central/rev/bc5898ff5c3d
https://hg.mozilla.org/mozilla-central/rev/d7e74bfc08ef
https://hg.mozilla.org/mozilla-central/rev/ac495f8eecd2
https://hg.mozilla.org/mozilla-central/rev/a9372964d060
https://hg.mozilla.org/mozilla-central/rev/f76391987517