Remove scoped_refptr::release which is confusing and causes leaks.

Remove scoped_refptr::release which is confusing and causes leaks.

This fixes one leak in production and one leak in tests. The rest of the changes
now show an explicit AddRef/Release pair, instead of a .release combined with
a ->Release().

BUG=None


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171893

[michaeln, 2012-12-06 00:26:11]

I didn't look at all of the changes, but as coded right now, looks like this CL
would introduce some subtle bugs.

https://codereview.chromium.org/11458003/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right):

https://codereview.chromium.org/11458003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/page_capture/page_capture_api.cc:49:
mhtml_file_.get());
This pattern is buggy.

You should also clear out the ref held in mhtml_file_ prior to invoking
ReleaseSoon. That release task may run on the target thread prior to this thread
making progress. When that happens the final release will occur on the wrong
thread.

https://codereview.chromium.org/11458003/diff/7001/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_context.cc (right):

https://codereview.chromium.org/11458003/diff/7001/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:50:
session_storage_database_->AddRef();
Same raciness here as mentioned before.

[piman, 2012-12-06 04:56:27]

Note, I will upload separate CLs for the cleanup in each of the subsystems
(based on this one). Let me know if you want to review any.

https://codereview.chromium.org/11458003/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right):

https://codereview.chromium.org/11458003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/page_capture/page_capture_api.cc:49:
mhtml_file_.get());
On 2012/12/06 00:26:12, michaeln wrote:
> This pattern is buggy.
> 
> You should also clear out the ref held in mhtml_file_ prior to invoking
> ReleaseSoon. That release task may run on the target thread prior to this
thread
> making progress. When that happens the final release will occur on the wrong
> thread.

You're right, done. I verified that the other changes are not subject to this
caveat (mostly because they call release on a local variable, that isn't visible
from other threads).

Note however that the new version, no more and no less than the new version, has
any way to ensure that some other thing (e.g. a posted task) doesn't have a
separate reference to that object.

https://codereview.chromium.org/11458003/diff/7001/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_context.cc (right):

https://codereview.chromium.org/11458003/diff/7001/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context.cc:50:
session_storage_database_->AddRef();
On 2012/12/06 00:26:12, michaeln wrote:
> Same raciness here as mentioned before.

Done.

[danakj, 2012-12-06 05:00:08]

https://codereview.chromium.org/11458003/diff/5031/cc/occlusion_tracker_unitt...
File cc/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/11458003/diff/5031/cc/occlusion_tracker_unitt...
cc/occlusion_tracker_unittest.cc:138: layer = NULL;
The NULL assignment seems unneeded. The callsite will hold the ref and release
it without this. Can't this just "return LayerPtrType(layer);"?

[piman, 2012-12-06 05:15:59]

https://codereview.chromium.org/11458003/diff/5031/cc/occlusion_tracker_unitt...
File cc/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/11458003/diff/5031/cc/occlusion_tracker_unitt...
cc/occlusion_tracker_unittest.cc:138: layer = NULL;
On 2012/12/06 05:00:09, danakj wrote:
> The NULL assignment seems unneeded. The callsite will hold the ref and release
> it without this. Can't this just "return LayerPtrType(layer);"?

I wanted to keep the semantics that it cleared the passed parameter. This is
what happens on the impl thread versions as well. But you're right that the
current call sites don't rely on it. I don't feel strongly either way.

[piman, 2012-12-06 21:21:44]

Moved changes that remove uses of ::release into separate CLs to avoid O(n^2)
reviews.
This is the final one that removes the method.

[brettw, 2012-12-07 20:05:36]

lgtm

[commit-bot: I haz the power, 2012-12-07 21:00:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/11458003/15001

[commit-bot: I haz the power, 2012-12-08 00:11:05]

Change committed as 171893