Add assert() for scoped_refptr<T>::operator->()

Add assert() for scoped_refptr<T>::operator->()

This is to make NULL dereference crashes more obvious.  Also matches scoped_ptr (which uses assert) and weak_ptr (which uses DCHECK).

DCHECK wasn't used because it pulls in logging.h, and that somehow conflicts with a webkit #define of LOG.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119820

[akalin, 2012-01-28 02:33:01]

+jar for review

[akalin, 2012-01-28 02:37:36]

On 2012/01/28 02:33:01, akalin wrote:
> +jar for review

one second, it looks like including logging.h conflicts with a webkit define for
LOG.  trying assert instead (like scoped_ptr)...

[akalin, 2012-01-28 02:43:15]

On 2012/01/28 02:37:36, akalin wrote:
> On 2012/01/28 02:33:01, akalin wrote:
> > +jar for review
> 
> one second, it looks like including logging.h conflicts with a webkit define
for
> LOG.  trying assert instead (like scoped_ptr)...

Okay, used assert() instead.  PTAL.

[jar (doing other things), 2012-01-28 02:47:22]

https://chromiumcodereview.appspot.com/9232069/diff/1003/base/memory/ref_coun...
File base/memory/ref_counted.h (right):

https://chromiumcodereview.appspot.com/9232069/diff/1003/base/memory/ref_coun...
base/memory/ref_counted.h:249: assert(ptr_ != NULL);
Could you clarify when this would be significant?

I'd assume that the result of this operator would be immediately deref'ed, to
get to some slot. I'd then expect it to segv.  Hence, the assert would be a
debug-only warning that the code is about to crash.

What caller-usage pattern am I missing?

[akalin, 2012-01-28 03:06:47]

On 2012/01/28 02:47:22, jar wrote:
>
https://chromiumcodereview.appspot.com/9232069/diff/1003/base/memory/ref_coun...
> File base/memory/ref_counted.h (right):
> 
>
https://chromiumcodereview.appspot.com/9232069/diff/1003/base/memory/ref_coun...
> base/memory/ref_counted.h:249: assert(ptr_ != NULL);
> Could you clarify when this would be significant?
> 
> I'd assume that the result of this operator would be immediately deref'ed, to
> get to some slot. I'd then expect it to segv.  Hence, the assert would be a
> debug-only warning that the code is about to crash.
> 
> What caller-usage pattern am I missing?

Here's a stack trace for a null-dereference without my patch:

        base::dse::(anonymous namespace)::StackDumpSignalHandler()
[0x7fedd3661539]
        0x7fedc77f2af0
        browser_sync::NewNonFrontendDataTypeController::StartAssociation()
[0x134970c]

As you can see, it's indistinguishable from your run-of-the-mill memory-stomping
bug.  Here's a stack trace with my patch:

        base::(anonymous namespace)::StackDumpSignalHandler() [0x7f7e54d6e6dd]
        0x7f7e48ef9af0
        0x7f7e48ef9a75
        0x7f7e48efd5c0
        0x7f7e48ef2941
        scoped_refptr<>::operator->() [0x134cb85]
        browser_sync::NewNonFrontendDataTypeController::StartAssociation()
[0x134bb00]

I know immediately it's a null-dereference of a scoped_refptr, which narrows
things down considerably.  I think this is worth having.

It's also worth pointing out that a NULL dereference doesn't necessarily result
in an immediate crash.  If the function call immediately calls another function
(on, say, a member variable) the crash may happen somewhere else far away with a
non-null pointer (say, 0x14).  I think it's worth catching NULL dereferences
early.

[jar (doing other things), 2012-01-30 20:36:46]

lgtm

[commit-bot: I haz the power, 2012-01-30 21:00:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9232069/1003

[commit-bot: I haz the power, 2012-01-31 00:17:21]

Try job failure for 9232069-1003 (retry) on win_rel for step "ui_tests".
It's a second try, previously, steps "browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-01-31 00:19:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9232069/1003

[commit-bot: I haz the power, 2012-01-31 02:16:57]

Change committed as 119820