1478533 - Throw compile-time error when an object is used as native-WeakMap key while the object's TraceKind is not added to CC graph

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[Tooru Fujisawa [:arai]]

separated from bug 1434305 comment #73

native-WeakMap's key should be representible in Cycle Collector's graph:

https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/xpcom/base/CycleCollectedJSRuntime.h#413-420
> // Returns true if the JS::TraceKind is one the cycle collector cares about.
> inline bool AddToCCKind(JS::TraceKind aKind)
> {
>   return aKind == JS::TraceKind::Object ||
>          aKind == JS::TraceKind::Script ||
>          aKind == JS::TraceKind::Scope ||
>          aKind == JS::TraceKind::RegExpShared;
> }

https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/xpcom/base/CycleCollectedJSRuntime.cpp#198-202
> void
> NoteWeakMapsTracer::trace(JSObject* aMap, JS::GCCellPtr aKey,
>                           JS::GCCellPtr aValue)
> {
> ...
>   // The cycle collector can only properly reason about weak maps if it can
>   // reason about the liveness of their keys, which in turn requires that
>   // the key can be represented in the cycle collector graph.  All existing
>   // uses of weak maps use either objects or scripts as keys, which are okay.
>   MOZ_ASSERT(AddToCCKind(aKey.kind()));

Currently it detects the case when running the function above.

it's better somehow detect such case at compile time, so that it won't be overlooked.

Comment 1

[Jon Coppeard (:jonco)]

Thanks for filing this arai.  Maybe we could add a static assert in WeakMap based in the Key template argument.  CC'ing sfink.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP patch

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 9024688 [details] [diff] [review]
WIP patch

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

As commented on IRC, please move the function to jsfriendapi.h with the other functions called by the CC.

Otherwise, looks great.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 9024688 [details] [diff] [review]
WIP patch

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

::: js/src/gc/WeakMap-inl.h
@@ +28,5 @@
>  template <class K, class V>
>  WeakMap<K, V>::WeakMap(JSContext* cx, JSObject* memOf)
>    : Base(cx->zone()), WeakMapBase(memOf, cx->zone())
>  {
> +    using ElemType = typename Entry::KeyType::ElementType;

Is Entry::KeyType::ElementType not just K here?

@@ +31,5 @@
>  {
> +    using ElemType = typename Entry::KeyType::ElementType;
> +    using NonPtrType = typename mozilla::RemovePointer<ElemType>::Type;
> +    static_assert(AddToCCKind(NonPtrType::TraceKind),
> +                  "The object's TraceKind needs to be added in AddToCCKind.");

The message should explain the problem, or reference the comments that explain it.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jon Coppeard (:jonco) from comment #4)
> ::: js/src/gc/WeakMap-inl.h
> @@ +28,5 @@
> >  template <class K, class V>
> >  WeakMap<K, V>::WeakMap(JSContext* cx, JSObject* memOf)
> >    : Base(cx->zone()), WeakMapBase(memOf, cx->zone())
> >  {
> > +    using ElemType = typename Entry::KeyType::ElementType;
> 
> Is Entry::KeyType::ElementType not just K here?
> 
K is HeapPtr<T>, whereas T is one of JSObject*, JSScript* or LazyScript*
https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/js/src/vm/Debugger.h#130

and HeapPtr<T>::ElementType get JSObject*, JSScript* or LazyScript*
https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/js/src/gc/Barrier.h#385

Yes, I could use K to replace Entry::KeyType, but not Entry::KeyType::ElementType

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jon Coppeard (:jonco) from comment #3)
> As commented on IRC, please move the function to jsfriendapi.h with the
> other functions called by the CC.
> 
Have a question regarding this.
So the definition of AddToCCKind is moved to jsfriendapi.h as well?
Because if so, then WeakMap-inl.h will need to include jsfriendapi.h, and this header is huge, not sure if this is fine though.

Thanks

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Yoshi Huang [:allstars.chh] from comment #6)
Updating this with what we said in the meeting today: Yes, thanks for spotting this, let's not include jsfriendapi.h anywhere we can avoid it.  public/TraceKind.h would probably be better.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

Comment 9

[Jon Coppeard (:jonco)]

Comment on attachment 9025316 [details] [diff] [review]
Patch

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

r=me with the changes below.  We'll also need additional review for the changes to xpcom.

::: js/public/TraceKind.h
@@ +73,5 @@
> +// in cycle collector's graph, otherwise the key is considered to be pointed
> +// from somewhere unknown, and results in leaking the subgraph which contains
> +// the key.
> +// See the comments in NoteWeakMapsTracer::trace for more details.
> +inline constexpr bool AddToCCKind(JS::TraceKind aKind)

Can you rename this please?  'IsCCTraceKind' would be clearer I think.

::: js/src/gc/WeakMap-inl.h
@@ +30,5 @@
>    : Base(cx->zone()), WeakMapBase(memOf, cx->zone())
>  {
> +    using ElemType = typename K::ElementType;
> +    using NonPtrType = typename mozilla::RemovePointer<ElemType>::Type;
> +    // The object's TraceKind needs to be added to CC graph if this object  is

nit: there's a double space in "object  is"

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #9)
> We'll also need additional review for the changes to xpcom.

When you've updated the patch, please flag :mccr8 for review.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v2

Addressed Comment 9.

Hi mccr8,
I've renamed the AddToCCKind to IsCCTraceKind and moved it from xpcom/base/CycleCollectedJSRuntime.h to js/public/TraceKind.h

Could you review this change from xpcom's perspective?

Thanks

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 9025342 [details] [diff] [review]
Patch. v2

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

r=me for the XPCOM changes. Thanks for the cleanup.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4dd037c4ce
Add a static_assert for WeakMap. r=jonco, mccr8

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/3d4dd037c4ce