Add checks for per thread heap

Add checks for per thread heap

A GC should only mark the objects that belong in its heap.
visitStack should only mark the objects that belong in its heap.
isHeapObjectAlive will only work if the object belongs to the current GCing thread's heap.

BUG=591606
Committed: https://crrev.com/fcd2684d99a74875a8d6444015362692ee6b08fd
Cr-Commit-Position: refs/heads/master@{#396379}

[keishi, 2016-05-25 12:55:53]

Description was changed from

==========
Add checks for per thread heap

BUG=591606
==========

to

==========
Add checks for per thread heap

A GC should only mark the objects that belong in its heap.
visitStack should only mark the objects that belong in its heap.
isHeapObjectAlive will only work if the object belongs to the current GCing
thread's heap.

BUG=591606
==========

[keishi, 2016-05-25 13:09:11]

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

[keishi, 2016-05-25 13:09:11]

PTAL

Part of:

1. This. Add checks for per thread heap
https://codereview.chromium.org/2012763002/

2. Allow CrossThreadPersistent in collections
https://codereview.chromium.org/2007283002/

3. Enable per thread heap for database thread
https://codereview.chromium.org/1909813002/

[haraken, 2016-05-25 15:41:22]

https://codereview.chromium.org/2012763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2012763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:209: return true;

Would you help me understand why we need to add this code? Returning true means
that if a thread calls isHeapObjectAlive(object) for an object that is dead in
another thread's heap, it returns true. I don't fully understand what it
means...?

[keishi, 2016-05-26 04:29:39]

https://codereview.chromium.org/2012763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2012763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:209: return true;
On 2016/05/25 15:41:22, haraken wrote:
> 
> Would you help me understand why we need to add this code? Returning true
means
> that if a thread calls isHeapObjectAlive(object) for an object that is dead in
> another thread's heap, it returns true. I don't fully understand what it
> means...?
> 

Added this while debugging but it looks like it wasn't necessary. Removed.

[keishi, 2016-05-26 04:33:02]

https://codereview.chromium.org/2012763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2012763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:205: // TODO(keishi): some tests
create CrossThreadPersistent on non attached threads.
The tests that require this are:
ExtensionApiTest.TabAudible
ScriptStreamingTest.EncodingFromBOM
ScriptStreamingTest.EncodingChanges
ScriptStreamingTest.SuppressingStreaming
ScriptStreamingTest.CompilingStreamedScriptWithParseError
ScriptStreamingTest.ScriptsWithSmallFirstChunk
ScriptStreamingTest.CancellingStreaming
ScriptStreamingTest.CompilingStreamedScript

[haraken, 2016-05-26 07:52:29]

https://codereview.chromium.org/2012763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2012763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:205: // TODO(keishi): some tests
create CrossThreadPersistent on non attached threads.
On 2016/05/26 04:33:02, keishi wrote:
> The tests that require this are:
> ExtensionApiTest.TabAudible
> ScriptStreamingTest.EncodingFromBOM
> ScriptStreamingTest.EncodingChanges
> ScriptStreamingTest.SuppressingStreaming
> ScriptStreamingTest.CompilingStreamedScriptWithParseError
> ScriptStreamingTest.ScriptsWithSmallFirstChunk
> ScriptStreamingTest.CancellingStreaming
> ScriptStreamingTest.CompilingStreamedScript

Conceptually it should be valid to support CrossThreadPersistent created on a
not-attached thread, so there is no reason we must fix the TODO. But if it
happens only in testing, I'd prefer fixing the test and removing the TODO (in a
follow up CL).

https://codereview.chromium.org/2012763002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2012763002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:209: return true;

Hmm, you still have this code?

[keishi, 2016-05-26 11:52:54]

https://codereview.chromium.org/2012763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2012763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:205: // TODO(keishi): some tests
create CrossThreadPersistent on non attached threads.
On 2016/05/26 07:52:29, haraken wrote:
> On 2016/05/26 04:33:02, keishi wrote:
> > The tests that require this are:
> > ExtensionApiTest.TabAudible
> > ScriptStreamingTest.EncodingFromBOM
> > ScriptStreamingTest.EncodingChanges
> > ScriptStreamingTest.SuppressingStreaming
> > ScriptStreamingTest.CompilingStreamedScriptWithParseError
> > ScriptStreamingTest.ScriptsWithSmallFirstChunk
> > ScriptStreamingTest.CancellingStreaming
> > ScriptStreamingTest.CompilingStreamedScript
> 
> Conceptually it should be valid to support CrossThreadPersistent created on a
> not-attached thread, so there is no reason we must fix the TODO. But if it
> happens only in testing, I'd prefer fixing the test and removing the TODO (in
a
> follow up CL).

OK. Leaving the TODO comment.

https://codereview.chromium.org/2012763002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2012763002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:209: return true;
On 2016/05/26 07:52:29, haraken wrote:
> 
> Hmm, you still have this code?

Sorry, probably made a mistake with git.

I've changed it to a DCHECK so developers using isHeapObjectAlive in
clearWeakMembers will notice if they use it on a cross thread object.

[haraken, 2016-05-26 15:50:33]

LGTM

[keishi, 2016-05-27 01:44:49]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-05-27 01:45:15]

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

[commit-bot: I haz the power, 2016-05-27 04:32:39]

Description was changed from

==========
Add checks for per thread heap

A GC should only mark the objects that belong in its heap.
visitStack should only mark the objects that belong in its heap.
isHeapObjectAlive will only work if the object belongs to the current GCing
thread's heap.

BUG=591606
==========

to

==========
Add checks for per thread heap

A GC should only mark the objects that belong in its heap.
visitStack should only mark the objects that belong in its heap.
isHeapObjectAlive will only work if the object belongs to the current GCing
thread's heap.

BUG=591606
==========

[commit-bot: I haz the power, 2016-05-27 04:32:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-27 04:33:57]

Description was changed from

==========
Add checks for per thread heap

A GC should only mark the objects that belong in its heap.
visitStack should only mark the objects that belong in its heap.
isHeapObjectAlive will only work if the object belongs to the current GCing
thread's heap.

BUG=591606
==========

to

==========
Add checks for per thread heap

A GC should only mark the objects that belong in its heap.
visitStack should only mark the objects that belong in its heap.
isHeapObjectAlive will only work if the object belongs to the current GCing
thread's heap.

BUG=591606

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

[commit-bot: I haz the power, 2016-05-27 04:33:58]

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