Remove CrossThreadPersistent::release

Remove CrossThreadPersistent::release

Calling CrossThreadPersistent::release was causing WorkerClients to be destroyed prematurely.
The stack for per thread heap enabled threads are invisible from other threads running GC. Using CrossThreadPersistent::release runs the risk of premature collection so we should delete it.

BUG=652001, 653272, 652458, 653657, 652556, 653124, 651790
Committed: https://crrev.com/fb7d72c8ec38f54f84c226c07c4c980283687d17
Cr-Commit-Position: refs/heads/master@{#424688}

[keishi, 2016-10-11 15:18:49]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[keishi, 2016-10-11 15:18:49]

During the last memory meeting I talked about getting ASAN crashes inside
CrossThreadPersistentRegion::shouldTracePersistentNode (like this report 
https://cluster-fuzz.appspot.com/testcase?key=4966855049740288) but I can no
longer reproduce it today on ToT.

PersistentBase::get should be inlined so I don't see why I was getting those
crashes. I think we can re-enable per thread heap when this
CrossThreadPersistent::release issue is solved.

[haraken, 2016-10-11 15:40:32]

LGTM

However, I don't think CrossThreadPersistent::release itself is a bad thing.
CrossThreadPersistent::release works fine as long as it's called after all other
pointers to the objects are gone. The problem here is just that
workerClients.release() is called prematurely before all other pointers are
gone. Am I understanding things correctly?

[keishi, 2016-10-12 01:20:34]

On 2016/10/11 15:40:32, haraken wrote:
> LGTM
> 
> However, I don't think CrossThreadPersistent::release itself is a bad thing.
> CrossThreadPersistent::release works fine as long as it's called after all
other
> pointers to the objects are gone. The problem here is just that
> workerClients.release() is called prematurely before all other pointers are
> gone. Am I understanding things correctly?

Yes. But I think its easy to make this mistake with release(). I thought it
would be better to remove release() to force the coder to assign to another
handle, then call CrossThreadPersistent::clear().

[haraken, 2016-10-12 01:37:12]

On 2016/10/12 01:20:34, keishi wrote:
> On 2016/10/11 15:40:32, haraken wrote:
> > LGTM
> > 
> > However, I don't think CrossThreadPersistent::release itself is a bad thing.
> > CrossThreadPersistent::release works fine as long as it's called after all
> other
> > pointers to the objects are gone. The problem here is just that
> > workerClients.release() is called prematurely before all other pointers are
> > gone. Am I understanding things correctly?
> 
> Yes. But I think its easy to make this mistake with release(). I thought it
> would be better to remove release() to force the coder to assign to another
> handle, then call CrossThreadPersistent::clear().

Ah, that totally makes sense.

Add a comment?

[keishi, 2016-10-12 03:01:00]

On 2016/10/12 01:37:12, haraken wrote:
> On 2016/10/12 01:20:34, keishi wrote:
> > On 2016/10/11 15:40:32, haraken wrote:
> > > LGTM
> > > 
> > > However, I don't think CrossThreadPersistent::release itself is a bad
thing.
> > > CrossThreadPersistent::release works fine as long as it's called after all
> > other
> > > pointers to the objects are gone. The problem here is just that
> > > workerClients.release() is called prematurely before all other pointers
are
> > > gone. Am I understanding things correctly?
> > 
> > Yes. But I think its easy to make this mistake with release(). I thought it
> > would be better to remove release() to force the coder to assign to another
> > handle, then call CrossThreadPersistent::clear().
> 
> Ah, that totally makes sense.
> 
> Add a comment?

Done.

[keishi, 2016-10-12 03:01:07]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-10-12 03:01:08]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2406333002/#ps20001
(title: "added comment")

[commit-bot: I haz the power, 2016-10-12 03:01:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 07:27:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-12 07:29:41]

Description was changed from

==========
Remove CrossThreadPersistent::release

Calling CrossThreadPersistent::release was causing WorkerClients to be destroyed
prematurely.
The stack for per thread heap enabled threads are invisible from other threads
running GC. Using CrossThreadPersistent::release runs the risk of premature
collection so we should delete it.

BUG=652001,653272,652458,653657,652556,653124,651790
==========

to

==========
Remove CrossThreadPersistent::release

Calling CrossThreadPersistent::release was causing WorkerClients to be destroyed
prematurely.
The stack for per thread heap enabled threads are invisible from other threads
running GC. Using CrossThreadPersistent::release runs the risk of premature
collection so we should delete it.

BUG=652001,653272,652458,653657,652556,653124,651790

Committed: https://crrev.com/fb7d72c8ec38f54f84c226c07c4c980283687d17
Cr-Commit-Position: refs/heads/master@{#424688}
==========

[commit-bot: I haz the power, 2016-10-12 07:29:42]

Patchset 2 (id:??) landed as
https://crrev.com/fb7d72c8ec38f54f84c226c07c4c980283687d17
Cr-Commit-Position: refs/heads/master@{#424688}