Closed Bug 1838703 Opened 1 year ago Closed 1 year ago

Bindings::SetAsFoo() methods that return OwningNonNull/NonNull should be [[nodiscard]]

Categories

(Core :: DOM: Bindings (WebIDL), defect, P1)

defect

Tracking

()

RESOLVED FIXED
116 Branch
Tracking Status
firefox116 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(1 file)

class OwningGPUOutOfMemoryErrorOrGPUValidationError : public AllOwningUnionBase
[...]
  union Value
  {
    UnionMember<OwningNonNull<mozilla::webgpu::OutOfMemoryError> > mGPUOutOfMemoryError;
    UnionMember<OwningNonNull<mozilla::webgpu::ValidationError> > mGPUValidationError;

  };

  TypeOrUninit mType;
  Value mValue;
OwningNonNull<mozilla::webgpu::OutOfMemoryError>&
OwningGPUOutOfMemoryErrorOrGPUValidationError::SetAsGPUOutOfMemoryError()
{
  if (mType == eGPUOutOfMemoryError) {
    return mValue.mGPUOutOfMemoryError.Value();
  }
  Uninit();
  mType = eGPUOutOfMemoryError;
  return mValue.mGPUOutOfMemoryError.SetValue();
}
class UnionMember {
  AlignedStorage2<T> mStorage;
[...]
  template <typename... Args>
  T& SetValue(Args&&... args) {
    new (mStorage.addr()) T(std::forward<Args>(args)...);
    return *mStorage.addr();
  }
template <class T>
class MOZ_IS_SMARTPTR_TO_REFCOUNTED OwningNonNull {
 public:
  using element_type = T;

  OwningNonNull() = default;

The problem is that SetValue() in-place constructs the union variant, and in this case the union variant isn't OutOfMemory (which has a deleted ctor), it's OwningNonNull<OutOfMemory>, which does have a super dangerous default constructor!

The bug in question that the fix for bug 1837557 started hitting on CI is:

        dom::OwningGPUOutOfMemoryErrorOrGPUValidationError error;
[...]
          case PopErrorScopeResultType::OutOfMemory:
            error.SetAsGPUOutOfMemoryError();
            break;

Marking as blocking webgpu-v1-cts-windows, since the scope of comment 2 on bug 1838694 has moved to this bug at :jgilbert's initiative.

Blocks: 1838739
Depends on: 1838739
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5cd8f2638cc5
[dom/bindings] Mark SetAsFoo() as nodiscard. r=peterv
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: