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

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript: GC
RESOLVED FIXED
11 months ago
8 months ago

People

(Reporter: jonco, Assigned: pbone)

Tracking

({good-first-bug})

unspecified
mozilla55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 months ago
This would help prevent bugs like bug 1337414.
(Reporter)

Comment 1

8 months ago
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.
Keywords: good-first-bug
(Assignee)

Updated

8 months ago
Assignee: nobody → pbone
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 months ago
Created attachment 8870067 [details] [diff] [review]
patch
Attachment #8870067 - Flags: review?(jcoppeard)
(Reporter)

Comment 3

8 months ago
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.
Attachment #8870067 - Flags: review?(jcoppeard)
(Assignee)

Comment 4

8 months ago
Created attachment 8870174 [details] [diff] [review]
patch
Attachment #8870067 - Attachment is obsolete: true
Attachment #8870174 - Flags: review?(jcoppeard)
(Reporter)

Comment 5

8 months ago
Comment on attachment 8870174 [details] [diff] [review]
patch

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

r=me providing the test pass on try.
Attachment #8870174 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 6

8 months ago
Created attachment 8870576 [details] [diff] [review]
patch

Updated to fix try failures.
Attachment #8870174 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Keywords: checkin-needed

Comment 7

8 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac6b111e23b
Assert that thing has correct runtime in js::TraceChildren. r=jonco
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fac6b111e23b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.