1291423 - 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]

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Daniel Holbert [:dholbert]]

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              ^
}

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1291423: Explicitly qualify the destructor call that we invoke in Maybe::reset.

Review commit: https://reviewboard.mozilla.org/r/68710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68710/

Comment 4

[Daniel Holbert [:dholbert]]

(Looks like Waldo is main reviewer for changes to Maybe.h; hence, tagging him for review here.)

Comment 5

[Jeff Walden [:Waldo]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

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/

Comment 7

[Daniel Holbert [:dholbert]]

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.
}

Comment 8

[Seth Fowler [:seth] [:s2h]]

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.

Comment 9

[Seth Fowler [:seth] [:s2h]]

I filed bug 1291478 about fixing the CustomAutoRooter type hierarchy.

Comment 10

[Jeff Walden [:Waldo]]

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>?

Comment 11

[Jeff Walden [:Waldo]]

(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.

Comment 12

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 13

[Daniel Holbert [:dholbert]]

(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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe87fbf9f4d
Explicitly qualify the destructor call that we invoke in Maybe::reset. r=Waldo

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dfe87fbf9f4d