Property setter and definer should not return "non-hole" values

[mmastrac]

The indexed and named property setters and definers should only allow setting "the hole" as a return value -- not any other type. We don't currently check this in any way, allowing embedders to footgun themselves pretty easily.

On the C++ side, PropertyCallbackInfo<void> only accepts a single kind of return value: an empty handle. This is different from calling SetUndefined(), which sets undefined rather than TheHole!

Our tests currently call set_undefined on the ReturnValue which is incorrect. We should either add a set_empty to ReturnValue, and potentially should consider adding a more strongly-typed return value interface.

Deleters and property callbacks are boolean and integers as well, but those are probably easier for embedders to get right.

  // Setting the hole value has different meanings depending on the usage:
  //  - for function template callbacks it means that the callback returns
  //    the undefined value,
  //  - for property getter callbacks is means that the callback returns
  //    the undefined value (for property setter callbacks the value returned
  //    is ignored),
  //  - for interceptor callbacks it means that the request was not handled.
  V8_INLINE void SetTheHole();

using IndexedPropertyDefinerCallbackV2 =
    Intercepted (*)(uint32_t index, const PropertyDescriptor& desc,
                    const PropertyCallbackInfo<void>& info);

template <typename T>
template <typename S>
void ReturnValue<T>::Set(const Local<S> handle) {
  static_assert(std::is_void<T>::value || std::is_base_of<T, S>::value,
                "type check");
  if (V8_UNLIKELY(handle.IsEmpty())) {
    SetTheHole();
  } else {
    SetInternal(handle.ptr());
  }
}

template <typename T>
void ReturnValue<T>::SetTheHole() {
  using I = internal::Internals;
#if V8_STATIC_ROOTS_BOOL
  SetInternal(I::StaticReadOnlyRoot::kTheHoleValue);
#else
  *value_ = I::GetRoot(GetIsolate(), I::kTheHoleValueRootIndex);
#endif  // V8_STATIC_ROOTS_BOOL
}

[bartlomieju]

This can be done by not providing ReturnValue argument to the relevant callbacks. I'll work on that.

[mmastrac]

After further research, it looks like the return value is ignored for these with the new APIs.

// TODO(ishell): just return v8::Intercepted.
Handle<Object> PropertyCallbackArguments::CallIndexedSetter(
    Handle<InterceptorInfo> interceptor, uint32_t index, Handle<Object> value) {
  DCHECK(!interceptor->is_named());
  Isolate* isolate = this->isolate();
  RCS_SCOPE(isolate, RuntimeCallCounterId::kIndexedSetterCallback);
  if (interceptor->has_new_callbacks_signature()) {
    // New Api relies on the return value to be set to undefined.
    // TODO(ishell): do this in the constructor once the old Api is deprecated.
    slot_at(kReturnValueIndex).store(ReadOnlyRoots(isolate).undefined_value());
    IndexedPropertySetterCallbackV2 f =
        ToCData<IndexedPropertySetterCallbackV2>(interceptor->setter());
    Handle<InterceptorInfo> has_side_effects;
    PREPARE_CALLBACK_INFO_INTERCEPTOR(isolate, f, void, has_side_effects);
    auto intercepted = f(index, v8::Utils::ToLocal(value), callback_info);
    if (intercepted == v8::Intercepted::kNo) return {};
    // Non-empty handle indicates that the request was intercepted.
    return isolate->factory()->undefined_value();

  } else {
    IndexedPropertySetterCallback f =
        ToCData<IndexedPropertySetterCallback>(interceptor->setter());
    Handle<InterceptorInfo> has_side_effects;
    PREPARE_CALLBACK_INFO_INTERCEPTOR(isolate, f, v8::Value, has_side_effects);
    f(index, v8::Utils::ToLocal(value), callback_info);
    return GetReturnValue<Object>(isolate);
  }
}