Fix crash when deleting ReadingListStore via ReadingListModelStorage*.

Fix crash when deleting ReadingListStore via ReadingListModelStorage*.

ReadingListStore inherits from multiple classes, including from
ReadingListModelStorage that does not define a virtual destructor.

Since ReadingListModelImpl has a scoped_ptr to ReadingListModelStorage,
it calls the base destructor with a pointer that does not correspond
to memory returned by "new" (it points to somewhere inside the memory
block).

The issue can be fixed by defining a virtual destructor on the base
class ReadingListModelStorage (as all class with virtual method should
do). Also mark the class a non-copyable since copying would cause slicing.

BUG=673169
Committed: https://crrev.com/39493e82e29a9d895f9f6de38a076ab43f1bf17c
Cr-Commit-Position: refs/heads/master@{#437800}

[sdefresne, 2016-12-11 18:50:07]

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

[commit-bot: I haz the power, 2016-12-11 18:50:18]

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

[sdefresne, 2016-12-11 18:51:57]

sdefresne@chromium.org changed reviewers:
+ noyau@chromium.org, olivierrobin@chromium.org

[sdefresne, 2016-12-11 18:51:58]

Please take a look.

When defining class with virtual methods, please mark the destructor as virtual.
It is always correct to do (and the alternative can lead to really hard to debug
leaks or crash as we can see here).

[sdefresne, 2016-12-11 18:52:31]

Description was changed from

==========
Fix crash when deleting ReadingListStore via ReadingListModelStorage*.

ReadingListStore inherits from multiple classes, including from
ReadingListModelStorage that does not define a virtual destructor.

Since ReadingListModelImpl has a scoped_ptr to ReadingListModelStorage,
it calls the base destructor with a pointer that does not correspond
to memory returned by "new" (it points to somewhere inside the memory
block).

The issue can be fixed by defining a virtual destructor on the base
class ReadingListModelStorage (as all class with virtual method should
do).

Also mark the class a non-copyable since copying would cause slicing
and even more incorrect code.

BUG=673169
==========

to

==========
Fix crash when deleting ReadingListStore via ReadingListModelStorage*.

ReadingListStore inherits from multiple classes, including from
ReadingListModelStorage that does not define a virtual destructor.

Since ReadingListModelImpl has a scoped_ptr to ReadingListModelStorage,
it calls the base destructor with a pointer that does not correspond
to memory returned by "new" (it points to somewhere inside the memory
block).

The issue can be fixed by defining a virtual destructor on the base
class ReadingListModelStorage (as all class with virtual method should
do). Also mark the class a non-copyable since copying would cause slicing.

BUG=673169
==========

[commit-bot: I haz the power, 2016-12-11 18:59:31]

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

[commit-bot: I haz the power, 2016-12-11 18:59:32]

Dry run: This issue passed the CQ dry run.

[Olivier, 2016-12-11 19:06:47]

LGTM

But now ReadingListStore inherits two classes (ReadingListModelStorage and
syncer::ModelTypeSyncBridge)
I thought this was only allowed if one was pure virtual.
Shouldn't the fix be to avoid having unique_ptr<ReadingListModelStorage> ?

[Olivier, 2016-12-11 19:11:04]

On 2016/12/11 19:06:47, Olivier Robin wrote:
> LGTM
> 
> But now ReadingListStore inherits two classes (ReadingListModelStorage and
> syncer::ModelTypeSyncBridge)
> I thought this was only allowed if one was pure virtual.
> Shouldn't the fix be to avoid having unique_ptr<ReadingListModelStorage> ?

Example, should this be done for ReadingListStoreDelegate too?

[sdefresne, 2016-12-11 19:28:20]

On 2016/12/11 19:11:04, Olivier Robin wrote:
> On 2016/12/11 19:06:47, Olivier Robin wrote:
> > LGTM
> > 
> > But now ReadingListStore inherits two classes (ReadingListModelStorage and
> > syncer::ModelTypeSyncBridge)
> > I thought this was only allowed if one was pure virtual.
> > Shouldn't the fix be to avoid having unique_ptr<ReadingListModelStorage> ?
> 
> Example, should this be done for ReadingListStoreDelegate too?

If you want to delete an instance of a derived class via a base class
destructor, you need to have the base class destructor virtual (this is also
pertinent if the destruction occurs via a std::unique_ptr). If you don't, the
behaviour is undefined (usually, call the base destructor with a pointer without
correction for the base-to-derived offset that can exists in case of multiple
inheritance). If a class inherit from multiple classes, any of them may have a
virtual destructor, but deletion is only valid if done via a pointer to the most
derived class or to any parent class that define it's destructor as virtual.

Regarding ReadingListStoreDelegate, yes it also needs a virtual destructor, this
is why I said that the whole reading list code base needed an audit :-/

[Olivier, 2016-12-11 19:30:39]

On 2016/12/11 19:28:20, sdefresne wrote:
> On 2016/12/11 19:11:04, Olivier Robin wrote:
> > On 2016/12/11 19:06:47, Olivier Robin wrote:
> > > LGTM
> > > 
> > > But now ReadingListStore inherits two classes (ReadingListModelStorage and
> > > syncer::ModelTypeSyncBridge)
> > > I thought this was only allowed if one was pure virtual.
> > > Shouldn't the fix be to avoid having unique_ptr<ReadingListModelStorage> ?
> > 
> > Example, should this be done for ReadingListStoreDelegate too?
> 
> If you want to delete an instance of a derived class via a base class
> destructor, you need to have the base class destructor virtual (this is also
> pertinent if the destruction occurs via a std::unique_ptr). If you don't, the
> behaviour is undefined (usually, call the base destructor with a pointer
without
> correction for the base-to-derived offset that can exists in case of multiple
> inheritance). If a class inherit from multiple classes, any of them may have a
> virtual destructor, but deletion is only valid if done via a pointer to the
most
> derived class or to any parent class that define it's destructor as virtual.
> 
> Regarding ReadingListStoreDelegate, yes it also needs a virtual destructor,
this
> is why I said that the whole reading list code base needed an audit :-/

I did a CL for ReadingListStoreDelegate
https://codereview.chromium.org/2565213002/#

[Olivier, 2016-12-11 19:40:03]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-12-11 19:40:14]

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-12-11 19:43:36]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1481485203701330, "parent_rev":
"c5eba27a87a9d9d047776c73eb03816c284b0858", "commit_rev":
"af194f64e904f07a766dae36e3e5510fa75dc84f"}

[commit-bot: I haz the power, 2016-12-11 19:44:03]

Description was changed from

==========
Fix crash when deleting ReadingListStore via ReadingListModelStorage*.

ReadingListStore inherits from multiple classes, including from
ReadingListModelStorage that does not define a virtual destructor.

Since ReadingListModelImpl has a scoped_ptr to ReadingListModelStorage,
it calls the base destructor with a pointer that does not correspond
to memory returned by "new" (it points to somewhere inside the memory
block).

The issue can be fixed by defining a virtual destructor on the base
class ReadingListModelStorage (as all class with virtual method should
do). Also mark the class a non-copyable since copying would cause slicing.

BUG=673169
==========

to

==========
Fix crash when deleting ReadingListStore via ReadingListModelStorage*.

ReadingListStore inherits from multiple classes, including from
ReadingListModelStorage that does not define a virtual destructor.

Since ReadingListModelImpl has a scoped_ptr to ReadingListModelStorage,
it calls the base destructor with a pointer that does not correspond
to memory returned by "new" (it points to somewhere inside the memory
block).

The issue can be fixed by defining a virtual destructor on the base
class ReadingListModelStorage (as all class with virtual method should
do). Also mark the class a non-copyable since copying would cause slicing.

BUG=673169

Review-Url: https://codereview.chromium.org/2563303002
==========

[commit-bot: I haz the power, 2016-12-11 19:44:04]

Committed patchset #1 (id:1)

[Nico, 2016-12-11 19:52:04]

On 2016/12/11 19:44:04, commit-bot: I haz the power wrote:
> Committed patchset #1 (id:1)

Fwiw "The issue can be fixed by defining a virtual destructor on the base
class ReadingListModelStorage (as all class with virtual method should
do)." isn't quite right: either that, or make the dtor protected.

[sdefresne, 2016-12-11 23:23:47]

On 2016/12/11 19:52:04, Nico wrote:
> On 2016/12/11 19:44:04, commit-bot: I haz the power wrote:
> > Committed patchset #1 (id:1)
> 
> Fwiw "The issue can be fixed by defining a virtual destructor on the base
> class ReadingListModelStorage (as all class with virtual method should
> do)." isn't quite right: either that, or make the dtor protected.

Agreed, if dtor is private/protected then it does not have to be virtual (though
it is still correct to declare it virtual then).

[commit-bot: I haz the power, 2016-12-12 15:09:58]

Description was changed from

==========
Fix crash when deleting ReadingListStore via ReadingListModelStorage*.

ReadingListStore inherits from multiple classes, including from
ReadingListModelStorage that does not define a virtual destructor.

Since ReadingListModelImpl has a scoped_ptr to ReadingListModelStorage,
it calls the base destructor with a pointer that does not correspond
to memory returned by "new" (it points to somewhere inside the memory
block).

The issue can be fixed by defining a virtual destructor on the base
class ReadingListModelStorage (as all class with virtual method should
do). Also mark the class a non-copyable since copying would cause slicing.

BUG=673169

Review-Url: https://codereview.chromium.org/2563303002
==========

to

==========
Fix crash when deleting ReadingListStore via ReadingListModelStorage*.

ReadingListStore inherits from multiple classes, including from
ReadingListModelStorage that does not define a virtual destructor.

Since ReadingListModelImpl has a scoped_ptr to ReadingListModelStorage,
it calls the base destructor with a pointer that does not correspond
to memory returned by "new" (it points to somewhere inside the memory
block).

The issue can be fixed by defining a virtual destructor on the base
class ReadingListModelStorage (as all class with virtual method should
do). Also mark the class a non-copyable since copying would cause slicing.

BUG=673169

Committed: https://crrev.com/39493e82e29a9d895f9f6de38a076ab43f1bf17c
Cr-Commit-Position: refs/heads/master@{#437800}
==========

[commit-bot: I haz the power, 2016-12-12 15:09:58]

Patchset 1 (id:??) landed as
https://crrev.com/39493e82e29a9d895f9f6de38a076ab43f1bf17c
Cr-Commit-Position: refs/heads/master@{#437800}