Closed Bug 1291423 Opened 3 years ago Closed 3 years ago

Maybe.h:377:7: error: destructor called on non-final 'mozilla::dom::TypedArrayRooter<...>' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

I'm trying to build with clang 3.9 (on Linux64), and I got this build error (from a build-warning being promoted to an error due to --enable-warnings-as-errors in my mozconfig):

{
 0:26.43 In file included from /scratch/work/builds/mozilla-inbound/obj/dom/bindings/TestExampleGenBinding.cpp:3:
 0:26.43 In file included from /scratch/work/builds/mozilla-inbound/mozilla/dom/bindings/test/TestBindingHeader.h:11:
 0:26.43 In file included from /scratch/work/builds/mozilla-inbound/obj/dist/include/mozilla/dom/BindingUtils.h:10:
 0:26.43 In file included from /scratch/work/builds/mozilla-inbound/obj/dist/include/jsfriendapi.h:12:
 0:26.44 /scratch/work/builds/mozilla-inbound/obj/dist/include/mozilla/Maybe.h:377:7: error: destructor called on non-final 'mozilla::dom::TypedArrayRooter<mozilla::dom::TypedArray<unsigned char, &js::UnwrapArrayBuffer, &JS_GetArrayBufferData, &js::GetArrayBufferLengthAndData, &JS_NewArrayBuffer> >' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
 0:26.44       ref().~T();
 0:26.44       ^
 0:26.44 /scratch/work/builds/mozilla-inbound/obj/dist/include/mozilla/Maybe.h:92:14: note: in instantiation of member function 'mozilla::Maybe<mozilla::dom::TypedArrayRooter<mozilla::dom::TypedArray<unsigned char, &js::UnwrapArrayBuffer, &JS_GetArrayBufferData, &js::GetArrayBufferLengthAndData, &JS_NewArrayBuffer> > >::reset' requested here
 0:26.44   ~Maybe() { reset(); }
 0:26.44              ^
 0:26.44 /scratch/work/builds/mozilla-inbound/obj/dom/bindings/TestExampleGenBinding.cpp:12241:40: note: in instantiation of member function 'mozilla::Maybe<mozilla::dom::TypedArrayRooter<mozilla::dom::TypedArray<unsigned char, &js::UnwrapArrayBuffer, &JS_GetArrayBufferData, &js::GetArrayBufferLengthAndData, &JS_NewArrayBuffer> > >::~Maybe' requested here
 0:26.44   Maybe<TypedArrayRooter<ArrayBuffer>> arg0_holder;
 0:26.44                                        ^
 0:26.44 /scratch/work/builds/mozilla-inbound/obj/dist/include/mozilla/Maybe.h:377:14: note: qualify call to silence this warning
 0:26.44       ref().~T();
 0:26.44              ^
}
This is inside of the generated code for this function in TestExampleGen.webidl:
> void passOptionalArrayBuffer(optional ArrayBuffer arg);
https://hg.mozilla.org/mozilla-central/annotate/ffac2798999c5b84f1b4605a1280994bb665a406/dom/bindings/test/TestExampleGen.webidl#l311

The generated code itself is:
> static bool
> passOptionalArrayBuffer(JSContext* cx, [...SNIP...])
> {
>   Optional<ArrayBuffer> arg0;
>   Maybe<TypedArrayRooter<ArrayBuffer>> arg0_holder;
>   if (args.hasDefined(0)) {
>     arg0.Construct();
>     arg0_holder.emplace(cx, &arg0.Value());
https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dom/bindings/TestExampleGenBinding.cpp#12237
So there are two hints in the build warning for how one could go about fixing this:

(1) "non-final" -- if we defined TypedArrayRooter as "final", then the issue would go away (because it'd be impossible for there to be subclasses).  Unfortunately, that won't work -- TypedArrayRooter does have various subclasses.  So, it can't be final.

(2) "note: qualify call to silence this warning" -- we can just invoke ref().T::~T() instead of ref().~T(), to be explicit about the destructor we're intending to invoke.  This *does* work.

I've got a patch locally that does this.

The question is, is this safe? I believe it is, because we know that our object here must really have type T (not some subclass of T), because it must've been constructed via Maybe::emplace, which invokes T's constructor (rather than the constructor of some T subclass).  So, it's fine for us to assume that the concrete type that's stored in our Maybe<> really does have type T, and we can explicitly use for T::~T() to clean it up.
Assignee: nobody → dholbert
Component: DOM → MFBT
(Looks like Waldo is main reviewer for changes to Maybe.h; hence, tagging him for review here.)
Comment on attachment 8777103 [details]
Bug 1291423: Explicitly qualify the destructor call that we invoke in Maybe::reset.

https://reviewboard.mozilla.org/r/68710/#review65744

Yeah, T::~T() makes sense here.  I think I might have hacked around this in JS engine code recently by adding finals to a few classes (that actually could accept it) -- nice that *someone's* actually giving enough scrutiny to recognize that Maybe should be fixed for the issue.
Attachment #8777103 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8777103 [details]
Bug 1291423: Explicitly qualify the destructor call that we invoke in Maybe::reset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68710/diff/1-2/
Thanks! I've now added a test, too, which directly triggers this build error (using clang 3.9).  Could you take a quick look at that and let me know if it looks OK?

Link directly to the test: https://reviewboard.mozilla.org/r/68710/diff/2#1

(FWIW, without the fix, TestMaybe.cpp will now fail to compile (i.e. the test will fail) with this build error:
{
$SRC/mfbt/tests/TestMaybe.cpp:11:
$OBJ/dist/include/mozilla/Maybe.h:377:7: error: destructor called on non-final 'MySuperClass' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
      ref().~T();
      ^
$SRC/mfbt/tests/TestMaybe.cpp:754:9: note: in instantiation of member function 'mozilla::Maybe<MySuperClass>::reset' requested here
  super.reset();
        ^
$OBJ/dist/include/mozilla/Maybe.h:377:14: note: qualify call to silence this warning
      ref().~T();
             ^
             MySuperClass::
1 error generated.
}
Flags: needinfo?(jwalden+bmo)
We should probably fix the underlying problem here as well, because it doesn't look too hard offhand, though I haven't looked at all the subclasses of CustomAutoRooter. We should definitely mark CustomAutoRooter's destructor protected and we can mark TypedArrayRooter as final if we replace cases of private inheritance with containment. I'll file a followup.
I filed bug 1291478 about fixing the CustomAutoRooter type hierarchy.
https://reviewboard.mozilla.org/r/68710/#review65794

::: mfbt/tests/TestMaybe.cpp:756
(Diff revision 2)
> +TestVirtualFunction() {
> +  Maybe<MySuperClass> super;
> +  super.emplace();
> +  super.reset();
> +
> +  Maybe<MySuperClass> derived;

Uh, Maybe<MyDerivedClass>?
Flags: needinfo?(jwalden+bmo)
(In reply to Seth Fowler [:seth] [:s2h] from comment #8)
> We should probably fix the underlying problem here as well, because it
> doesn't look too hard offhand, though I haven't looked at all the subclasses
> of CustomAutoRooter.

Verging on pedantry, but I would not call the underlying situation a "problem".  At least, not for the reason of this particular instance of this warning.

Maybe<T> is unusual in that it's effectively an elaborate form of putting a T on the stack.  And because it's putting a literal T on the stack, destructing that T virtually *will* always call ~T() and *would not* call some subclass destructor and *does not* need to clean up anything that would require destruction in a subclass.

So semantics consistent with an actual stacked, local variable, mean Maybe *should* call exactly ~T().

I don't have any problems with making more stuff final, or switching to encapsulation from inheritance.  But that change isn't necessary if Maybe<T> takes the correct steps to destroy its T.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> Verging on pedantry, but I would not call the underlying situation a
> "problem".  At least, not for the reason of this particular instance of this
> warning.

We're talking about different problems. =p The problem I'm referring to is that subclassing a class without a virtual destructor is potentially unsafe if you don't take explicit steps to stop people from messing it up. But you knew that. =)

Obviously it's preferable that Maybe destroy its contained instance with the same behavior that a real stack value would have. I'm not arguing against the fix in this bug, I'm just saying that if it's easy to fix up the CustomAutoRooter stuff, we should do it. (Especially since it looks to me like it's part of the public JS API.) If it's too hard, then probably the ROI isn't there.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> > +  Maybe<MySuperClass> derived;
> 
> Uh, Maybe<MyDerivedClass>?

Thanks! I'll fix that & land.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe87fbf9f4d
Explicitly qualify the destructor call that we invoke in Maybe::reset. r=Waldo
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/dfe87fbf9f4d
Status: NEW → 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.