Closed Bug 1731218 Opened 4 months ago Closed 4 months ago

Rooted implementation casts between unrelated types

Categories

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

task

Tracking

()

RESOLVED FIXED
94 Branch
Tracking Status
firefox94 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

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.

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.

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

This removes the need for a bunch of reinterpret_casts.

Depends on D125950

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

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

Depends on D125952

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

Pushed by jcoppeard@mozilla.com:
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

Backed out for causing spider-monkey bustages in RootingAPI

Backout link

Push with failures

Failure log

Flags: needinfo?(jcoppeard)
Pushed by jcoppeard@mozilla.com:
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
Flags: needinfo?(jcoppeard)
You need to log in before you can comment on or make changes to this bug.