Closed
Bug 1341752
Opened 7 years ago
Closed 7 years ago
js::TraceChildren should assert that it's not called with a GC thing from another runtime
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jonco, Assigned: pbone)
Details
(Keywords: good-first-bug)
Attachments
(1 file, 2 obsolete files)
3.07 KB,
patch
|
Details | Diff | Splinter Review |
This would help prevent bugs like bug 1337414.
Reporter | ||
Comment 1•7 years 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•7 years ago
|
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8870067 -
Flags: review?(jcoppeard)
Reporter | ||
Comment 3•7 years 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•7 years ago
|
||
Attachment #8870067 -
Attachment is obsolete: true
Attachment #8870174 -
Flags: review?(jcoppeard)
Reporter | ||
Comment 5•7 years 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•7 years ago
|
||
Updated to fix try failures.
Attachment #8870174 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
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
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fac6b111e23b
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•