1341752 - js::TraceChildren should assert that it's not called with a GC thing from another runtime

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

This would help prevent bugs like bug 1337414.

Comment 1

[Jon Coppeard (:jonco)]

To be more specific, some GC things are allowed to be referenced cross-runtime: permanent atoms, well known symbols and self-hosted data (see IsOwnedByOtherRuntime).  I think this may change in the future with the multi-threaded changes though.

There's also the issue of there being two overloads of TraceChildren.  Everything should be calling the version that takes a GCCellPtr now unless there's some good reason.

Comment 2

[Paul Bone [:pbone]]

Attachment: patch

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8870067 [details] [diff] [review]
patch

Review of attachment 8870067 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Marking.cpp
@@ -161,1 @@
>  template <> bool ThingIsPermanentAtomOrWellKnownSymbol<JSString>(JSString* str) {

You'll need to move all the template specialisations to the header file too otherwise the other .cpp file won't see them.

::: js/src/gc/Tracer.cpp
@@ +113,5 @@
>  
>  struct TraceChildrenFunctor {
>      template <typename T>
> +    void operator()(JSTracer* trc, void* _thing) {
> +        T* thing = static_cast<T*>(_thing);

nit: I think we usually use a name like 'thingArg' rather than '_thing' in this case.

Comment 4

[Paul Bone [:pbone]]

Attachment: patch

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8870174 [details] [diff] [review]
patch

Review of attachment 8870174 [details] [diff] [review]:
-----------------------------------------------------------------

r=me providing the test pass on try.

Comment 6

[Paul Bone [:pbone]]

Attachment: patch

Updated to fix try failures.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac6b111e23b
Assert that thing has correct runtime in js::TraceChildren. r=jonco

Comment 8

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

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