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

RESOLVED FIXED in Firefox 65

Status

()

defect
P3
minor
RESOLVED FIXED
Last year
8 months ago

People

(Reporter: arai, Assigned: allstars.chh)

Tracking

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Depends on: 1434305
Thanks for filing this arai.  Maybe we could add a static assert in WeakMap based in the Key template argument.  CC'ing sfink.
Component: JavaScript Engine → JavaScript: GC
Assignee: nobody → allstars.chh
Status: NEW → ASSIGNED
Posted patch WIP patch (obsolete) — Splinter Review
Attachment #9024688 - Flags: feedback?(jcoppeard)
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.
Attachment #9024688 - Flags: feedback?(jcoppeard) → feedback+
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.
(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
(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
(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.
Posted patch Patch (obsolete) — Splinter Review
Attachment #9024688 - Attachment is obsolete: true
Attachment #9025316 - Flags: review?(jcoppeard)
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"
Attachment #9025316 - Flags: review?(jcoppeard) → review+
(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.
Posted patch Patch. v2Splinter Review
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
Attachment #9025316 - Attachment is obsolete: true
Attachment #9025342 - Flags: review?(continuation)
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.
Attachment #9025342 - Flags: review?(continuation) → review+
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4dd037c4ce
Add a static_assert for WeakMap. r=jonco, mccr8
https://hg.mozilla.org/mozilla-central/rev/3d4dd037c4ce
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.