This look like a fall out issue from https://codereview.chromium.org/2919243002/

That cl looks unrelated; mispaste?

Sorry, probably jumped to a conclusion there. That was the last clang static analysis change that has come to my attention and the issue was filed in this component (which I triage). Perhaps this should be re-routed?

kmarshall set up the static analyzer build; I'm guessing he filed this for himself and just applied this label since it looks related.

Let's just mark in Available instead of Untriaged.

That's correct. Sorry, is there a more appropriate component that I should use instead?

Makes sense to put static analysis related things in here, so let's leave it here.

(ajwong, dcheng because this might be a fun puzzle.)

I think #1 can be cleaned up by putting ANALYZER_ASSUME_TRUE(false) in RefCountedBase's deletion logic. There's no point in following codepaths which simulate the deletion and dereferencing of data. It's impossible, the refcounting system specifically prevents that. See https://chromium-review.googlesource.com/535382 for a WIP fix.

What is less clear-cut is how to clean up a similar warning for std::move(). We implement move assignment by swapping the contents of |this| with a temporary scoped_refptr. The static analyzer is unaware that the moved object that receives the movee's ptr_ is deleted post-move, hence the warning that its contents have leaked.

../../base/memory/ref_counted.h:515:5: warning: Potential leak of memory pointed to by field 'ptr_'
    return *this;
    ^
../../base/memory/ref_counted_unittest.cc:390:35: note: Memory is allocated
    ScopedRefPtrCountBase *raw1 = new ScopedRefPtrCountBase();
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/memory/ref_counted_unittest.cc:401:7: note: Calling move assignment operator for 'scoped_refptr'
      p1 = std::move(p2);
      ^~~~~~~~~~~~~~~~~~
../../base/memory/ref_counted.h:515:5: note: Potential leak of memory pointed to by field 'ptr_'
    return *this;
    ^

huh... is there a reason to do swap() here in liue of having a (possibly private) release function?

Also, I have no context for how the static analyzer works...but how is this distinction different from handling normal RAII in static analysis?

IIUC ref-counting will mean that the analyzer gets confused in two ways:

1. It can't be sure that we don't delete early, and use-after-free the object.

2. It can't be sure that we *ever* delete the object, since deletion of the container only corresponds to a dec-ref call, not to concrete deletion.

(1) is the issue that your WIP patch from #7 addresses, I think?

(2) seems to require that the static analyzer can effective confirm that there are as many dec-refs as inc-refs, which isn't possible in general, so perhaps we just need to annotate to disable leak analysis on the move path?

Disabling all analysis on the move path would prune a lot of valid paths too. 

I think we might be able to rewrite the assignment in a slightly more verbose way that allows us to prune just the ptr_ "leak" case?

OK I figured out why it's written this way.

This was a clever way of making sure the originally stored pointer's lifetime isn't extended. If it was just a simple pointer swap:
  scoped_refptr& operator=(scoped_refptr&& r) {
    swap(r);
    return *this;
  }

then the lifetime would be unexpectedly extended in an example like this:

void MyClass::StealPointer(std::vector<scoped_refptr<T>>* v) {
  // With a simple swap, |stolen_ptr_|'s original pointer's refcount
  // isn't released until the vector goes out of scope!
  stolen_ptr_ = std::move(v[0]);
}

That being said, this feels excessively subtle. We should probably just write it out explicitly like we do for copy assignment.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7273eddad8f51440a56c6305cddc8be484c3328f

commit 7273eddad8f51440a56c6305cddc8be484c3328f
Author: Kevin Marshall <kmarshall@chromium.org>
Date: Wed Jun 21 18:50:40 2017

Fix static analyzer noise in ref_counted.h

* Bad use-after-free warning: add static analysis directive to Release(),
  preventing the analyzer from following the deletion codepath.
* Bad pointer warning: swapping with a named variable instead of a temporary
  object fixes a bogus pointer leak error.


Bug: 731917
Change-Id: I2d495efa61b2b2bc30448dc79a4ecdc3a595f721
Reviewed-on: https://chromium-review.googlesource.com/535382
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481261}
[modify] https://crrev.com/7273eddad8f51440a56c6305cddc8be484c3328f/base/logging.h
[modify] https://crrev.com/7273eddad8f51440a56c6305cddc8be484c3328f/base/memory/ref_counted.h
[modify] https://crrev.com/7273eddad8f51440a56c6305cddc8be484c3328f/base/memory/ref_counted_unittest.cc

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot