Large heap collection type hits assertion in Heap::allocationSizeFromSize

Large heap collection type hits assertion in Heap::allocationSizeFromSize

allocationSizeFromSize is called for large object page allocations as well.

BUG=597953
Committed: https://crrev.com/57a75864e012b06db30f4b027e24f5fd758e46a3
Committed: https://crrev.com/908b160454bbbb283245b624a42a86afc34f518b
Cr-Original-Commit-Position: refs/heads/master@{#384230}
Cr-Commit-Position: refs/heads/master@{#400958}

[keishi, 2016-03-31 07:55:42]

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

[keishi, 2016-03-31 07:55:53]

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

[commit-bot: I haz the power, 2016-03-31 07:55:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842263004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1842263004/1

[keishi, 2016-03-31 07:58:27]

When you run the code below, the DOMTimerCoordinator::m_timers becomes so large
it hits the assertion.
       for (; ;) {
            setTimeout("document.bgColor='white'", 3000)
            setTimeout("document.bgColor='lightpink'", 3000)
            setTimeout("document.bgColor = 'pink'", 3500)
            setTimeout("document.bgColor =  'deeppink'", 3000)
            setTimeout("document.bgColor = 'red'", 3000)
            setTimeout("document.bgColor = 'tomato'", 3500)
            setTimeout("document.bgColor = 'darkred'", 4000)
        }

[haraken, 2016-03-31 08:00:41]

LGTM

https://codereview.chromium.org/1842263004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1842263004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.h:222: RELEASE_ASSERT(size <
maxHeapObjectSize);

Can we remove the maxHeapObjectSize definition?

https://codereview.chromium.org/1842263004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1842263004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.h:222: RELEASE_ASSERT(size <
std::numeric_limits<size_t>::max() - sizeof(HeapObjectHeader));

A better assert would be:

size_t allocationSize = size + sizeof(HeapObjectHeader);
RELEASE_ASSERT(allocationSize > size);

[haraken, 2016-03-31 08:01:38]

On 2016/03/31 07:58:27, keishi wrote:
> When you run the code below, the DOMTimerCoordinator::m_timers becomes so
large
> it hits the assertion.
>        for (; ;) {
>             setTimeout("document.bgColor='white'", 3000)
>             setTimeout("document.bgColor='lightpink'", 3000)
>             setTimeout("document.bgColor = 'pink'", 3500)
>             setTimeout("document.bgColor =  'deeppink'", 3000)
>             setTimeout("document.bgColor = 'red'", 3000)
>             setTimeout("document.bgColor = 'tomato'", 3500)
>             setTimeout("document.bgColor = 'darkred'", 4000)
>         }

That's crazy... you'll anyway hit OOM.

Either way this change looks good.

[keishi, 2016-03-31 08:07:39]

https://codereview.chromium.org/1842263004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1842263004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.h:222: RELEASE_ASSERT(size <
maxHeapObjectSize);
On 2016/03/31 08:00:41, haraken wrote:
> 
> Can we remove the maxHeapObjectSize definition?

HeapAllocator::quantizedSize is using it.

https://codereview.chromium.org/1842263004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1842263004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.h:222: RELEASE_ASSERT(size <
std::numeric_limits<size_t>::max() - sizeof(HeapObjectHeader));
On 2016/03/31 08:00:41, haraken wrote:
> 
> A better assert would be:
> 
> size_t allocationSize = size + sizeof(HeapObjectHeader);
> RELEASE_ASSERT(allocationSize > size);

Done.

[keishi, 2016-03-31 08:07:44]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-03-31 08:07:45]

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/1842263004/#ps20001
(title: "fixed")

[commit-bot: I haz the power, 2016-03-31 08:07:54]

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

[commit-bot: I haz the power, 2016-03-31 09:26:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-31 09:29:22]

Description was changed from

==========
Large heap collection type hits assertion in Heap::allocationSizeFromSize

allocationSizeFromSize is called for large object page allocations as well.

BUG=597953
==========

to

==========
Large heap collection type hits assertion in Heap::allocationSizeFromSize

allocationSizeFromSize is called for large object page allocations as well.

BUG=597953

Committed: https://crrev.com/57a75864e012b06db30f4b027e24f5fd758e46a3
Cr-Commit-Position: refs/heads/master@{#384230}
==========

[commit-bot: I haz the power, 2016-03-31 09:29:23]

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

[Rick Byers, 2016-04-05 21:17:54]

On 2016/03/31 09:29:23, commit-bot: I haz the power wrote:
> Patchset 2 (id:??) landed as
> https://crrev.com/57a75864e012b06db30f4b027e24f5fd758e46a3
> Cr-Commit-Position: refs/heads/master@{#384230}

FYI looks like this change has triggered a bunch of crashes on the perf bots:
https://bugs.chromium.org/p/chromium/issues/detail?id=600377

[keishi, 2016-04-06 01:33:41]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1864923002/ by keishi@chromium.org.

The reason for reverting is: Caused crash on perf bots.
crbug.com/600377.

[keishi, 2016-06-21 07:04:37]

Description was changed from

==========
Large heap collection type hits assertion in Heap::allocationSizeFromSize

allocationSizeFromSize is called for large object page allocations as well.

BUG=597953

Committed: https://crrev.com/57a75864e012b06db30f4b027e24f5fd758e46a3
Cr-Commit-Position: refs/heads/master@{#384230}
==========

to

==========
Large heap collection type hits assertion in Heap::allocationSizeFromSize

allocationSizeFromSize is called for large object page allocations as well.

BUG=597953

Committed: https://crrev.com/57a75864e012b06db30f4b027e24f5fd758e46a3
Cr-Commit-Position: refs/heads/master@{#384230}
==========

[keishi, 2016-06-21 07:05:12]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-06-21 07:05:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842263004/20001

[commit-bot: I haz the power, 2016-06-21 07:26:59]

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

[commit-bot: I haz the power, 2016-06-21 07:27:00]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[keishi, 2016-06-21 09:05:42]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-06-21 09:05:43]

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/1842263004/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-21 09:06:06]

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

[commit-bot: I haz the power, 2016-06-21 12:00:57]

Description was changed from

==========
Large heap collection type hits assertion in Heap::allocationSizeFromSize

allocationSizeFromSize is called for large object page allocations as well.

BUG=597953

Committed: https://crrev.com/57a75864e012b06db30f4b027e24f5fd758e46a3
Cr-Commit-Position: refs/heads/master@{#384230}
==========

to

==========
Large heap collection type hits assertion in Heap::allocationSizeFromSize

allocationSizeFromSize is called for large object page allocations as well.

BUG=597953

Committed: https://crrev.com/57a75864e012b06db30f4b027e24f5fd758e46a3
Cr-Commit-Position: refs/heads/master@{#384230}
==========

[commit-bot: I haz the power, 2016-06-21 12:00:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-21 12:03:11]

Description was changed from

==========
Large heap collection type hits assertion in Heap::allocationSizeFromSize

allocationSizeFromSize is called for large object page allocations as well.

BUG=597953

Committed: https://crrev.com/57a75864e012b06db30f4b027e24f5fd758e46a3
Cr-Commit-Position: refs/heads/master@{#384230}
==========

to

==========
Large heap collection type hits assertion in Heap::allocationSizeFromSize

allocationSizeFromSize is called for large object page allocations as well.

BUG=597953

Committed: https://crrev.com/57a75864e012b06db30f4b027e24f5fd758e46a3
Committed: https://crrev.com/908b160454bbbb283245b624a42a86afc34f518b
Cr-Original-Commit-Position: refs/heads/master@{#384230}
Cr-Commit-Position: refs/heads/master@{#400958}
==========

[commit-bot: I haz the power, 2016-06-21 12:03:12]

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