Allow custom deleters to opt out of self reset checks for scoped_ptr.

Allow custom deleters to opt out of self reset checks for scoped_ptr.

The self-reset check makes sense for the default deleters, because it
would otherwise leave a dangling pointer stored in the scoped_ptr.
However, a custom deleter might actually decrement a reference count
under the hood. This self-reset check can make assignment operators
implementation a lot uglier. One example is net's KeyPair: because
there might be a self-assignment, the original code needed to proxy
the incoming scoped_ptrs via a stack temporary before moving them
into their final location.

BUG=418347
Committed: https://crrev.com/9961abd411eab6ec1cf9421078dfdaa001a7e2b5
Cr-Commit-Position: refs/heads/master@{#299571}

[dcheng, 2014-09-27 07:21:58]

dcheng@chromium.org changed reviewers:
+ brettw@chromium.org, jyasskin@chromium.org, rsleevi@chromium.org

[dcheng, 2014-09-29 16:29:11]

Allow custom deleters to opt out of self reset checks for scoped_ptr.

The self-reset check makes sense for the default deleters, because it
would otherwise leave a dangling pointer stored in the scoped_ptr.
However, a custom deleter might actually decrement a reference count
under the hood. This self-reset check can make assignment operators
implementation a lot uglier. One example is net's KeyPair: because
there might be a self-assignment, the original code needed to proxy
the incoming scoped_ptrs via a stack temporary before moving them
into their final location.

BUG=418347

[awong, 2014-09-29 17:00:33]

On 2014/09/29 16:29:11, dcheng wrote:
> Allow custom deleters to opt out of self reset checks for scoped_ptr.
> 
> The self-reset check makes sense for the default deleters, because it
> would otherwise leave a dangling pointer stored in the scoped_ptr.
> However, a custom deleter might actually decrement a reference count
> under the hood. This self-reset check can make assignment operators
> implementation a lot uglier. One example is net's KeyPair: because
> there might be a self-assignment, the original code needed to proxy
> the incoming scoped_ptrs via a stack temporary before moving them
> into their final location.
> 
> BUG=418347

LGTM

[Ryan Sleevi, 2014-09-29 17:04:41]

LGTM, but since you're here, can you also add it to the ScopedNSSTypes? Mattm@
and oneubeck@ ran into this as well.

[Jeffrey Yasskin, 2014-09-29 17:23:19]

-1. 

"my_scoped_ptr.reset(a);" is a claim that the calling code owns 'a', which is
incompatible with |my_scoped_ptr| owning the same value. Even if you're using a
custom deleter to allow a scoped_ptr to represent a unique handle to a
reference, allowing self-reset muddies the difference between scoped_ptr and
shared_ptr. 

This extra behavior will also make it harder to move to std::unique_ptr when our
libraries advance enough to allow it.

https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
net/ssl/openssl_client_key_store.cc:53: void
OpenSSLClientKeyStore::KeyPair::operator=(const KeyPair& other) {
operator= should generally be implemented with a by-value signature and a swap,
so:

void OpenSSLClientKeyStore::KeyPair::operator=(KeyPair other) {
  this->swap(other);
}

That avoids the need to duplicate the code to copy the input value.

[Ryan Sleevi, 2014-09-29 17:32:48]

On 2014/09/29 17:23:19, Jeffrey Yasskin wrote:
> -1. 
> 
> "my_scoped_ptr.reset(a);" is a claim that the calling code owns 'a', which is
> incompatible with |my_scoped_ptr| owning the same value. Even if you're using
a
> custom deleter to allow a scoped_ptr to represent a unique handle to a
> reference, allowing self-reset muddies the difference between scoped_ptr and
> shared_ptr. 
> 
> This extra behavior will also make it harder to move to std::unique_ptr when
our
> libraries advance enough to allow it.
> 
> 

No. It isn't incompatible at all.

Please see the related GCC bug.

The claim is that both parties, according to the third party API, own objects A
and B. That A and B point to the same object is an implementation detail of the
third party API.

Both owners are obligated to release their objects (A and B). They do so via a
custom deleter call to the third party API.

No invariants are violated, and the library CANNOT enforce the requirement we
are doing here, precisely because it breaks with custom deleters.

https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
> File net/ssl/openssl_client_key_store.cc (right):
> 
>
https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
> net/ssl/openssl_client_key_store.cc:53: void
> OpenSSLClientKeyStore::KeyPair::operator=(const KeyPair& other) {
> operator= should generally be implemented with a by-value signature and a
swap,
> so:
> 
> void OpenSSLClientKeyStore::KeyPair::operator=(KeyPair other) {
>   this->swap(other);
> }
> 
> That avoids the need to duplicate the code to copy the input value.

[Ryan Sleevi, 2014-09-29 17:32:51]

On 2014/09/29 17:23:19, Jeffrey Yasskin wrote:
> -1. 
> 
> "my_scoped_ptr.reset(a);" is a claim that the calling code owns 'a', which is
> incompatible with |my_scoped_ptr| owning the same value. Even if you're using
a
> custom deleter to allow a scoped_ptr to represent a unique handle to a
> reference, allowing self-reset muddies the difference between scoped_ptr and
> shared_ptr. 
> 
> This extra behavior will also make it harder to move to std::unique_ptr when
our
> libraries advance enough to allow it.
> 
> 

No. It isn't incompatible at all.

Please see the related GCC bug.

The claim is that both parties, according to the third party API, own objects A
and B. That A and B point to the same object is an implementation detail of the
third party API.

Both owners are obligated to release their objects (A and B). They do so via a
custom deleter call to the third party API.

No invariants are violated, and the library CANNOT enforce the requirement we
are doing here, precisely because it breaks with custom deleters.

https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
> File net/ssl/openssl_client_key_store.cc (right):
> 
>
https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
> net/ssl/openssl_client_key_store.cc:53: void
> OpenSSLClientKeyStore::KeyPair::operator=(const KeyPair& other) {
> operator= should generally be implemented with a by-value signature and a
swap,
> so:
> 
> void OpenSSLClientKeyStore::KeyPair::operator=(KeyPair other) {
>   this->swap(other);
> }
> 
> That avoids the need to duplicate the code to copy the input value.

[Jeffrey Yasskin, 2014-09-29 17:47:50]

On Mon, Sep 29, 2014 at 10:32 AM,  <rsleevi@chromium.org> wrote:
> On 2014/09/29 17:23:19, Jeffrey Yasskin wrote:
>>
>> -1.
>
>
>> "my_scoped_ptr.reset(a);" is a claim that the calling code owns 'a', which
>> is
>> incompatible with |my_scoped_ptr| owning the same value. Even if you're
>> using
>
> a
>>
>> custom deleter to allow a scoped_ptr to represent a unique handle to a
>> reference, allowing self-reset muddies the difference between scoped_ptr
>> and
>> shared_ptr.
>
>
>> This extra behavior will also make it harder to move to std::unique_ptr
>> when
>
> our
>>
>> libraries advance enough to allow it.
>
>
>
>
> No. It isn't incompatible at all.
>
> Please see the related GCC bug.
>
> The claim is that both parties, according to the third party API, own
> objects A
> and B. That A and B point to the same object is an implementation detail of
> the
> third party API.
>
> Both owners are obligated to release their objects (A and B). They do so via
> a
> custom deleter call to the third party API.
>
> No invariants are violated, and the library CANNOT enforce the requirement
> we
> are doing here, precisely because it breaks with custom deleters.

'k, good point. I retract my objection.

You're still using more code than you need in the assignment operator.

>
https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
>>
>> File net/ssl/openssl_client_key_store.cc (right):
>
>
>
>
https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
>>
>> net/ssl/openssl_client_key_store.cc:53: void
>> OpenSSLClientKeyStore::KeyPair::operator=(const KeyPair& other) {
>> operator= should generally be implemented with a by-value signature and a
>
> swap,
>>
>> so:
>
>
>> void OpenSSLClientKeyStore::KeyPair::operator=(KeyPair other) {
>>    this->swap(other);
>> }
>
>
>> That avoids the need to duplicate the code to copy the input value.
>
>
>
>
> https://codereview.chromium.org/610533003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2014-09-29 18:50:06]

On 2014/09/29 at 17:47:50, jyasskin wrote:
> On Mon, Sep 29, 2014 at 10:32 AM,  <rsleevi@chromium.org> wrote:
> > On 2014/09/29 17:23:19, Jeffrey Yasskin wrote:
> >>
> >> -1.
> >
> >
> >> "my_scoped_ptr.reset(a);" is a claim that the calling code owns 'a', which
> >> is
> >> incompatible with |my_scoped_ptr| owning the same value. Even if you're
> >> using
> >
> > a
> >>
> >> custom deleter to allow a scoped_ptr to represent a unique handle to a
> >> reference, allowing self-reset muddies the difference between scoped_ptr
> >> and
> >> shared_ptr.
> >
> >
> >> This extra behavior will also make it harder to move to std::unique_ptr
> >> when
> >
> > our
> >>
> >> libraries advance enough to allow it.
> >
> >
> >
> >
> > No. It isn't incompatible at all.
> >
> > Please see the related GCC bug.
> >
> > The claim is that both parties, according to the third party API, own
> > objects A
> > and B. That A and B point to the same object is an implementation detail of
> > the
> > third party API.
> >
> > Both owners are obligated to release their objects (A and B). They do so via
> > a
> > custom deleter call to the third party API.
> >
> > No invariants are violated, and the library CANNOT enforce the requirement
> > we
> > are doing here, precisely because it breaks with custom deleters.
> 
> 'k, good point. I retract my objection.
> 
> You're still using more code than you need in the assignment operator.
> 
> >
https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
> >>
> >> File net/ssl/openssl_client_key_store.cc (right):
> >
> >
> >
> >
https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
> >>
> >> net/ssl/openssl_client_key_store.cc:53: void
> >> OpenSSLClientKeyStore::KeyPair::operator=(const KeyPair& other) {
> >> operator= should generally be implemented with a by-value signature and a
> >
> > swap,
> >>
> >> so:
> >
> >
> >> void OpenSSLClientKeyStore::KeyPair::operator=(KeyPair other) {
> >>    this->swap(other);
> >> }
> >
> >
> >> That avoids the need to duplicate the code to copy the input value.
> >
> >
> >
> >
> > https://codereview.chromium.org/610533003/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

Perhaps I'm missing something, but I don't think swap() is suitable for a copy
assignment operator?

[Ryan Sleevi, 2014-09-29 18:57:08]

https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
net/ssl/openssl_client_key_store.cc:53: void
OpenSSLClientKeyStore::KeyPair::operator=(const KeyPair& other) {
On 2014/09/29 17:23:19, Jeffrey Yasskin wrote:
> operator= should generally be implemented with a by-value signature and a
swap,
> so:
> 
> void OpenSSLClientKeyStore::KeyPair::operator=(KeyPair other) {
>   this->swap(other);
> }
> 
> That avoids the need to duplicate the code to copy the input value.

Except this isn't a swap. It's a copy.

[Jeffrey Yasskin, 2014-09-29 19:20:53]

https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
net/ssl/openssl_client_key_store.cc:53: void
OpenSSLClientKeyStore::KeyPair::operator=(const KeyPair& other) {
On 2014/09/29 18:57:08, Ryan Sleevi (expect_delays) wrote:
> On 2014/09/29 17:23:19, Jeffrey Yasskin wrote:
> > operator= should generally be implemented with a by-value signature and a
> swap,
> > so:
> > 
> > void OpenSSLClientKeyStore::KeyPair::operator=(KeyPair other) {
> >   this->swap(other);
> > }
> > 
> > That avoids the need to duplicate the code to copy the input value.
> 
> Except this isn't a swap. It's a copy.

Yeah, the copy happens because the argument is passed by value, invoking the
copy constructor. Then the new value gets swapped into *this.

This saves a set of EVP_PKEY_dup() calls when the argument to operator= is a
temporary, because in that case the copy constructor doesn't need to be called
to create a by-value argument.

Search for the "copy-and-swap idiom".

[Ryan Sleevi, 2014-09-29 19:24:51]

On 2014/09/29 19:20:53, Jeffrey Yasskin wrote:
>
https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
> File net/ssl/openssl_client_key_store.cc (right):
> 
>
https://codereview.chromium.org/610533003/diff/20001/net/ssl/openssl_client_k...
> net/ssl/openssl_client_key_store.cc:53: void
> OpenSSLClientKeyStore::KeyPair::operator=(const KeyPair& other) {
> On 2014/09/29 18:57:08, Ryan Sleevi (expect_delays) wrote:
> > On 2014/09/29 17:23:19, Jeffrey Yasskin wrote:
> > > operator= should generally be implemented with a by-value signature and a
> > swap,
> > > so:
> > > 
> > > void OpenSSLClientKeyStore::KeyPair::operator=(KeyPair other) {
> > >   this->swap(other);
> > > }
> > > 
> > > That avoids the need to duplicate the code to copy the input value.
> > 
> > Except this isn't a swap. It's a copy.
> 
> Yeah, the copy happens because the argument is passed by value, invoking the
> copy constructor. Then the new value gets swapped into *this.
> 
> This saves a set of EVP_PKEY_dup() calls when the argument to operator= is a
> temporary, because in that case the copy constructor doesn't need to be called
> to create a by-value argument.

Sorry, I missed your comment re: changing from ref- to by-val.

> 
> Search for the "copy-and-swap idiom".

And here I thought the only reason to do it was for noexcept safety :) I didn't
think about the temp.

[dcheng, 2014-09-29 22:31:20]

dcheng@chromium.org changed reviewers:
+ ajwong@chromium.org

[dcheng, 2014-09-29 22:31:21]

So just to make sure I understand this (I just learned about the copy and swap
idiom today... =P), we wouldn't need any of the base changes if we rewrote the
assignment operators to use that idiom right?

In the case of a self-assignment, we'd create a copy and then swap that state to
|this|. The copy elision savings jyasskin@ was referring to is only when we pass
a temporary to the assignment operator, right?

[Ryan Sleevi, 2014-09-29 22:36:33]

On 2014/09/29 22:31:21, dcheng wrote:
> So just to make sure I understand this (I just learned about the copy and swap
> idiom today... =P), we wouldn't need any of the base changes if we rewrote the
> assignment operators to use that idiom right?

Ish?

There are still places where we were using a base::Passed() of a ScopedPK11Slot,
which for NSS is ref-counted.

When we do
a = b (or a = b.Pass()), we run into scoped_ptr<>'s operator= doing the
.reset(), and thus triggering the abort.

We'd have to rewrite all that code to use
a.swap(b)

which is not really idiomatic towards our usage of scoped_ptr<>

> 
> In the case of a self-assignment, we'd create a copy and then swap that state
to
> |this|. The copy elision savings jyasskin@ was referring to is only when we
pass
> a temporary to the assignment operator, right?

[Jeffrey Yasskin, 2014-09-29 22:39:30]

On Mon, Sep 29, 2014 at 3:31 PM,  <dcheng@chromium.org> wrote:
> So just to make sure I understand this (I just learned about the copy and
> swap
> idiom today... =P), we wouldn't need any of the base changes if we rewrote
> the
> assignment operators to use that idiom right?

You wouldn't, but I think Ryan's point is that the base changes are
semantically correct anyway, and may protect against flakiness in the
future. In particular, if two completely separate calls to
EVP_PKEY_dup() can return the same value, you don't really want to
crash when one is assigned over the other.

> In the case of a self-assignment, we'd create a copy and then swap that
> state to
> |this|. The copy elision savings jyasskin@ was referring to is only when we
> pass
> a temporary to the assignment operator, right?

Right. Literal self-assignment is only optimized by an explicit 'if'
in the operator, and it's not generally common enough for that to be
worth it.

> https://codereview.chromium.org/610533003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2014-09-29 23:23:16]

+pneubeck, +mattm as FYI

I've changed the KeyPair assignment operator to use the copy and swap idiom
jyasskin mentioned. I've also updated the scoped NSS types as well... I'm not
familiar with the interfaces for NSS or OpenSSL, so hopefully I didn't mess up
something.

[Ryan Sleevi, 2014-09-29 23:27:40]

//net LGTM

[dcheng, 2014-09-30 00:24:11]

Added a reference to the copy-and-swap idiom in KeyPair's assignment operator,
so people don't think passing by value was an accident.

[dcheng, 2014-09-30 00:24:19]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-09-30 00:25:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610533003/80001

[commit-bot: I haz the power, 2014-09-30 06:17:25]

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

[commit-bot: I haz the power, 2014-09-30 06:17:26]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[dcheng, 2014-09-30 17:57:08]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-09-30 17:59:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610533003/100001

[commit-bot: I haz the power, 2014-09-30 18:42:08]

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

[commit-bot: I haz the power, 2014-09-30 18:42:09]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2014-09-30 19:02:09]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:04:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610533003/100001

[commit-bot: I haz the power, 2014-09-30 22:05:45]

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

[commit-bot: I haz the power, 2014-09-30 22:05:46]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[dcheng, 2014-09-30 23:00:09]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-09-30 23:01:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610533003/120001

[dcheng, 2014-10-01 17:45:16]

PTAL. I had to make some adjustments to base/memory/scoped_ptr.h to make the
death tests work on Android. EXPECT_DEATH on Android catches _exit() but not
abort().

[dcheng, 2014-10-02 18:54:00]

Ping. Do any reviewers have an opinion on changing the abort() to _exit()?

Originally, I made this a CHECK(false) or LOG(FATAL). Unfortunately, that
depends on including base/logging.h. Even more unfortunately, some Blink code
includes scoped_ptr.h, and there are conflicting #defines between base/logging.h
and Blink's WTF logging. Per darin's suggestion, I changed the CHECK to use
abort().

However, abort() bypasses whatever logic Android is using to implement death
tests and causes EXPECT_DEATH tests to fail. I took a look at what
LOG(FATAL)/CHECK uses to kill the process, and they ultimately call _exit() via
base::debug::BreakDebugger(). I copied that logic and opted to call _exit() here
as well, but Albert points out that for an assert, it seems a bit more correct
to call abort() instead of _exit().

... as a compromise, would it make sense to call base::debug::BreakDebugger()
instead?

[Ryan Sleevi, 2014-10-02 19:31:03]

On 2014/10/02 18:54:00, dcheng wrote:
> Ping. Do any reviewers have an opinion on changing the abort() to _exit()?
> 
> Originally, I made this a CHECK(false) or LOG(FATAL). Unfortunately, that
> depends on including base/logging.h. Even more unfortunately, some Blink code
> includes scoped_ptr.h, and there are conflicting #defines between
base/logging.h
> and Blink's WTF logging. Per darin's suggestion, I changed the CHECK to use
> abort().
> 
> However, abort() bypasses whatever logic Android is using to implement death
> tests and causes EXPECT_DEATH tests to fail. I took a look at what
> LOG(FATAL)/CHECK uses to kill the process, and they ultimately call _exit()
via
> base::debug::BreakDebugger(). I copied that logic and opted to call _exit()
here
> as well, but Albert points out that for an assert, it seems a bit more correct
> to call abort() instead of _exit().
> 
> ... as a compromise, would it make sense to call base::debug::BreakDebugger()
> instead?

I was hoping someone on the Linux side (jyasskin, brett) would chime in.

Is there a reason not to use assert, like we do elsewhere in scoped_ptr<> ?

[Jeffrey Yasskin, 2014-10-02 19:34:32]

On 2014/10/02 19:31:03, Ryan Sleevi wrote:
> On 2014/10/02 18:54:00, dcheng wrote:
> > Ping. Do any reviewers have an opinion on changing the abort() to _exit()?
> > 
> > Originally, I made this a CHECK(false) or LOG(FATAL). Unfortunately, that
> > depends on including base/logging.h. Even more unfortunately, some Blink
code
> > includes scoped_ptr.h, and there are conflicting #defines between
> base/logging.h
> > and Blink's WTF logging. Per darin's suggestion, I changed the CHECK to use
> > abort().
> > 
> > However, abort() bypasses whatever logic Android is using to implement death
> > tests and causes EXPECT_DEATH tests to fail. I took a look at what
> > LOG(FATAL)/CHECK uses to kill the process, and they ultimately call _exit()
> via
> > base::debug::BreakDebugger(). I copied that logic and opted to call _exit()
> here
> > as well, but Albert points out that for an assert, it seems a bit more
correct
> > to call abort() instead of _exit().
> > 
> > ... as a compromise, would it make sense to call
base::debug::BreakDebugger()
> > instead?
> 
> I was hoping someone on the Linux side (jyasskin, brett) would chime in.

The concrete difference is that abort() will make the process die with a SIGABRT
signal, while _exit(1) makes it die normally with an error code. I don't know
enough about the Android death tests to know why that would make a difference
for them.

> Is there a reason not to use assert, like we do elsewhere in scoped_ptr<> ?

[dcheng, 2014-10-02 19:37:09]

On 2014/10/02 at 19:31:03, rsleevi wrote:
> On 2014/10/02 18:54:00, dcheng wrote:
> > Ping. Do any reviewers have an opinion on changing the abort() to _exit()?
> > 
> > Originally, I made this a CHECK(false) or LOG(FATAL). Unfortunately, that
> > depends on including base/logging.h. Even more unfortunately, some Blink
code
> > includes scoped_ptr.h, and there are conflicting #defines between
base/logging.h
> > and Blink's WTF logging. Per darin's suggestion, I changed the CHECK to use
> > abort().
> > 
> > However, abort() bypasses whatever logic Android is using to implement death
> > tests and causes EXPECT_DEATH tests to fail. I took a look at what
> > LOG(FATAL)/CHECK uses to kill the process, and they ultimately call _exit()
via
> > base::debug::BreakDebugger(). I copied that logic and opted to call _exit()
here
> > as well, but Albert points out that for an assert, it seems a bit more
correct
> > to call abort() instead of _exit().
> > 
> > ... as a compromise, would it make sense to call
base::debug::BreakDebugger()
> > instead?
> 
> I was hoping someone on the Linux side (jyasskin, brett) would chime in.
> 
> Is there a reason not to use assert, like we do elsewhere in scoped_ptr<> ?

I don't recall why I made this a fatal error even in non-debug builds. If I had
to guess at my reasons, it has to do with the fact that not aborting makes these
problems harder to catch, since you just end up with a dangling pointer. Many of
the other asserts are asserting that the stored pointer is non-null.

[dcheng, 2014-10-02 23:47:08]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org

[dcheng, 2014-10-02 23:47:35]

+danakj as a new base owner to weigh in on the abort()/_exit() business (since
brettw has removed himself as an owner).

[dcheng, 2014-10-03 00:02:50]

On 2014/10/02 23:47:35, dcheng wrote:
> +danakj as a new base owner to weigh in on the abort()/_exit() business (since
> brettw has removed himself as an owner).

To clarify, the problem is how to trigger termination (or whether we should)
when we detect self-reset. There are several options:

1. Use base/logging.h and solve the macro issues in Blink. I tried this when I
first added the self-reset abort() and gave up.

2. Use abort() and disable the death test on Android.

3. Use _exit(). I have to #include different headers for Windows and Posix, and
to me, doesn't read quite the same as abort().

4. Use base::debug::BreakDebugger(). However, it's not intuitive to me that this
aborts Chrome.

5. Use assert(). Won't catch dangling pointers in release code (which it
currently does today).

[dcheng, 2014-10-06 20:35:40]

Ping. Absent any opinions, my plan is to:
1. Restore the original behavior and just use abort().
2. Disable the tests on Android.
3. CQ it after that.

Please let me know if you'd like me to investigate/do something else. Thanks!

[Ryan Sleevi, 2014-10-07 00:37:14]

On 2014/10/06 20:35:40, dcheng wrote:
> Ping. Absent any opinions, my plan is to:
> 1. Restore the original behavior and just use abort().
> 2. Disable the tests on Android.
> 3. CQ it after that.
> 
> Please let me know if you'd like me to investigate/do something else. Thanks!

I'm ambivalent to the point of favouring assert(). I'm not keen on _exit(),
abort() works but still feels weird.

[pneubeck (no reviews), 2014-10-07 09:30:07]

pneubeck@chromium.org changed reviewers:
+ pneubeck@chromium.org

[pneubeck (no reviews), 2014-10-07 09:30:09]

looks good to me.

https://codereview.chromium.org/610533003/diff/160001/base/memory/scoped_ptr_...
File base/memory/scoped_ptr_unittest.cc (right):

https://codereview.chromium.org/610533003/diff/160001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:671: TEST(ScopedPtrTest,
SelfResetAbortsWithDefaultFreeDeleter) {
It's a bit irritating that you test with a 'default' deleter here: I had to read
the scoped_ptr implementation to find out whether FreeDeleter is somehow special
handled.

shouldn't this test that a custom deleter without using the opt-out still
aborts?
i.e. the same as SelfResetWithCustomDeleter but without 'typedef void
AllowSelfReset;' ?

https://codereview.chromium.org/610533003/diff/160001/net/ssl/openssl_client_...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/610533003/diff/160001/net/ssl/openssl_client_...
net/ssl/openssl_client_key_store.cc:58: // Intentionally pass by value, in order
to use the copy-and-swap idiom.
should this comment better be at the operator= function?

[danakj, 2014-10-07 18:49:38]

On 2014/10/07 00:37:14, Ryan Sleevi wrote:
> On 2014/10/06 20:35:40, dcheng wrote:
> > Ping. Absent any opinions, my plan is to:
> > 1. Restore the original behavior and just use abort().
> > 2. Disable the tests on Android.
> > 3. CQ it after that.
> > 
> > Please let me know if you'd like me to investigate/do something else.
Thanks!
> 
> I'm ambivalent to the point of favouring assert(). I'm not keen on _exit(),
> abort() works but still feels weird.

My instinct is to use base::debug::BreakDebugger() with a comment that this is
what CHECK() does, and a TODO() to use CHECK when blink fixes their macros.


I've also tried to rename the macros in blink to resolve this problem but it led
to a bikeshed shop.

[danakj, 2014-10-07 19:34:32]

On Tue, Oct 7, 2014 at 2:49 PM, <danakj@chromium.org> wrote:

> On 2014/10/07 00:37:14, Ryan Sleevi wrote:
>
>> On 2014/10/06 20:35:40, dcheng wrote:
>> > Ping. Absent any opinions, my plan is to:
>> > 1. Restore the original behavior and just use abort().
>> > 2. Disable the tests on Android.
>> > 3. CQ it after that.
>> >
>> > Please let me know if you'd like me to investigate/do something else.
>>
> Thanks!
>
>  I'm ambivalent to the point of favouring assert(). I'm not keen on
>> _exit(),
>> abort() works but still feels weird.
>>
>
> My instinct is to use base::debug::BreakDebugger() with a comment that
> this is
> what CHECK() does, and a TODO() to use CHECK when blink fixes their macros.


Albert pointed out to me that we should use abort() if that's what
unique_ptr would be using for this.

From what I can see[1] unique_ptr does not abort() or do anything to check
for this, it just assigns and then frees the pointer and goes on with life.

So then, this abort/CHECK/whatever is scoped_ptr specific already, and is
just being added to help track down offenders? Or why do we want to have
this abort at all?

[1]
http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/memory?view=markup
line 2625

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2014-10-07 22:40:09]

On 2014/10/07 at 19:34:32, danakj wrote:
> On Tue, Oct 7, 2014 at 2:49 PM, <danakj@chromium.org> wrote:
> 
> > On 2014/10/07 00:37:14, Ryan Sleevi wrote:
> >
> >> On 2014/10/06 20:35:40, dcheng wrote:
> >> > Ping. Absent any opinions, my plan is to:
> >> > 1. Restore the original behavior and just use abort().
> >> > 2. Disable the tests on Android.
> >> > 3. CQ it after that.
> >> >
> >> > Please let me know if you'd like me to investigate/do something else.
> >>
> > Thanks!
> >
> >  I'm ambivalent to the point of favouring assert(). I'm not keen on
> >> _exit(),
> >> abort() works but still feels weird.
> >>
> >
> > My instinct is to use base::debug::BreakDebugger() with a comment that
> > this is
> > what CHECK() does, and a TODO() to use CHECK when blink fixes their macros.
> 
> 
> Albert pointed out to me that we should use abort() if that's what
> unique_ptr would be using for this.
> 
> From what I can see[1] unique_ptr does not abort() or do anything to check
> for this, it just assigns and then frees the pointer and goes on with life.
> 

There are some open bugs to add an assert to unique_ptr to catch self-reset
cases (for example, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58159), but you
are correct that they are not currently implemented.

> So then, this abort/CHECK/whatever is scoped_ptr specific already, and is
> just being added to help track down offenders? Or why do we want to have
> this abort at all?
> 

Is there a reason not to abort? For all default deleters, a self-reset is
guaranteed to leave a dangling pointer. The google3 version of scoped_ptr treats
self-reset as a fatal error as well.

> [1]
> http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/memory?view=markup
> line 2625
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
>

[dcheng, 2014-10-07 22:40:37]

https://codereview.chromium.org/610533003/diff/160001/base/memory/scoped_ptr_...
File base/memory/scoped_ptr_unittest.cc (right):

https://codereview.chromium.org/610533003/diff/160001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:671: TEST(ScopedPtrTest,
SelfResetAbortsWithDefaultFreeDeleter) {
On 2014/10/07 at 09:30:08, pneubeck wrote:
> It's a bit irritating that you test with a 'default' deleter here: I had to
read the scoped_ptr implementation to find out whether FreeDeleter is somehow
special handled.
> 

I don't completely follow what you'd like me to change for this comment.

> shouldn't this test that a custom deleter without using the opt-out still
aborts?
> i.e. the same as SelfResetWithCustomDeleter but without 'typedef void
AllowSelfReset;' ?

Done.

https://codereview.chromium.org/610533003/diff/160001/net/ssl/openssl_client_...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/610533003/diff/160001/net/ssl/openssl_client_...
net/ssl/openssl_client_key_store.cc:58: // Intentionally pass by value, in order
to use the copy-and-swap idiom.
On 2014/10/07 at 09:30:09, pneubeck wrote:
> should this comment better be at the operator= function?

I'm ambivalent on this. I felt like it was better to describe the idiom where it
actually appeared, but I don't think putting it in the header is wrong either.
So moved. Done.

[pneubeck (no reviews), 2014-10-08 06:47:58]

https://codereview.chromium.org/610533003/diff/180001/net/ssl/openssl_client_...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/610533003/diff/180001/net/ssl/openssl_client_...
net/ssl/openssl_client_key_store.cc:55: this->swap(other);
nit: could 'this->' be dropped? maybe there was a reason for it that I don't
see.

[dcheng, 2014-10-08 18:40:22]

https://codereview.chromium.org/610533003/diff/180001/net/ssl/openssl_client_...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/610533003/diff/180001/net/ssl/openssl_client_...
net/ssl/openssl_client_key_store.cc:55: this->swap(other);
On 2014/10/08 at 06:47:57, pneubeck wrote:
> nit: could 'this->' be dropped? maybe there was a reason for it that I don't
see.

Done.

[danakj, 2014-10-08 19:01:20]

On 2014/10/07 22:40:09, dcheng wrote:
> On 2014/10/07 at 19:34:32, danakj wrote:
> > On Tue, Oct 7, 2014 at 2:49 PM, <mailto:danakj@chromium.org> wrote:
> > 
> > > On 2014/10/07 00:37:14, Ryan Sleevi wrote:
> > >
> > >> On 2014/10/06 20:35:40, dcheng wrote:
> > >> > Ping. Absent any opinions, my plan is to:
> > >> > 1. Restore the original behavior and just use abort().
> > >> > 2. Disable the tests on Android.
> > >> > 3. CQ it after that.
> > >> >
> > >> > Please let me know if you'd like me to investigate/do something else.
> > >>
> > > Thanks!
> > >
> > >  I'm ambivalent to the point of favouring assert(). I'm not keen on
> > >> _exit(),
> > >> abort() works but still feels weird.
> > >>
> > >
> > > My instinct is to use base::debug::BreakDebugger() with a comment that
> > > this is
> > > what CHECK() does, and a TODO() to use CHECK when blink fixes their
macros.
> > 
> > 
> > Albert pointed out to me that we should use abort() if that's what
> > unique_ptr would be using for this.
> > 
> > From what I can see[1] unique_ptr does not abort() or do anything to check
> > for this, it just assigns and then frees the pointer and goes on with life.
> > 
> 
> There are some open bugs to add an assert to unique_ptr to catch self-reset
> cases (for example, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58159), but
you
> are correct that they are not currently implemented.
> 
> > So then, this abort/CHECK/whatever is scoped_ptr specific already, and is
> > just being added to help track down offenders? Or why do we want to have
> > this abort at all?
> > 
> 
> Is there a reason not to abort? For all default deleters, a self-reset is
> guaranteed to leave a dangling pointer. The google3 version of scoped_ptr
treats
> self-reset as a fatal error as well.

If unique_ptr doesn't assert this, then us doing an abort is of limited value
since it would disappear when we switch to unique_ptr. But thanks for pointing
to the bug for unique_ptr asserting as well. It sounds like they will do this in
the future so let's go for it.

I don't see any use of abort() in llvm memory header, but 1 use of assert. Do
you have an idea how llvm would/will implement such a check on reset()? Whatever
you think they would do is fine with me, and if that thing doesn't work on
android we can disable the test with a TODO to fix android for now.

[dcheng, 2014-10-13 16:20:06]

On 2014/10/08 at 19:01:20, danakj wrote:
> On 2014/10/07 22:40:09, dcheng wrote:
> > On 2014/10/07 at 19:34:32, danakj wrote:
> > > On Tue, Oct 7, 2014 at 2:49 PM, <mailto:danakj@chromium.org> wrote:
> > > 
> > > > On 2014/10/07 00:37:14, Ryan Sleevi wrote:
> > > >
> > > >> On 2014/10/06 20:35:40, dcheng wrote:
> > > >> > Ping. Absent any opinions, my plan is to:
> > > >> > 1. Restore the original behavior and just use abort().
> > > >> > 2. Disable the tests on Android.
> > > >> > 3. CQ it after that.
> > > >> >
> > > >> > Please let me know if you'd like me to investigate/do something else.
> > > >>
> > > > Thanks!
> > > >
> > > >  I'm ambivalent to the point of favouring assert(). I'm not keen on
> > > >> _exit(),
> > > >> abort() works but still feels weird.
> > > >>
> > > >
> > > > My instinct is to use base::debug::BreakDebugger() with a comment that
> > > > this is
> > > > what CHECK() does, and a TODO() to use CHECK when blink fixes their
macros.
> > > 
> > > 
> > > Albert pointed out to me that we should use abort() if that's what
> > > unique_ptr would be using for this.
> > > 
> > > From what I can see[1] unique_ptr does not abort() or do anything to check
> > > for this, it just assigns and then frees the pointer and goes on with
life.
> > > 
> > 
> > There are some open bugs to add an assert to unique_ptr to catch self-reset
> > cases (for example, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58159), but
you
> > are correct that they are not currently implemented.
> > 
> > > So then, this abort/CHECK/whatever is scoped_ptr specific already, and is
> > > just being added to help track down offenders? Or why do we want to have
> > > this abort at all?
> > > 
> > 
> > Is there a reason not to abort? For all default deleters, a self-reset is
> > guaranteed to leave a dangling pointer. The google3 version of scoped_ptr
treats
> > self-reset as a fatal error as well.
> 
> If unique_ptr doesn't assert this, then us doing an abort is of limited value
since it would disappear when we switch to unique_ptr. But thanks for pointing
to the bug for unique_ptr asserting as well. It sounds like they will do this in
the future so let's go for it.
> 
> I don't see any use of abort() in llvm memory header, but 1 use of assert. Do
you have an idea how llvm would/will implement such a check on reset()? Whatever
you think they would do is fine with me, and if that thing doesn't work on
android we can disable the test with a TODO to fix android for now.

Given the feedback, I've just changed this to an assert(), since this is how I
expect it to be implemented upstream eventually.

[danakj, 2014-10-14 17:24:53]

That LGTM

[dcheng, 2014-10-14 17:28:50]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-14 17:30:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610533003/220001

[commit-bot: I haz the power, 2014-10-14 20:09:27]

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

[commit-bot: I haz the power, 2014-10-14 20:09:28]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[dcheng, 2014-10-14 20:27:43]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-14 20:28:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610533003/330001

[commit-bot: I haz the power, 2014-10-14 22:42:17]

Committed patchset #13 (id:330001)

[commit-bot: I haz the power, 2014-10-14 22:43:42]

Patchset 13 (id:??) landed as
https://crrev.com/9961abd411eab6ec1cf9421078dfdaa001a7e2b5
Cr-Commit-Position: refs/heads/master@{#299571}