Invalidate cross-thread persistents on heap termination. (issue 1265103003 by sigbjornf@opera.com)

[sigb...@opera.com]

Reviewers: oilpan-reviews,

Message:
please take a look.

If you accept the premise that CTPs can live longer than the objects they
refer
to (due to thread heap shutdown), then something like this seems in order to
make that safe.

I haven't invested much time in optimizing the invalidation pass, for now.

Description:
Invalidate cross-thread persistents on heap termination.

R=
BUG=515432

Please review this at https://codereview.chromium.org/1265103003/

Base URL: svn://svn.chromium.org/blink/trunk

Affected files (+99, -3 lines):
M Source/platform/heap/HeapTest.cpp
M Source/platform/heap/PersistentNode.h
M Source/platform/heap/PersistentNode.cpp
M Source/platform/heap/ThreadState.cpp

[har...@chromium.org]

Thanks for working on this! Mostly LG.

(I think this will be a good step to make our heaps per-thread. I need to
write
a design doc though...)



https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/PersistentNode.cpp
File Source/platform/heap/PersistentNode.cpp (right):

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/PersistentNode.cpp#newcode68
Source/platform/heap/PersistentNode.cpp:68: // This to allow an Oilpan
thread to shut down and terminate

This is to allow

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/PersistentNode.cpp#newcode129
Source/platform/heap/PersistentNode.cpp:129: // into the heaps of each
ThreadState & use that count to terminate traversal.)

Given that the number of CrossThreadPersistents should be limited, I'm
not worried about the overhead very much.

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/PersistentNode.h
File Source/platform/heap/PersistentNode.h (right):

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/PersistentNode.h#newcode88
Source/platform/heap/PersistentNode.h:88: m_self =
reinterpret_cast<Address>(reinterpret_cast<uintptr_t>(m_self) | 0x1);

Or I'd prefer just clearing the pointer. The semantics will be:

- CrossThreadPersistent can outlive the thread that has allocated the
pointing object. The outlived CrossThreadPersistent is cleared out when
the owner thread gets detached.

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/ThreadState.cpp
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/ThreadState.cpp#newcode244
Source/platform/heap/ThreadState.cpp:244: prepareHeapForTermination();

Nit: I'd rename this to prepareForThreadStateTermination().

https://codereview.chromium.org/1265103003/

[sigb...@opera.com]

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/PersistentNode.cpp
File Source/platform/heap/PersistentNode.cpp (right):

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/PersistentNode.cpp#newcode129
Source/platform/heap/PersistentNode.cpp:129: // into the heaps of each
ThreadState & use that count to terminate traversal.)

On 2015/08/03 00:45:25, haraken wrote:

> Given that the number of CrossThreadPersistents should be limited, I'm
not
> worried about the overhead very much.

I'd like to revisit and address, but thread local shutdowns aren't that
plentiful to boot.

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/PersistentNode.h
File Source/platform/heap/PersistentNode.h (right):

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/PersistentNode.h#newcode88
Source/platform/heap/PersistentNode.h:88: m_self =
reinterpret_cast<Address>(reinterpret_cast<uintptr_t>(m_self) | 0x1);

On 2015/08/03 00:45:25, haraken wrote:

> Or I'd prefer just clearing the pointer. The semantics will be:

> - CrossThreadPersistent can outlive the thread that has allocated the
pointing
> object. The outlived CrossThreadPersistent is cleared out when the
owner thread
> gets detached.

Can do that instead, that is, clear out the underlying raw pointer of
the m_self here.

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/ThreadState.cpp
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1265103003/diff/20001/Source/platform/heap/ThreadState.cpp#newcode244
Source/platform/heap/ThreadState.cpp:244: prepareHeapForTermination();

On 2015/08/03 00:45:25, haraken wrote:

> Nit: I'd rename this to prepareForThreadStateTermination().

Done.

https://codereview.chromium.org/1265103003/

[har...@chromium.org]

LGTM


https://codereview.chromium.org/1265103003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1265103003/40001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1265103003/40001


https://codereview.chromium.org/1265103003/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199891

https://codereview.chromium.org/1265103003/