1500064 - IdSet in enumeration code needs to be rooted

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P1)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Steve Fink [:sfink] [:s:]]

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[8] 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.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1500064 - Use GCHashSet in property enumeration code. r?jonco

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.

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7141a7da1020

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/7141a7da1020

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Is this something we're going to want to backport to ESR60 during the next cycle for 60.4?

Comment 7

[Jan de Mooij [:jandem]]

(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.