Closed Bug 1297244 Opened 3 years ago Closed 3 years ago

Comment limiting the input types for Heap is neither true nor backed by an assertion

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: ekleog, Assigned: ekleog)

Details

Attachments

(1 file, 1 obsolete file)

Comment at http://searchfox.org/mozilla-central/source/js/public/RootingAPI.h#216 lacks JSFunction* and nsXBLMaybeCompiled. In addition, it is not confirmed by an assertion.
Attached patch assert-heap-input-type.patch (obsolete) — Splinter Review
Here is a patch that works around the need for nsXBLMaybeCompiled by allowing it to be Heap'ed. 

I'm especially not sure about the naming for `is_heap_able_DO_NOT_SPECIALIZE` (maybe it is obvious by the fact that it's in `js::`?), as well as the whole "heap-able" name for naming things that can be put inside a JS::Heap<>; but didn't find anything better.
Attachment #8783773 - Flags: review?(terrence)
Comment on attachment 8783773 [details] [diff] [review]
assert-heap-input-type.patch

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

Ping me in IRC if you want to discuss any of this feedback.

::: js/public/RootingAPI.h
@@ +124,5 @@
>  template <typename T>
>  class HeapBase {};
>  
> +// Cannot use FOR_EACH_HEAP_ABLE_GC_POINTER_TYPE, as this would import too many macros into scope
> +template <typename T> struct is_heap_able_DO_NOT_SPECIALIZE    { static constexpr bool value = false; };

This is great. For historical reasons we use camel-case instead of underscores, so I think a better name would be IsHeapConstructableType. Leave off the DO_NOT_SPECIALIZE. If you're specializing a GC type you should already be justifiably terrified.

@@ +125,5 @@
>  class HeapBase {};
>  
> +// Cannot use FOR_EACH_HEAP_ABLE_GC_POINTER_TYPE, as this would import too many macros into scope
> +template <typename T> struct is_heap_able_DO_NOT_SPECIALIZE    { static constexpr bool value = false; };
> +template <> struct is_heap_able_DO_NOT_SPECIALIZE<JS::Value>   { static constexpr bool value = true; };

Instead of duplicating this list, please move the _PUBLIC_ types macro to js/public/GCPolicy.h. I thought it was already public for UbiNode.h, but I guess that's only TraceKind so far.

@@ +223,5 @@
>   *
>   * Heap<T> objects should only be used on the heap. GC references stored on the
>   * C/C++ stack must use Rooted/Handle/MutableHandle instead.
>   *
> + * Type T must be one of: JS::Value, jsid, JSObject*, JSString*, JSScript* or JSFunction*

Hurm, I think it might be more accurate to say that we only want this instantiated on public types. If we're inside SpiderMonkey we should use one of the inlinable barrier wrappers.

::: js/src/gc/Policy.h
@@ +72,5 @@
> +    D(jsid) \
> +    D(JSObject*) \
> +    D(JSString*) \
> +    D(JSScript*) \
> +    D(JSFunction*)

It looks like this list is not substantially different from FOR_EACH_PUBLIC_*. In fact, at least one of the members, JSString*, does appear to be used with JS::Heap. Instead of creating a new list-of-types, I think we should just apply both of the public lists and accept that we might be instantiating one more thing than strictly necessary.
Attachment #8783773 - Flags: review?(terrence)
Thanks for the review! Here is a patch with comments addressed. Is it better now?
Attachment #8783773 - Attachment is obsolete: true
Attachment #8784575 - Flags: review?(terrence)
Comment on attachment 8784575 [details] [diff] [review]
assert-heap-input-type.patch

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

Looks good!
Attachment #8784575 - Flags: review?(terrence) → review+
Thanks!
Keywords: checkin-needed
Please post a Try link for this patch.
Keywords: checkin-needed
Here it is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43083eac0d87

This being a static_assert only, I ran build tests only, which seem to pass.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96281cc98bc6
Assert on the type given as a parameter to Heap. r=terrence
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/96281cc98bc6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.