CSS: Make ~InvalidationSet protected

CSS: Make ~InvalidationSet protected

InvalidationSet now implements ref counting directly, in the same manner
as LayoutPart, instead of inheriting from RefCounted.

Previous attemps at making the InvalidationSet destructor protected had
failed because in one or more builds (win_chromium_compile_dbg_ng), an
error message was being reported in
'void WTF::RefCounted<blink::InvalidationSet>::deref(void) const'.

BUG=412572
Committed: https://crrev.com/4cfb70c64f105e5d8d50df215bac5a5b01d0ceb7
Cr-Commit-Position: refs/heads/master@{#414405}

[Eric Willigers, 2016-08-24 14:57:48]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 14:58:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Willigers, 2016-08-24 15:06:38]

ericwilligers@chromium.org changed reviewers:
+ haraken@chromium.org, rune@opera.com

[haraken, 2016-08-24 15:31:17]

Why do you want to implement the reference counting system manually?

[commit-bot: I haz the power, 2016-08-24 16:47:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 16:47:21]

Dry run: This issue passed the CQ dry run.

[Eric Willigers, 2016-08-24 20:23:01]

On 2016/08/24 15:31:17, haraken wrote:
> Why do you want to implement the reference counting system manually?


We have two classes, DescendantInvalidationSet and SiblingInvalidationSet, with
most of their functionality in common.
So we have a base class, InvalidationSet, which is logically abstract.

I initially implemented InvalidationSet with a virtual destructor, but the vptr
caused a memory regression when SiblingInvalidationSet was introduced.
SiblingInvalidationSet is rare, so we suspected the vptr and removed it,
successfully avoiding the memory regression.

Removing the vptr involved implementing InvalidationSet::deref(), to delete the
appropriate subclass. I would have also liked to make
the destructor of InvalidationSet protected, to ensure RefPtr<InvalidationSet>
never executes a 'delete (InvalidationSet*)'.

Unfortunately, with some compilers we see the following compile error:

wtf\refcounted.h(159): error C2248: 'blink::InvalidationSet::~InvalidationSet':
cannot access protected member declared in class 'blink::InvalidationSet'
core\css\invalidation\invalidationset.h(185): note: compiler has generated
'blink::InvalidationSet::~InvalidationSet' here
core\css\invalidation\styleinvalidator.h(19): note: see declaration of
'blink::InvalidationSet'
wtf\refcounted.h(157): note: while compiling class template member function
'void WTF::RefCounted<blink::InvalidationSet>::deref(void) const'
core\css\invalidation\invalidationset.h(81): note: see reference to class
template instantiation 'WTF::RefCounted<blink::InvalidationSet>' being compiled

At one stage I thought the problem might be that InvalidationSet::deref() isn't
const, unlike RefCounted<>::deref(), but making it const doesn't eliminate the
compile error.


So now we have at least four options: 
1. (The current situation) Allow some compilers to keep requiring 'void
WTF::RefCounted<blink::InvalidationSet>::deref(void) const' being able to call
delete with an InvalidationSet*. Keep ~InvalidationSet non-protected.

2. (This patch) Implement ref counting in InvalidationSet, just like LayoutPart,
the other blink core class that implements deref itself.

3. Replace all RefPtr<InvalidationSet> with RefPtr<DescendantInvalidationSet>
and RefPtr<SiblingInvalidationSet>.

4. Supply a specialization 

template<>
void WTF::RefCounted<blink::InvalidationSet>::deref() const
{
    if (!derefBase())
        return;
    static_cast<const blink::InvalidationSet*>(this)->destroy();
}

[Eric Willigers, 2016-08-24 20:23:24]

Description was changed from

==========
CSS: Make ~InvalidationSet protected

InvalidationSet now implements ref counting directly, in the same manner
as LayoutPart, instead of inheriting from RefCounted.

Previous attemps at making the InvalidationSet destructor protected had
failed because in one or more builds (win_chromium_compile_dbg_ng), an
error message was being reported in
'void WTF::RefCounted<blink::InvalidationSet>::deref(void) const'.

BUG=412572
==========

to

==========
CSS: Make ~InvalidationSet protected

InvalidationSet now implements ref counting directly, in the same manner
as LayoutPart, instead of inheriting from RefCounted.

Previous attemps at making the InvalidationSet destructor protected had
failed because in one or more builds (win_chromium_compile_dbg_ng), an
error message was being reported in
'void WTF::RefCounted<blink::InvalidationSet>::deref(void) const'.

BUG=412572
==========

[Eric Willigers, 2016-08-24 20:23:24]

ericwilligers@chromium.org changed reviewers:
+ esprehn@chromium.org

[haraken, 2016-08-25 01:23:47]

On 2016/08/24 20:23:01, Eric Willigers wrote:
> On 2016/08/24 15:31:17, haraken wrote:
> > Why do you want to implement the reference counting system manually?
> 
> 
> We have two classes, DescendantInvalidationSet and SiblingInvalidationSet,
with
> most of their functionality in common.
> So we have a base class, InvalidationSet, which is logically abstract.
> 
> I initially implemented InvalidationSet with a virtual destructor, but the
vptr
> caused a memory regression when SiblingInvalidationSet was introduced.
> SiblingInvalidationSet is rare, so we suspected the vptr and removed it,
> successfully avoiding the memory regression.
> 
> Removing the vptr involved implementing InvalidationSet::deref(), to delete
the
> appropriate subclass. I would have also liked to make
> the destructor of InvalidationSet protected, to ensure RefPtr<InvalidationSet>
> never executes a 'delete (InvalidationSet*)'.
> 
> Unfortunately, with some compilers we see the following compile error:
> 
> wtf\refcounted.h(159): error C2248:
'blink::InvalidationSet::~InvalidationSet':
> cannot access protected member declared in class 'blink::InvalidationSet'
> core\css\invalidation\invalidationset.h(185): note: compiler has generated
> 'blink::InvalidationSet::~InvalidationSet' here
> core\css\invalidation\styleinvalidator.h(19): note: see declaration of
> 'blink::InvalidationSet'
> wtf\refcounted.h(157): note: while compiling class template member function
> 'void WTF::RefCounted<blink::InvalidationSet>::deref(void) const'
> core\css\invalidation\invalidationset.h(81): note: see reference to class
> template instantiation 'WTF::RefCounted<blink::InvalidationSet>' being
compiled
> 
> At one stage I thought the problem might be that InvalidationSet::deref()
isn't
> const, unlike RefCounted<>::deref(), but making it const doesn't eliminate the
> compile error.
> 
> 
> So now we have at least four options: 
> 1. (The current situation) Allow some compilers to keep requiring 'void
> WTF::RefCounted<blink::InvalidationSet>::deref(void) const' being able to call
> delete with an InvalidationSet*. Keep ~InvalidationSet non-protected.
> 
> 2. (This patch) Implement ref counting in InvalidationSet, just like
LayoutPart,
> the other blink core class that implements deref itself.
> 
> 3. Replace all RefPtr<InvalidationSet> with RefPtr<DescendantInvalidationSet>
> and RefPtr<SiblingInvalidationSet>.
> 
> 4. Supply a specialization 
> 
> template<>
> void WTF::RefCounted<blink::InvalidationSet>::deref() const
> {
>     if (!derefBase())
>         return;
>     static_cast<const blink::InvalidationSet*>(this)->destroy();
> }

Thanks for the details -- makes sense. LGTM.

Let's add a brief comment and explain why InvalidationSet implements m_refCount
manually.

[Eric Willigers, 2016-08-25 04:32:38]

> Let's add a brief comment and explain why InvalidationSet implements
m_refCount
> manually.

Comment added.

[Eric Willigers, 2016-08-25 04:32:51]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 04:33:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 04:40:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 04:40:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2016-08-25 04:50:52]

On 2016/08/25 04:32:38, Eric Willigers wrote:
> > Let's add a brief comment and explain why InvalidationSet implements
> m_refCount
> > manually.
> 
> Comment added.

Thanks! LGTM.

[Eric Willigers, 2016-08-25 05:22:03]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 05:22:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 06:38:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 06:38:27]

Dry run: This issue passed the CQ dry run.

[Eric Willigers, 2016-08-25 12:23:27]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2016-08-25 12:24:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 12:28:15]

Description was changed from

==========
CSS: Make ~InvalidationSet protected

InvalidationSet now implements ref counting directly, in the same manner
as LayoutPart, instead of inheriting from RefCounted.

Previous attemps at making the InvalidationSet destructor protected had
failed because in one or more builds (win_chromium_compile_dbg_ng), an
error message was being reported in
'void WTF::RefCounted<blink::InvalidationSet>::deref(void) const'.

BUG=412572
==========

to

==========
CSS: Make ~InvalidationSet protected

InvalidationSet now implements ref counting directly, in the same manner
as LayoutPart, instead of inheriting from RefCounted.

Previous attemps at making the InvalidationSet destructor protected had
failed because in one or more builds (win_chromium_compile_dbg_ng), an
error message was being reported in
'void WTF::RefCounted<blink::InvalidationSet>::deref(void) const'.

BUG=412572
==========

[commit-bot: I haz the power, 2016-08-25 12:28:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-25 12:30:01]

Description was changed from

==========
CSS: Make ~InvalidationSet protected

InvalidationSet now implements ref counting directly, in the same manner
as LayoutPart, instead of inheriting from RefCounted.

Previous attemps at making the InvalidationSet destructor protected had
failed because in one or more builds (win_chromium_compile_dbg_ng), an
error message was being reported in
'void WTF::RefCounted<blink::InvalidationSet>::deref(void) const'.

BUG=412572
==========

to

==========
CSS: Make ~InvalidationSet protected

InvalidationSet now implements ref counting directly, in the same manner
as LayoutPart, instead of inheriting from RefCounted.

Previous attemps at making the InvalidationSet destructor protected had
failed because in one or more builds (win_chromium_compile_dbg_ng), an
error message was being reported in
'void WTF::RefCounted<blink::InvalidationSet>::deref(void) const'.

BUG=412572

Committed: https://crrev.com/4cfb70c64f105e5d8d50df215bac5a5b01d0ceb7
Cr-Commit-Position: refs/heads/master@{#414405}
==========

[commit-bot: I haz the power, 2016-08-25 12:30:03]

Patchset 2 (id:??) landed as
https://crrev.com/4cfb70c64f105e5d8d50df215bac5a5b01d0ceb7
Cr-Commit-Position: refs/heads/master@{#414405}