Disable collection backing reallocation during pre finalizer

Disable collection backing reallocation during pre finalization

If a collection backing is reallocated during pre finalization, the backing containing stale pointers ends up uncollected for one cycle. If conservative GC ends up tracing this buffer by accident, the stale pointers gets traced.

This CL disallows collection backing reallocation during pre finalizers. The backing size will end up being adjusted later.

BUG=709201
Review-Url: https://codereview.chromium.org/2815663002
Cr-Commit-Position: refs/heads/master@{#464359}
Committed: https://chromium.googlesource.com/chromium/src/+/0820c8882ec4691521bb7767877abc30123177d4

[keishi, 2017-04-12 05:53:52]

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

[commit-bot: I haz the power, 2017-04-12 05:54:06]

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

[keishi, 2017-04-12 06:06:44]

Description was changed from

==========
No realloc

BUG=
==========

to

==========
Disable collection backing reallocation during pre finalization

If a collection backing is reallocated during pre finalization, the backing
containing stale pointers ends up uncollected for one cycle. If conservative GC
ends up tracing this buffer by accident, the stale pointers gets traced.

This CL disallows collection backing reallocation during pre finalizers. The
backing size will end up being adjusted later.

BUG=709201
==========

[keishi, 2017-04-12 06:07:39]

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

[keishi, 2017-04-12 06:07:39]

PTAL

[kouhei (in TOK), 2017-04-12 06:14:00]

kouhei@chromium.org changed reviewers:
+ kouhei@chromium.org

[kouhei (in TOK), 2017-04-12 06:14:01]

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right):

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapAllocator.h:154: static bool
IsBackingReallocationAllowed() {
Can we make it clear that this is inside prefinalizer so that we can use the
predicate in DCHECKs to assert certain methods are not called from prefinalizer.

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

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1271:
NoBackingReallocationScope no_backing_reallocation(this);
How about just using platform/wtf/AutoReset.h?

[haraken, 2017-04-12 06:44:34]

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right):

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapAllocator.h:154: static bool
IsBackingReallocationAllowed() {
On 2017/04/12 06:14:01, kouhei wrote:
> Can we make it clear that this is inside prefinalizer so that we can use the
> predicate in DCHECKs to assert certain methods are not called from
prefinalizer.

Maybe we can rename this to IsObjectRessurectionForbidden.

I cannot come up with a better name...

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

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1271:
NoBackingReallocationScope no_backing_reallocation(this);
On 2017/04/12 06:14:01, kouhei wrote:
> How about just using platform/wtf/AutoReset.h?

I'd prefer using a scope since ThreadState is already using a bunch of scopes.

You also need to add the scope to the weak processing.

Nit: Move the scope to below ScriptForbiddenIfMainThreadScope.
SweepForbiddenScope and ScriptForbiddenIfMainThreadScope should be coupled.

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.h:280: void
EnterNoBackingReallocationScope() { no_backing_reallocation_ = true; }

Add DCHECK(!no_backing_reallocation_).

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.h:281: void
LeaveNoBackingReallocationScope() { no_backing_reallocation_ = false; }

Add DCHECK(no_backing_reallocation_).

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/wtf/HashTable.h (right):

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/wtf/HashTable.h:840:
Allocator::IsAllocationAllowed();

Do you know what the IsAllocationAllowed check is for?

[keishi, 2017-04-12 06:50:17]

Patchset #3 (id:40001) has been deleted

[keishi, 2017-04-12 07:50:04]

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

[commit-bot: I haz the power, 2017-04-12 07:50:35]

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

[keishi, 2017-04-12 09:43:54]

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right):

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapAllocator.h:154: static bool
IsBackingReallocationAllowed() {
On 2017/04/12 06:44:34, haraken wrote:
> On 2017/04/12 06:14:01, kouhei wrote:
> > Can we make it clear that this is inside prefinalizer so that we can use the
> > predicate in DCHECKs to assert certain methods are not called from
> prefinalizer.
> 
> Maybe we can rename this to IsObjectRessurectionForbidden.
> 
> I cannot come up with a better name...

Done.

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

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1271:
NoBackingReallocationScope no_backing_reallocation(this);
On 2017/04/12 06:44:34, haraken wrote:
> On 2017/04/12 06:14:01, kouhei wrote:
> > How about just using platform/wtf/AutoReset.h?
> 
> I'd prefer using a scope since ThreadState is already using a bunch of scopes.
> 
> You also need to add the scope to the weak processing.
Weak processing is already in the NoAllocationScope. Since I reused
Allocator::isAllocationAllowed, we don't need to add
ObjectResurrectionForbiddenScope. Should I still add it.

> Nit: Move the scope to below ScriptForbiddenIfMainThreadScope.
> SweepForbiddenScope and ScriptForbiddenIfMainThreadScope should be coupled.

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.h:280: void
EnterNoBackingReallocationScope() { no_backing_reallocation_ = true; }
On 2017/04/12 06:44:34, haraken wrote:
> 
> Add DCHECK(!no_backing_reallocation_).

Done.

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.h:281: void
LeaveNoBackingReallocationScope() { no_backing_reallocation_ = false; }
On 2017/04/12 06:44:34, haraken wrote:
> 
> Add DCHECK(no_backing_reallocation_).

Done.

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/wtf/HashTable.h (right):

https://codereview.chromium.org/2815663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/wtf/HashTable.h:840:
Allocator::IsAllocationAllowed();
On 2017/04/12 06:44:34, haraken wrote:
> 
> Do you know what the IsAllocationAllowed check is for?

Used IsAllocationAllowed instead of adding a new flag.

[kouhei (in TOK), 2017-04-12 09:49:28]

lgtm

[haraken, 2017-04-12 11:03:37]

LGTM

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

https://codereview.chromium.org/2815663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapAllocator.h:152:
!ThreadState::Current()->IsObjectRessurectionForbidden();

Hmm, it looks confusing to change the meaning of IsAllocationAllowed here. I'd
prefer separating the two methods.

I'm wondering if we can remove NoAllocationScope somehow. For example, the
NoAllocationScope in CollectGarbage wouldn't be needed since we can check if
we're in a GC or not by isInGC().

https://codereview.chromium.org/2815663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/2815663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1272:
ObjectRessurectionForbiddenScope object_ressurection_forbidden(this);

Add a comment.

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

https://codereview.chromium.org/2815663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.h:161: class
ObjectRessurectionForbiddenScope final {

Add a class-level comment.

[commit-bot: I haz the power, 2017-04-12 11:42:41]

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

[commit-bot: I haz the power, 2017-04-12 11:42:41]

Dry run: This issue passed the CQ dry run.

[keishi, 2017-04-13 05:22:43]

Added backing expand death test.

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

https://codereview.chromium.org/2815663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapAllocator.h:152:
!ThreadState::Current()->IsObjectRessurectionForbidden();
On 2017/04/12 11:03:37, haraken wrote:
> 
> Hmm, it looks confusing to change the meaning of IsAllocationAllowed here. I'd
> prefer separating the two methods.
> 
> I'm wondering if we can remove NoAllocationScope somehow. For example, the
> NoAllocationScope in CollectGarbage wouldn't be needed since we can check if
> we're in a GC or not by isInGC().

I added a separate method on Allocator.

Looks like we may able to remove it but leaving NoAllocationScope as is for this
CL.

https://codereview.chromium.org/2815663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/2815663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1272:
ObjectRessurectionForbiddenScope object_ressurection_forbidden(this);
On 2017/04/12 11:03:37, haraken wrote:
> 
> Add a comment.

Done.

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

https://codereview.chromium.org/2815663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.h:161: class
ObjectRessurectionForbiddenScope final {
On 2017/04/12 11:03:37, haraken wrote:
> 
> Add a class-level comment.

Done.

[haraken, 2017-04-13 06:45:32]

LGTM

[keishi, 2017-04-13 06:56:14]

The CQ bit was checked by keishi@chromium.org

[keishi, 2017-04-13 06:56:15]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2815663002/#ps100001
(title: "fix")

[commit-bot: I haz the power, 2017-04-13 06:56:31]

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, 2017-04-13 08:37:18]

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

[commit-bot: I haz the power, 2017-04-13 08:37:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[keishi, 2017-04-13 09:06:39]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2017-04-13 09:07:00]

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, 2017-04-13 09:37:11]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492074399790640,
"parent_rev": "4074f42f1c8360b2f429549b8819f4d9f604ebd4", "commit_rev":
"0820c8882ec4691521bb7767877abc30123177d4"}

[commit-bot: I haz the power, 2017-04-13 09:38:04]

Description was changed from

==========
Disable collection backing reallocation during pre finalization

If a collection backing is reallocated during pre finalization, the backing
containing stale pointers ends up uncollected for one cycle. If conservative GC
ends up tracing this buffer by accident, the stale pointers gets traced.

This CL disallows collection backing reallocation during pre finalizers. The
backing size will end up being adjusted later.

BUG=709201
==========

to

==========
Disable collection backing reallocation during pre finalization

If a collection backing is reallocated during pre finalization, the backing
containing stale pointers ends up uncollected for one cycle. If conservative GC
ends up tracing this buffer by accident, the stale pointers gets traced.

This CL disallows collection backing reallocation during pre finalizers. The
backing size will end up being adjusted later.

BUG=709201

Review-Url: https://codereview.chromium.org/2815663002
Cr-Commit-Position: refs/heads/master@{#464359}
Committed:
https://chromium.googlesource.com/chromium/src/+/0820c8882ec4691521bb7767877a...
==========

[commit-bot: I haz the power, 2017-04-13 09:38:06]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0820c8882ec4691521bb7767877a...

[sof, 2017-04-15 18:57:30]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2017-04-15 18:57:31]

https://codereview.chromium.org/2815663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/wtf/HashTable.h (right):

https://codereview.chromium.org/2815663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/wtf/HashTable.h:839:
!Allocator::IsObjectResurrectionForbidden() &&
Isn't this redundant (as IsAllocationAllowed() also checks)?

https://codereview.chromium.org/2815663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/wtf/Vector.h (right):

https://codereview.chromium.org/2815663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/wtf/Vector.h:1629: if
(Allocator::IsObjectResurrectionForbidden())
Why leave early for the new_capacity == 0 case?

[keishi, 2017-04-17 06:37:55]

https://codereview.chromium.org/2815663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/wtf/HashTable.h (right):

https://codereview.chromium.org/2815663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/wtf/HashTable.h:839:
!Allocator::IsObjectResurrectionForbidden() &&
On 2017/04/15 18:57:31, sof wrote:
> Isn't this redundant (as IsAllocationAllowed() also checks)?

Oops I modified HeapAllocator::IsAllocationAllowed by mistake. Made 
https://codereview.chromium.org/2818413002/

https://codereview.chromium.org/2815663002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/wtf/Vector.h (right):

https://codereview.chromium.org/2815663002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/wtf/Vector.h:1629: if
(Allocator::IsObjectResurrectionForbidden())
On 2017/04/15 18:57:31, sof wrote:
> Why leave early for the new_capacity == 0 case?

Created fix at https://codereview.chromium.org/2816373003/