[webkit-reviews] review granted: [Bug 174958] Add URLSchemeHandler API tests that verify the lack of URLSchemeTask object leaks : [Attachment 316701] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 174958: Add URLSchemeHandler API tests that verify the lack of
URLSchemeTask object leaks
https://bugs.webkit.org/show_bug.cgi?id=174958

Attachment 316701: Patch

https://bugs.webkit.org/attachment.cgi?id=316701&action=review




--- Comment #14 from Darin Adler <darin at apple.com> ---
Comment on attachment 316701
  --> https://bugs.webkit.org/attachment.cgi?id=316701
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316701&action=review

> Source/WTF/wtf/InstanceCounted.h:2
> + * Copyright (C) 2017 Apple Inc.  All rights reserved.

Should only have one space here after the period.

> Source/WTF/wtf/InstanceCounted.h:29
> +// Since it adds runtime overhead, in both managing the count variable and
adding a vtable,

We get no benefit from adding a vtable here (see below).

>>> Source/WTF/wtf/InstanceCounted.h:44
>>> +	 virtual ~InstanceCounted()
>> 
>> I don't understand why virtual is useful here. It's kinda dangerous because
is can cause bad deletion if someone holds just the base and deletes the object
while the derived type's dtor isn't itself virtual.
> 
> I might misunderstand what you're saying, as it goes against my understanding
of virtual destructors. I thought any virtual method in a base makes any
derived method virtual, even if the derivers don't explicitly declare virtual.
And I thought this extended to destructors.
> 
> e.g.
> 
> class Base {
> public:
>     ~Base() { printf("~Base\n"); }
> };
> 
> class Derived : public Base {
> public:
>     ~Derived() { printf("~Derived\n"); }
> };
> 
> Base* b = new Derived;
> delete b;  // Only ~Base is called. ~Derived is *not*
> 
> class VBase {
> public:
>     virtual ~VBase() { printf("~VBase\n"); }
> };
> 
> class VDerived : public VBase {
> public:
>     ~VDerived() { printf("~VDerived\n"); } // This destructor is
automatically virtual
> };
> 
> VBase* b = new VDerived;
> delete b; // Both ~VDerived and ~VBase are called
> 
> ---
> 
> Just ran that code to confirm my understanding. Am I misunderstanding you, or
missing something else here?

Yes, you are correct Brady, the danger JF mentions of bad deletion is not real
because if a base class has a virtual destructor the derived classes’
destructors are automatically virtual too.

But I agree with JF that it is not helpful or necessary to make the destructor
in this class template be virtual. Instead the destructor should be
non-virtual. The destructor should also be protected rather than public.

A virtual destructor here would help us if we wanted to allow people to safely
destroy the object that contains this sub-object using an InstanceCounted<X>*;
if it’s a virtual destructor then we will properly destroy the larger object,
calling the correct destructors.

But we don’t want that. Instead we simply want people to not even try to
destroy the object using an InstanceCounted<X>*; we’d like it to be a
compilation error. A way to *almost* express that is to make the destructor
protected. If it is protected, it can be called implicitly inside derived
classes’ destructors and can’t be called by code outside derived classes. It’s
not quite right, because sadly there is no simple way to make sure it can’t be
called in other places in derived classes’ code, which we would like to forbid
if we could.

The rule of thumb for virtual-ness of destructors is generally: If the class is
designed to be used polymorphically, it will define at least one virtual
function, and then the destructor should be virtual too. The compiler even
generates a warning for that case. This class is not designed to be used
polymorphically and so it need not have any virtual functions. There are plenty
of other examples of that in WebKit.

The constructor should also be protected, for the same reason that the
destructor should be. The only public member the class template needs is the
instanceCount function; no other members are designed to be used outside the
class template, except that the derived classes need to call through to the
constructor and destructor in their constructors and destructors.

This class template also should also define protected copy and move
constructors that increment the instance count just like the default
constructor does, unless we plan to use it only for non-copyable, non-movable
classes. If we don’t define copy and move constructors at all, the compiler
will generate public ones automatically, and when it does it will generate
constructors that don’t increment the instance count. Since a move does not
destroy an object, even the move constructor should just bump the instance
count by 1.

> Source/WTF/wtf/InstanceCounted.h:53
> +    static std::atomic_size_t s_instances;

s_instances sounds like a collection of instances, not a count of instances. I
suggest calling it instanceCount, just like the function.

>>> Source/WebKit/UIProcess/API/C/WKTestingSupport.cpp:44
>>> +	 return API::URLSchemeTask::instanceCount();
>> 
>> This would be nicer if the base always declared instanceCount() and it just
returned zero in release.
> 
> That'd probably be fine.

Yes, great idea.