Replace 'operator*' with explicit 'get' method on SmartPointer

Replace 'operator*' with explicit 'get' method on SmartPointer

Made operator* return reference to the raw type, not pointer. New method 'get()' should be used when raw pointer is needed.

Also removed useless inline modifier from the SmaprtPointer methods and added const modifier to the methods that don't change smart pointer.

Made ~SmartPointerBase protected to avoid accidental calls of the non-virtual base class's destructor.

drive-by: fixed use after free in src/factory.cc

BUG=None
LOG=N
R=alph@chromium.org, svenpanne@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=18275

[alph, 2013-12-03 14:15:03]

lgtm

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h
File src/smart-pointers.h (right):

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h#newcode87
src/smart-pointers.h:87: ASSERT(p_ == NULL || p_ != new_value);
maybe: if (p_ == new_value) return;

[yurys, 2013-12-03 14:28:19]

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h
File src/smart-pointers.h (right):

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h#newcode87
src/smart-pointers.h:87: ASSERT(p_ == NULL || p_ != new_value);
On 2013/12/03 14:15:03, alph wrote:
> maybe: if (p_ == new_value) return;

This should never happen in practice. In fact support for self reset have been
removed from scoped_ptr in Chromium as it caused some tricky errors (see
http://crbug.com/162971).

[svenpanne, 2013-12-03 16:17:55]

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h
File src/smart-pointers.h (right):

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h#newcode54
src/smart-pointers.h:54: ~SmartPointerBase() { if (p_) Deallocator::Delete(p_);
}
Hmmm, this has been like that before, but why is it safe to have a non-virtual
destructor in a base class here? As it is, nothing prevents an incorrect usage
AFAICT...

[yurys, 2013-12-04 07:34:01]

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h
File src/smart-pointers.h (right):

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h#newcode54
src/smart-pointers.h:54: ~SmartPointerBase() { if (p_) Deallocator::Delete(p_);
}
On 2013/12/03 16:17:55, svenpanne wrote:
> Hmmm, this has been like that before, but why is it safe to have a non-virtual
> destructor in a base class here? As it is, nothing prevents an incorrect usage
> AFAICT...

This is a good question. I believe the problem is that SmartArrayPointer and
SmartPointer inherit from SmartPointerBase. This could be solved e.g. by making
SmartPointer a template with single parameter T. Then it may use Deallocator as
type traits internally, Deallocator in turn can be specialized for array types.
This way we could use SmartPointer for both array types and scalar types.

It may make sense to replace current implementation of SmartPointer with one
from Blink or Chromium where all such problems have already been solved. WDYT?

[Sven Panne, 2013-12-04 07:40:56]

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h
File src/smart-pointers.h (right):

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h#newcode54
src/smart-pointers.h:54: ~SmartPointerBase() { if (p_) Deallocator::Delete(p_);
}
On 2013/12/04 07:34:01, yurys wrote:
> This is a good question. I believe the problem is that SmartArrayPointer and
> SmartPointer inherit from SmartPointerBase. This could be solved e.g. by
making
> SmartPointer a template with single parameter T. Then it may use Deallocator
as
> type traits internally, Deallocator in turn can be specialized for array
types.
> This way we could use SmartPointer for both array types and scalar types.
> 
> It may make sense to replace current implementation of SmartPointer with one
> from Blink or Chromium where all such problems have already been solved. WDYT?

I think this class is actually trying to be a (poor & buggy) replacement for
std::unique_ptr, perhaps we can use that instead. If there is some portability
problem, we could at least try to mimic unique_ptr's interface, so we can switch
easily in the future.

I am not sure what Chrome provides and if there are any portability problems,
but I think Jochen knows (adding him)...

[alph, 2013-12-04 07:44:47]

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h
File src/smart-pointers.h (right):

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h#newcode54
src/smart-pointers.h:54: ~SmartPointerBase() { if (p_) Deallocator::Delete(p_);
}
On 2013/12/04 07:34:01, yurys wrote:
> On 2013/12/03 16:17:55, svenpanne wrote:
> > Hmmm, this has been like that before, but why is it safe to have a
non-virtual
> > destructor in a base class here? As it is, nothing prevents an incorrect
usage
> > AFAICT...
> 
> This is a good question. I believe the problem is that SmartArrayPointer and
> SmartPointer inherit from SmartPointerBase. This could be solved e.g. by
making
> SmartPointer a template with single parameter T. Then it may use Deallocator
as
> type traits internally, Deallocator in turn can be specialized for array
types.
> This way we could use SmartPointer for both array types and scalar types.

How about making the SmartPointerBase destructor protected?
Actually nobody should have reference it via the base class.

> It may make sense to replace current implementation of SmartPointer with one
> from Blink or Chromium where all such problems have already been solved. WDYT?

[jochen (gone - plz use gerrit), 2013-12-04 08:07:47]

we can't use std::unique_ptr because it's not available on mac os 10.6's
libstdc++

Using get() however is inline with the std::unique_ptr API so I think this is a
good change.

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h
File src/smart-pointers.h (right):

https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h#newcode54
src/smart-pointers.h:54: ~SmartPointerBase() { if (p_) Deallocator::Delete(p_);
}
On 2013/12/04 07:44:48, alph wrote:
> On 2013/12/04 07:34:01, yurys wrote:
> > On 2013/12/03 16:17:55, svenpanne wrote:
> > > Hmmm, this has been like that before, but why is it safe to have a
> non-virtual
> > > destructor in a base class here? As it is, nothing prevents an incorrect
> usage
> > > AFAICT...
> > 
> > This is a good question. I believe the problem is that SmartArrayPointer and
> > SmartPointer inherit from SmartPointerBase. This could be solved e.g. by
> making
> > SmartPointer a template with single parameter T. Then it may use Deallocator
> as
> > type traits internally, Deallocator in turn can be specialized for array
> types.
> > This way we could use SmartPointer for both array types and scalar types.
> 
> How about making the SmartPointerBase destructor protected?
> Actually nobody should have reference it via the base class.
> 
> > It may make sense to replace current implementation of SmartPointer with one
> > from Blink or Chromium where all such problems have already been solved.
WDYT?
> 

Yes, making it protected makes sense. Since this is a template thing and not a
virtual class hierarchy it should not be virtual.

[yurys, 2013-12-04 08:36:27]

On 2013/12/04 07:44:47, alph wrote:
> https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h
> File src/smart-pointers.h (right):
> 
>
https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h#newcode54
> src/smart-pointers.h:54: ~SmartPointerBase() { if (p_)
Deallocator::Delete(p_);
> }
> On 2013/12/04 07:34:01, yurys wrote:
> > On 2013/12/03 16:17:55, svenpanne wrote:
> > > Hmmm, this has been like that before, but why is it safe to have a
> non-virtual
> > > destructor in a base class here? As it is, nothing prevents an incorrect
> usage
> > > AFAICT...
> > 
> > This is a good question. I believe the problem is that SmartArrayPointer and
> > SmartPointer inherit from SmartPointerBase. This could be solved e.g. by
> making
> > SmartPointer a template with single parameter T. Then it may use Deallocator
> as
> > type traits internally, Deallocator in turn can be specialized for array
> types.
> > This way we could use SmartPointer for both array types and scalar types.
> 
> How about making the SmartPointerBase destructor protected?
> Actually nobody should have reference it via the base class.
> 
Done.

[yurys, 2013-12-04 08:43:09]

On 2013/12/04 08:07:47, jochen wrote:
> we can't use std::unique_ptr because it's not available on mac os 10.6's
> libstdc++
> 
> Using get() however is inline with the std::unique_ptr API so I think this is
a
> good change.
> 
scoped_ptr doesn't depend on unique_ptr while provides similar interface which
makes transition to unique_ptr very easy when the time comes. Taking that into
account it may make sense to replace SmartPointer with scoped_ptr and replace it
later with unique_ptr. Do you suggest we should skip this intermediate step and
switch straight to unique_ptr when support for mac os 10.6 is deprecated?

> https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h
> File src/smart-pointers.h (right):
> 
>
https://codereview.chromium.org/101763003/diff/1/src/smart-pointers.h#newcode54
> src/smart-pointers.h:54: ~SmartPointerBase() { if (p_)
Deallocator::Delete(p_);
> }
> On 2013/12/04 07:44:48, alph wrote:
> > On 2013/12/04 07:34:01, yurys wrote:
> > > On 2013/12/03 16:17:55, svenpanne wrote:
> > > > Hmmm, this has been like that before, but why is it safe to have a
> > non-virtual
> > > > destructor in a base class here? As it is, nothing prevents an incorrect
> > usage
> > > > AFAICT...
> > > 
> > > This is a good question. I believe the problem is that SmartArrayPointer
and
> > > SmartPointer inherit from SmartPointerBase. This could be solved e.g. by
> > making
> > > SmartPointer a template with single parameter T. Then it may use
Deallocator
> > as
> > > type traits internally, Deallocator in turn can be specialized for array
> > types.
> > > This way we could use SmartPointer for both array types and scalar types.
> > 
> > How about making the SmartPointerBase destructor protected?
> > Actually nobody should have reference it via the base class.
> > 
> > > It may make sense to replace current implementation of SmartPointer with
one
> > > from Blink or Chromium where all such problems have already been solved.
> WDYT?
> > 
> 
> Yes, making it protected makes sense. Since this is a template thing and not a
> virtual class hierarchy it should not be virtual.

Done.

[yurys, 2013-12-05 07:15:22]

Sven, I've made the destructor protected, do you have any other concerns?

[Sven Panne, 2013-12-05 07:38:12]

On 2013/12/05 07:15:22, yurys wrote:
> Sven, I've made the destructor protected, do you have any other concerns?

I think it is OK, moving to std::unique_ptr might still take a few years... :-}

[Sven Panne, 2013-12-05 07:38:12]

On 2013/12/05 07:15:22, yurys wrote:
> Sven, I've made the destructor protected, do you have any other concerns?

I think it is OK, moving to std::unique_ptr might still take a few years... :-}

[yurys, 2013-12-05 07:51:20]

On 2013/12/05 07:38:12, Sven Panne wrote:
> On 2013/12/05 07:15:22, yurys wrote:
> > Sven, I've made the destructor protected, do you have any other concerns?
> 
> I think it is OK, moving to std::unique_ptr might still take a few years...
:-}

Could you provide OWNER's approval then ? :-)

[Sven Panne, 2013-12-05 08:11:23]

LGTM. Ooops, forgot that there was only a non-OWNER LGTM :-}

[yurys, 2013-12-05 08:31:32]

On 2013/12/05 08:11:23, Sven Panne wrote:
> LGTM. Ooops, forgot that there was only a non-OWNER LGTM :-}

Thank you!

[yurys, 2013-12-09 07:42:25]

Committed patchset #4 manually as r18275 (presubmit successful).