Fix ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Fix ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=661363
Committed: https://crrev.com/92cc84f200b7839f220a26633f1ba8983ea361aa
Cr-Commit-Position: refs/heads/master@{#429530}

[chcunningham, 2016-11-01 23:19:52]

Description was changed from

==========
ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=
==========

to

==========
ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=
==========

[chcunningham, 2016-11-01 23:20:05]

Description was changed from

==========
ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=
==========

to

==========
Fix ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=
==========

[chcunningham, 2016-11-01 23:23:05]

Description was changed from

==========
Fix ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=
==========

to

==========
Fix ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=661363
==========

[chcunningham, 2016-11-01 23:23:05]

chcunningham@chromium.org changed reviewers:
+ haraken@chromium.org

[chcunningham, 2016-11-01 23:26:05]

https://codereview.chromium.org/2464273002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/2464273002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapTest.cpp:3932: // {
I'd like to test the isHeapObjectAlive = TRUE case here, but I'm not setting it
up right. If I uncomment this, the test failes at the last two EXPECT_TRUE
calls. Please advise

[chcunningham, 2016-11-01 23:26:29]

The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 23:27:05]

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

[chcunningham, 2016-11-02 00:11:30]

The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 00:12:08]

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

[haraken, 2016-11-02 01:12:47]

LGTM

https://codereview.chromium.org/2464273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/2464273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapTest.cpp:3942: // }

Can we remove this comment? It's not worth adding comment-outed code.

[chcunningham, 2016-11-02 01:19:06]

https://codereview.chromium.org/2464273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/2464273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapTest.cpp:3942: // }
On 2016/11/02 01:12:47, haraken wrote:
> 
> Can we remove this comment? It's not worth adding comment-outed code.

Sorry, I mean to uncomment it actually, but it doesn't pass. The stack overflow
problem is definitely solved, but for some reason the checkAndMarkPointer does
not cause the later isHeapObjectAlive calls to return true. Can you point out
what I'm missing? Is there some better way to mark the object alive?

[haraken, 2016-11-02 01:22:30]

https://codereview.chromium.org/2464273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/2464273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapTest.cpp:3942: // }
On 2016/11/02 01:19:06, chcunningham wrote:
> On 2016/11/02 01:12:47, haraken wrote:
> > 
> > Can we remove this comment? It's not worth adding comment-outed code.
> 
> Sorry, I mean to uncomment it actually, but it doesn't pass. The stack
overflow
> problem is definitely solved, but for some reason the checkAndMarkPointer does
> not cause the later isHeapObjectAlive calls to return true. Can you point out
> what I'm missing? Is there some better way to mark the object alive?

Ah, actually this test doesn't make sense in the first place because
isHeapObjectAlive can be used only during a GC phase.

Instead of adding this test, can you add EXPECT_TRUE/FALSE to ClassWithMember?

[commit-bot: I haz the power, 2016-11-02 01:55:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 01:55:25]

Dry run: This issue passed the CQ dry run.

[chcunningham, 2016-11-02 19:18:09]

The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 19:18:53]

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

[chcunningham, 2016-11-02 19:20:09]

On 2016/11/02 01:22:30, haraken wrote:
>
https://codereview.chromium.org/2464273002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right):
> 
>
https://codereview.chromium.org/2464273002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/heap/HeapTest.cpp:3942: // }
> On 2016/11/02 01:19:06, chcunningham wrote:
> > On 2016/11/02 01:12:47, haraken wrote:
> > > 
> > > Can we remove this comment? It's not worth adding comment-outed code.
> > 
> > Sorry, I mean to uncomment it actually, but it doesn't pass. The stack
> overflow
> > problem is definitely solved, but for some reason the checkAndMarkPointer
does
> > not cause the later isHeapObjectAlive calls to return true. Can you point
out
> > what I'm missing? Is there some better way to mark the object alive?
> 
> Ah, actually this test doesn't make sense in the first place because
> isHeapObjectAlive can be used only during a GC phase.
> 
> Instead of adding this test, can you add EXPECT_TRUE/FALSE to ClassWithMember?

How's this? Doesn't test the FALSE case, but in hindsight thats not really
important to verify the fix.

[commit-bot: I haz the power, 2016-11-02 20:47:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 20:47:33]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-11-03 02:43:42]

On 2016/11/02 19:20:09, chcunningham wrote:
> On 2016/11/02 01:22:30, haraken wrote:
> >
>
https://codereview.chromium.org/2464273002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2464273002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/heap/HeapTest.cpp:3942: // }
> > On 2016/11/02 01:19:06, chcunningham wrote:
> > > On 2016/11/02 01:12:47, haraken wrote:
> > > > 
> > > > Can we remove this comment? It's not worth adding comment-outed code.
> > > 
> > > Sorry, I mean to uncomment it actually, but it doesn't pass. The stack
> > overflow
> > > problem is definitely solved, but for some reason the checkAndMarkPointer
> does
> > > not cause the later isHeapObjectAlive calls to return true. Can you point
> out
> > > what I'm missing? Is there some better way to mark the object alive?
> > 
> > Ah, actually this test doesn't make sense in the first place because
> > isHeapObjectAlive can be used only during a GC phase.
> > 
> > Instead of adding this test, can you add EXPECT_TRUE/FALSE to
ClassWithMember?
> 
> How's this? Doesn't test the FALSE case, but in hindsight thats not really
> important to verify the fix.

Yeah, it seems hard to write a test for this. I'm okay with landing the fix
without a test.

[chcunningham, 2016-11-03 05:41:06]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2016-11-03 05:41:07]

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/2464273002/#ps40001
(title: "Fix test.")

[commit-bot: I haz the power, 2016-11-03 05:41:26]

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-11-03 05:45:57]

Description was changed from

==========
Fix ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=661363
==========

to

==========
Fix ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=661363
==========

[commit-bot: I haz the power, 2016-11-03 05:45:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-03 05:48:15]

Description was changed from

==========
Fix ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=661363
==========

to

==========
Fix ThreadHeap::isHeapObjectAlive(const T*& ptr) stack overflow.

Prior to this change, the method simply called itself. On debug builds
this manifests as a stack overflow. On release builds the assembly
optimizations result in a jump instruction that jumps to itself - render
proc will appear hung.

TEST=new blink_heap_unittest
BUG=661363

Committed: https://crrev.com/92cc84f200b7839f220a26633f1ba8983ea361aa
Cr-Commit-Position: refs/heads/master@{#429530}
==========

[commit-bot: I haz the power, 2016-11-03 05:48:16]

Patchset 3 (id:??) landed as
https://crrev.com/92cc84f200b7839f220a26633f1ba8983ea361aa
Cr-Commit-Position: refs/heads/master@{#429530}