Closed Bug 1500064 Opened 2 years ago Closed 2 years ago
Set in enumeration code needs to be rooted
46 bytes, text/x-phabricator-request
|Details | Review|
This is a Maybe<IdSet> and IdSet is a HashSet<jsid, ...> Because (1) we can call arbitrary code while this set is on the stack (via scripted proxy traps, so content can trigger a GC etc) and (2) not all ids in the set are guaranteed to exist elsewhere (the |props| Vector contains a subset), this seems to be a rooting hazard. The simplest option is probably to make this a GCHashSet. A bit unfortunate for me because I wanted to make this an InlineSet but oh well. We could add GCInlineSet later maybe.
I'm not sure how s-s this is because we only use this set to check whether we've already seen a particular id. OTOH, it might be possible to leak info like "is any origin using atom x?" or something.
Note that bug 1321014 will fix the part about the hazard analysis not seeing through the Maybe<> to report this problem. I think it may require further annotation to get it to complain about the HashSet, though, since HashSet contains a table of type js::detail::HashTableEntry<const jsid>*. That won't be treated as a GC pointer type because (1) it's a pointer to a GC pointer, and (2) HashTableEntry contains untyped data (uint8 in this case). But I should be able to use MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS or something like it to see all the way through to the hazard. I filed bug 1500247.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
This changes Maybe<IdSet> to Rooted<IdSet>. I think the Maybe<> is not buying us much because HashTable's storage is now lazily allocated (bug 1481998). I didn't notice any regressions in micro-benchmarks; this patch might be a small improvement actually.
Is this something we're going to want to backport to ESR60 during the next cycle for 60.4?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #6) > Is this something we're going to want to backport to ESR60 during the next > cycle for 60.4? Probably not because (1) I still think the sec-moderate rating is correct and (2) the patch depends on some HashSet changes that are not in ESR60 (bug 1481998) and so the ESR60 patch would either have to be rewritten or regress perf.
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main64+]
You need to log in before you can comment on or make changes to this bug.