Change nonstd::unique_ptr to take a custom deleter.

Change nonstd::unique_ptr to take a custom deleter.

Code is mostly stolen from Chromium's scoped_ptr.

- Add unit tests.
- Use this to fix a leak.

BUG=chromium:531408
R=jyasskin@chromium.org, tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/cef2a9c51bee4b987fc813013d45dad6535a9a46

[Lei Zhang, 2015-09-18 22:35:48]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2015-09-18 22:37:17]

thestig@chromium.org changed reviewers:
+ jyasskin@chromium.org, tsepez@chromium.org

[Lei Zhang, 2015-09-18 22:37:17]

I really want custom deleters, so I copied a lot of code out of scoped_ptr.h.
O:-)

jyasskin: PTAL at third_party/base
tsepez: The rest

[Lei Zhang, 2015-09-18 22:40:57]

Oh, and fx_basic_unique_ptr_unittest.cpp is in patch set 2. I copied over all
the tests that don't involve scoped_ptr::Pass().

[Tom Sepez, 2015-09-18 23:29:33]

I took a pass at the non-third_party/base stuff. I'll defer on the other to
Jeffrey.

https://codereview.chromium.org/1351383004/diff/40001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1351383004/diff/40001/BUILD.gn#newcode738
BUILD.gn:738: "core/src/fxcrt/fx_basic_unique_ptr_unittest.cpp",
I guess this is OK, I might like it better if it were in the same directory as
unique_ptr.h itself, as FX doesn't have any code actually in this test. Does
that torque the build?

https://codereview.chromium.org/1351383004/diff/40001/core/include/fxcrt/fx_b...
File core/include/fxcrt/fx_basic.h (right):

https://codereview.chromium.org/1351383004/diff/40001/core/include/fxcrt/fx_b...
core/include/fxcrt/fx_basic.h:956: // TODO(thestig) Remove in favor of
nonstd::unique_ptr.
except that this is more like shared ptr, with ref-counting pushed off onto each
of the call sites.

https://codereview.chromium.org/1351383004/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1351383004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2379:
nonstd::unique_ptr<CPDF_Object, ReleaseDeleter<CPDF_Object>> obj(
Is there a bug for this leak? Maybe separate CLs in case this gets reverted, we
don't lose the ptr work ...

https://codereview.chromium.org/1351383004/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/JS_Object.cpp (right):

https://codereview.chromium.org/1351383004/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/JS_Object.cpp:93: m_pObject.Reset();
Do we even have to do this?  This member is a v8::Global<>, doesn't its own dtor
do this when its called automatically?

[Lei Zhang, 2015-09-18 23:51:52]

https://codereview.chromium.org/1351383004/diff/40001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1351383004/diff/40001/BUILD.gn#newcode738
BUILD.gn:738: "core/src/fxcrt/fx_basic_unique_ptr_unittest.cpp",
On 2015/09/18 23:29:32, Tom Sepez wrote:
> I guess this is OK, I might like it better if it were in the same directory as
> unique_ptr.h itself, as FX doesn't have any code actually in this test. Does
> that torque the build?

Moved over to third_party/base/nonstd_unique_ptr_unittest.cpp.

https://codereview.chromium.org/1351383004/diff/40001/core/include/fxcrt/fx_b...
File core/include/fxcrt/fx_basic.h (right):

https://codereview.chromium.org/1351383004/diff/40001/core/include/fxcrt/fx_b...
core/include/fxcrt/fx_basic.h:956: // TODO(thestig) Remove in favor of
nonstd::unique_ptr.
On 2015/09/18 23:29:32, Tom Sepez wrote:
> except that this is more like shared ptr, with ref-counting pushed off onto
each
> of the call sites.

Is it? I'm not seeing the ref-counting.

https://codereview.chromium.org/1351383004/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1351383004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2379:
nonstd::unique_ptr<CPDF_Object, ReleaseDeleter<CPDF_Object>> obj(
On 2015/09/18 23:29:32, Tom Sepez wrote:
> Is there a bug for this leak? Maybe separate CLs in case this gets reverted,
we
> don't lose the ptr work ...

Yes, there is a BUG= line for this CL.

https://codereview.chromium.org/1351383004/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/JS_Object.cpp (right):

https://codereview.chromium.org/1351383004/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/JS_Object.cpp:93: m_pObject.Reset();
On 2015/09/18 23:29:32, Tom Sepez wrote:
> Do we even have to do this?  This member is a v8::Global<>, doesn't its own
dtor
> do this when its called automatically?

Don't think so. Note, this is a v8::Global, but ~Global calls Reset().

[Jeffrey Yasskin, 2015-09-19 00:00:48]

Can you also include the non-compilation test from scoped_ptr?

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
File third_party/base/nonstd_unique_ptr.h (right):

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:76: #include <algorithm>
I don't see any uses of these two headers.

You are using <ostream>.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:92: // invokes 'delete'. The default
deleter for scoped_ptr<T>.
s/scoped_ptr/unique_ptr/

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:150: struct ShouldAbortOnSelfReset {
Can you drop this, and just use the C++11 behavior of reset() (call the deleter
on the original value if it's non-null)?

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:216: C& operator*() const {
Could you disable unique_ptr<T[]>::operator* and operator->? They don't exist in
std::unique_ptr for arrays, and I'd like as much compatibility as possible.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:282: unique_ptr(C* p, const D& d) :
internal::unique_ptr_base<C, D>(p, d) {}
This isn't quite the same as C++11, but it should be good enough. The C++11
interface is moderately complicated.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:323: explicit unique_ptr(C* p) :
internal::unique_ptr_base<C, D>(p) {}
You need to also copy the "template <typename U> explicit scoped_ptr(U* array);"
in order to block non-C arguments.

And also "template <typename U> void reset(U* array);"

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
File third_party/base/nonstd_unique_ptr_unittest.cpp (right):

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr_unittest.cpp:29: ~ConDecLogger() override {
--*ptr_; }
Is "Dec" an abbreviation for "Destruction"?

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/templa...
File third_party/base/template_util.h (right):

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/templa...
third_party/base/template_util.h:65: struct IsClassHelper {
Drop IsClassHelper since you're not using it?

[Tom Sepez, 2015-09-21 16:54:33]

LGTM on the non-base code.

[Tom Sepez, 2015-09-22 17:36:20]

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
File third_party/base/nonstd_unique_ptr_unittest.cpp (right):

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr_unittest.cpp:25: class ConDecLogger : public
ConDecLoggerParent {
Actually, I want to steal this file to test nonstd::move(), do you want to land
first or do you want to merge?

[Tom Sepez, 2015-09-22 22:11:04]

ConDecLoggerParent {
> Actually, I want to steal this file to test nonstd::move(), do you want to
land
> first or do you want to merge?

And I stole it.  And I renamed to CtorDtorLogger per comment above.  Should be a
simple merge.

[Lei Zhang, 2015-09-22 22:52:40]

On 2015/09/22 22:11:04, Tom Sepez wrote:
> ConDecLoggerParent {
> > Actually, I want to steal this file to test nonstd::move(), do you want to
> land
> > first or do you want to merge?
> 
> And I stole it.  And I renamed to CtorDtorLogger per comment above.  Should be
a
> simple merge.

Sure. Steal all the bits you like. I'll get back to this CL soon and rebase as
needed.

[Lei Zhang, 2015-09-23 00:38:56]

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
File third_party/base/nonstd_unique_ptr.h (right):

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:76: #include <algorithm>
On 2015/09/19 00:00:47, Jeffrey Yasskin wrote:
> I don't see any uses of these two headers.
> 
> You are using <ostream>.

Done.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:92: // invokes 'delete'. The default
deleter for scoped_ptr<T>.
On 2015/09/19 00:00:47, Jeffrey Yasskin wrote:
> s/scoped_ptr/unique_ptr/

Done.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:150: struct ShouldAbortOnSelfReset {
On 2015/09/19 00:00:47, Jeffrey Yasskin wrote:
> Can you drop this, and just use the C++11 behavior of reset() (call the
deleter
> on the original value if it's non-null)?

So, let the caller shoot themselves in the foot with a double free?

{
  Foo* raw_foo = new Foo;
  std::unique_ptr<Foo> foo(raw_foo);
  foo.reset(raw_foo);  // free once
}  // double free

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:216: C& operator*() const {
On 2015/09/19 00:00:47, Jeffrey Yasskin wrote:
> Could you disable unique_ptr<T[]>::operator* and operator->? They don't exist
in
> std::unique_ptr for arrays, and I'd like as much compatibility as possible.

Moved out of the base impl.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:282: unique_ptr(C* p, const D& d) :
internal::unique_ptr_base<C, D>(p, d) {}
On 2015/09/19 00:00:47, Jeffrey Yasskin wrote:
> This isn't quite the same as C++11, but it should be good enough. The C++11
> interface is moderately complicated.

Acknowledged.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:323: explicit unique_ptr(C* p) :
internal::unique_ptr_base<C, D>(p) {}
On 2015/09/19 00:00:48, Jeffrey Yasskin wrote:
> You need to also copy the "template <typename U> explicit scoped_ptr(U*
array);"
> in order to block non-C arguments.
> 
> And also "template <typename U> void reset(U* array);"

Done.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
File third_party/base/nonstd_unique_ptr_unittest.cpp (right):

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr_unittest.cpp:25: class ConDecLogger : public
ConDecLoggerParent {
On 2015/09/22 17:36:20, Tom Sepez wrote:
> Actually, I want to steal this file to test nonstd::move(), do you want to
land
> first or do you want to merge?

All yours.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr_unittest.cpp:29: ~ConDecLogger() override {
--*ptr_; }
On 2015/09/19 00:00:48, Jeffrey Yasskin wrote:
> Is "Dec" an abbreviation for "Destruction"?

Tom renamed this in his CL.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/templa...
File third_party/base/template_util.h (right):

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/templa...
third_party/base/template_util.h:65: struct IsClassHelper {
On 2015/09/19 00:00:48, Jeffrey Yasskin wrote:
> Drop IsClassHelper since you're not using it?

Done.

[Jeffrey Yasskin, 2015-09-23 00:53:35]

Any news on the non-compilation tests? LGTM anyway.

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
File third_party/base/nonstd_unique_ptr.h (right):

https://codereview.chromium.org/1351383004/diff/60001/third_party/base/nonstd...
third_party/base/nonstd_unique_ptr.h:150: struct ShouldAbortOnSelfReset {
On 2015/09/23 00:38:55, Lei Zhang wrote:
> On 2015/09/19 00:00:47, Jeffrey Yasskin wrote:
> > Can you drop this, and just use the C++11 behavior of reset() (call the
> deleter
> > on the original value if it's non-null)?
> 
> So, let the caller shoot themselves in the foot with a double free?
> 
> {
>   Foo* raw_foo = new Foo;
>   std::unique_ptr<Foo> foo(raw_foo);
>   foo.reset(raw_foo);  // free once
> }  // double free

Yeah. ASan should save us in a lot of cases, and we'll lose the protection
anyway when we go to a C++11 library.

https://codereview.chromium.org/1351383004/diff/100001/third_party/base/nonst...
File third_party/base/nonstd_unique_ptr.h (right):

https://codereview.chromium.org/1351383004/diff/100001/third_party/base/nonst...
third_party/base/nonstd_unique_ptr.h:176: // Note that running data_.ptr = p can
lead to undefined behavior if
I forgot to mention this: you can go straight to the C++11 behavior in this new
class. Assign data_.ptr=p before destroying 'old'.

https://codereview.chromium.org/1351383004/diff/100001/third_party/base/nonst...
third_party/base/nonstd_unique_ptr.h:273: if (that.data_.ptr != this->data_.ptr)
You can remove this check: a self-reset will naturally release the pointer, and
re-acquire it cleanly.

https://codereview.chromium.org/1351383004/diff/100001/third_party/base/nonst...
third_party/base/nonstd_unique_ptr.h:347: if (that.data_.ptr != this->data_.ptr)
Same here.

[Lei Zhang, 2015-09-23 01:00:09]

On 2015/09/23 00:53:35, Jeffrey Yasskin wrote:
> Any news on the non-compilation tests? LGTM anyway.

Oh, forgot about that. Did you want me to just include the tests for reference
but ifdef them out?

[Jeffrey Yasskin, 2015-09-23 01:04:44]

On 2015/09/23 01:00:09, Lei Zhang wrote:
> On 2015/09/23 00:53:35, Jeffrey Yasskin wrote:
> > Any news on the non-compilation tests? LGTM anyway.
> 
> Oh, forgot about that. Did you want me to just include the tests for reference
> but ifdef them out?

Ideally, I'd like to keep the assertions that some code fails to compile, but I
don't know if the third_party/base test infrastructure is capable of it. It
looks like gyp and gn might have native support for the .nc extension
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/BUILD.gn&l=1...
If so, just copying scoped_ptr_unittest.nc to unique_ptr_unittest.nc, modifying
its contents a bit, and adding it to this BUILD.gn should work.

[Lei Zhang, 2015-09-23 01:11:30]

Oh, that thing. I'll take a look at the non-compilation tests.

https://codereview.chromium.org/1351383004/diff/100001/third_party/base/nonst...
File third_party/base/nonstd_unique_ptr.h (right):

https://codereview.chromium.org/1351383004/diff/100001/third_party/base/nonst...
third_party/base/nonstd_unique_ptr.h:176: // Note that running data_.ptr = p can
lead to undefined behavior if
On 2015/09/23 00:53:34, Jeffrey Yasskin wrote:
> I forgot to mention this: you can go straight to the C++11 behavior in this
new
> class. Assign data_.ptr=p before destroying 'old'.

Done.

https://codereview.chromium.org/1351383004/diff/100001/third_party/base/nonst...
third_party/base/nonstd_unique_ptr.h:273: if (that.data_.ptr != this->data_.ptr)
On 2015/09/23 00:53:34, Jeffrey Yasskin wrote:
> You can remove this check: a self-reset will naturally release the pointer,
and
> re-acquire it cleanly.

Done.

https://codereview.chromium.org/1351383004/diff/100001/third_party/base/nonst...
third_party/base/nonstd_unique_ptr.h:347: if (that.data_.ptr != this->data_.ptr)
On 2015/09/23 00:53:35, Jeffrey Yasskin wrote:
> Same here.

Done.

[Lei Zhang, 2015-09-23 01:40:28]

On 2015/09/23 01:04:44, Jeffrey Yasskin wrote:
> On 2015/09/23 01:00:09, Lei Zhang wrote:
> > On 2015/09/23 00:53:35, Jeffrey Yasskin wrote:
> > > Any news on the non-compilation tests? LGTM anyway.
> > 
> > Oh, forgot about that. Did you want me to just include the tests for
reference
> > but ifdef them out?
> 
> Ideally, I'd like to keep the assertions that some code fails to compile, but
I
> don't know if the third_party/base test infrastructure is capable of it. It
> looks like gyp and gn might have native support for the .nc extension
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/BUILD.gn&l=1...
> If so, just copying scoped_ptr_unittest.nc to unique_ptr_unittest.nc,
modifying
> its contents a bit, and adding it to this BUILD.gn should work.

The NC test support in Chromium comes from build/nocompile.gypi and
tools/nocompile_driver.py, as seen in https://codereview.chromium.org/678263003
, but we are not there yet in Chromium. I can file a bug for hooking that up and
work on that separately.

[Lei Zhang, 2015-09-23 02:15:53]

Committed patchset #6 (id:120001) manually as
cef2a9c51bee4b987fc813013d45dad6535a9a46 (presubmit successful).