Fix assert in Page::Initialize. (issue 11275229)

[ul...@chromium.org]

Reviewers: Michael Starzinger,

Message:
Please take a look. This assertion hits in long running Google Drive
extension
in 64-bit Chrome.

Description:
Fix assert in Page::Initialize.

R=mstar...@chromium.org

Please review this at https://chromiumcodereview.appspot.com/11275229/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/spaces-inl.h


Index: src/spaces-inl.h
diff --git a/src/spaces-inl.h b/src/spaces-inl.h
index
8a576a83f606cf2adb314237fbbdca969742436b..9775e5d9427cef0a35754969fc6fbc3bb18c1da3
100644
--- a/src/spaces-inl.h
+++ b/src/spaces-inl.h
@@ -164,7 +164,10 @@ Page* Page::Initialize(Heap* heap,
Executability executable,
PagedSpace* owner) {
Page* page = reinterpret_cast<Page*>(chunk);
- ASSERT(chunk->size() <= static_cast<size_t>(kPageSize));
+ ASSERT(page->area_size() <= kPageSize);
+ // Code range allocation can return chunks larger than a page.
+ ASSERT(chunk->size() <= static_cast<size_t>(kPageSize) ||
+ executable == EXECUTABLE);
ASSERT(chunk->owner() == owner);
owner->IncreaseCapacity(page->area_size());
owner->Free(page->area_start(), page->area_size());

[ul...@chromium.org]

For the record, patch that reproduces assertion hit (not adding it to the
CL):

diff --git a/src/spaces.h b/src/spaces.h
index 9121e9c..be49a8a 100644
--- a/src/spaces.h
+++ b/src/spaces.h
@@ -852,6 +852,11 @@ class CodeRange {
return start <= address && address < start + code_range_->size();
}

+ size_t current_allocation_block_size() {
+ if (allocation_list_.length() == 0) return 0;
+ return allocation_list_[current_allocation_block_index_].size;
+ }
+
// Allocates a chunk of memory from the large-object portion of
// the code range. On platforms with no separate code range, should
// not be called.
diff --git a/test/cctest/test-heap.cc b/test/cctest/test-heap.cc
index 811973b..abbce6b 100644
--- a/test/cctest/test-heap.cc
+++ b/test/cctest/test-heap.cc
@@ -2441,3 +2441,53 @@ TEST(Regression144230) {
USE(global->SetProperty(*name, *call_function, NONE, kNonStrictMode));
CompileRun("call();");
}
+
+
+ static Handle<Code> CreateCode(int size) {
+ AlwaysAllocateScope always_allocate;
+#define __ assm.
+ Assembler assm(Isolate::Current(), NULL, 0);
+ for (int i = 0; i < size; i++) {
+ __ nop();
+ }
+ CodeDesc desc;
+ assm.GetCode(&desc);
+ MaybeObject* maybe_code = HEAP->CreateCode(
+ desc,
+ Code::ComputeFlags(Code::STUB),
+ Handle<Object>());
+ Object* code = NULL;
+ bool success = maybe_code->ToObject(&code);
+ CHECK(success);
+ CHECK(code->IsCode());
+ return Handle<Code>(Code::cast(code));
+}
+
+
+TEST(LargeExecutableMemoryChunk) {
+ InitializeVM();
+ v8::HandleScope scope;
+ CodeRange* code_range = ISOLATE->code_range();
+ if (!code_range->exists() ||
+ code_range->current_allocation_block_size() < 4 * Page::kPageSize) {
+ return;
+ }
+ const int kMaxCodeSize = MemoryAllocator::CodePageAreaSize() -
+ Code::kHeaderSize;
+ Address buffer;
+ size_t requested, allocated;
+ // Allocate and release a memory chunk to make sure that code range
+ // free list is not empty. Otherwise, we will get OOM failure later on.
+ requested = kMaxCodeSize;
+ buffer = code_range->AllocateRawMemory(requested, &allocated);
+ code_range->FreeRawMemory(buffer, allocated);
+ // Leave only 3 pages in the code range.
+ requested = code_range->current_allocation_block_size() - 3 *
Page::kPageSize;
+ buffer = code_range->AllocateRawMemory(requested, &allocated);
+ {
+ v8::HandleScope inner_scope;
+ CreateCode(kMaxCodeSize);
+ CreateCode(kMaxCodeSize); // This will allocate a chunk of size > page
size.
+ }
+ code_range->FreeRawMemory(buffer, allocated);
+}

https://chromiumcodereview.appspot.com/11275229/

[mstar...@chromium.org]

LGTM with two comments.


https://codereview.chromium.org/11275229/diff/1/src/spaces-inl.h
File src/spaces-inl.h (right):

https://codereview.chromium.org/11275229/diff/1/src/spaces-inl.h#newcode167
src/spaces-inl.h:167: ASSERT(page->area_size() <= kPageSize);
This should actually be ASSERT(page->area_size() <=
kNonCodeObjectAreaSize);

https://codereview.chromium.org/11275229/diff/1/src/spaces-inl.h#newcode169
src/spaces-inl.h:169: ASSERT(chunk->size() <=
static_cast<size_t>(kPageSize) ||
I am not sure if we should even have this second assertion anymore. If
nobody beside the MemoryAllocator should rely on chunk->size() being a
special size, we should not assert it here. So I think we should
completely remove the second assert, but could be convinced otherwise.

https://codereview.chromium.org/11275229/