New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 861624 link

Starred by 4 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

VerifyObjectStartBitmapIsConsistentWithPayload() can be called with an allocation point

Project Member Reported by keishi@chromium.org, Jul 9

Issue description

VerifyObjectStartBitmapIsConsistentWithPayload() doesn't work when there is an allocation point on its page.

Originally found https://chromium-review.googlesource.com/c/chromium/src/+/1127202

I was able to reproduce the issue:
https://drive.google.com/open?id=1q2Vw3ojQQXjDMlSqHeEITl319YF-vW17

Example code:

class MyA : public GarbageCollectedFinalized<MyA> {
public:
  MyA() {
    LOG(ERROR) << "MyA " << this << " sizeof " << sizeof(this);
  }
  ~MyA() {LOG(ERROR) << "~MyA " << this;}
  void Trace(Visitor* visitor) {
    buffer_[0] = 'C';
  }
  EAGERLY_FINALIZE(); // Using eagerly finalize so all objects use the same arena.
private:
  char buffer_[256];
};

class MyB : public GarbageCollected<MyB> {
public:
  MyB() {
    LOG(ERROR) << "MyB " << this << " sizeof " << sizeof(this);
  }
  void Trace(Visitor* visitor) {
    buffer_[0] = 'C';
  }
  EAGERLY_FINALIZE();
private:
  char buffer_[256];
};

class MyD : public GarbageCollected<MyB> {
public:
  MyD() {
    LOG(ERROR) << "MyD " << this << " sizeof " << sizeof(this);
  }
  void Trace(Visitor* visitor) {
    buffer_[0] = 'C';
  }
  EAGERLY_FINALIZE();
private:
  char buffer_[128];
};

class MyC : public GarbageCollectedFinalized<MyC> {
public:
  MyC(Persistent<MyD>* d) : d_(d) {
    LOG(ERROR) << "MyC " << this << " sizeof " << sizeof(this);
  }
  ~MyC() {
    LOG(ERROR) << "~MyC " << this;
    (*d_) = new MyD();
    LOG(ERROR) << "PageFromObject(this) " << PageFromObject(this);
    LOG(ERROR) << "PageFromObject((*d_).Get()) " << PageFromObject((*d_).Get());
    CHECK(PageFromObject(this) == PageFromObject((*d_).Get()));
  }
  void Trace(Visitor* visitor) {
    buffer_[0] = 'C';
  }
  EAGERLY_FINALIZE();
private:
  char buffer_[256];
  Persistent<MyD>* d_;
};

class MyE : public GarbageCollected<MyE> {
public:
  MyE() {
  }
  void Trace(Visitor* visitor) {
  }
  EAGERLY_FINALIZE();
};

TEST(HeapTest, Exp) {
  ThreadState::Current()->CollectAllGarbage();
  Persistent<MyA> a = new MyA();
  BasePage* page = PageFromObject(a.Get());
  Persistent<MyB> b = new MyB();
  CHECK(page == PageFromObject(b.Get()));
  Persistent<MyD> d = nullptr;
  Persistent<MyC> c = new MyC(&d);
  CHECK(page == PageFromObject(c.Get()));
  PersistentHeapVector<Member<MyE>> eRetainer;
  while (true) {
    MyE* e = new MyE();
    eRetainer.push_back(e);
    BasePage* page = PageFromObject(e);
    if (reinterpret_cast<NormalPageArena*>(page->Arena())->RemainingAllocationSize() < 128) {
      break;
    }
  }
  LOG(ERROR) << "PreciselyCollectGarbage";
  PreciselyCollectGarbage();
  LOG(ERROR) << "PreciselyCollectGarbage END";
  a = nullptr;
  LOG(ERROR) << "PreciselyCollectGarbage A page " << page;
  PreciselyCollectGarbage();
  LOG(ERROR) << "PreciselyCollectGarbage A END";
  c = nullptr;
  LOG(ERROR) << "PreciselyCollectGarbage B page " << page;
  PreciselyCollectGarbage();
  LOG(ERROR) << "PreciselyCollectGarbage B END";
  ThreadState::Current()->CollectAllGarbage();
}
 
Components: Blink>MemoryAllocator>GarbageCollection
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a11c0c98a3f7d5d95c28787a630d6ab85bc0fff5

commit a11c0c98a3f7d5d95c28787a630d6ab85bc0fff5
Author: Keishi Hattori <keishi@chromium.org>
Date: Mon Jul 09 12:58:29 2018

Oilpan: VerifyObjectStartBitmapIsConsistentWithPayload() should work when allocation point is on the page.

VerifyObjectStartBitmapIsConsistentWithPayload() can be called with an allocation point set, when a FreeListEntry is added and then used for allocation within the same sweep phase.

This CL resolves the issue by making VerifyObjectStartBitmapIsConsistentWithPayload() support the case where an allocation point is set.

Bug: 861624
Change-Id: I65740e0fb3e157ebf36a65673bc1293cb49b0102
Reviewed-on: https://chromium-review.googlesource.com/1127902
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573280}
[modify] https://crrev.com/a11c0c98a3f7d5d95c28787a630d6ab85bc0fff5/third_party/blink/renderer/platform/heap/heap_page.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jul 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ce298ca906f883be59f4f299fb51607e0439793

commit 8ce298ca906f883be59f4f299fb51607e0439793
Author: Keishi Hattori <keishi@chromium.org>
Date: Tue Jul 10 09:35:14 2018

Revert "Oilpan: VerifyObjectStartBitmapIsConsistentWithPayload() should work when allocation point is on the page."

This reverts commit a11c0c98a3f7d5d95c28787a630d6ab85bc0fff5.

Reason for revert: Contained bug.

Original change's description:
> Oilpan: VerifyObjectStartBitmapIsConsistentWithPayload() should work when allocation point is on the page.
> 
> VerifyObjectStartBitmapIsConsistentWithPayload() can be called with an allocation point set, when a FreeListEntry is added and then used for allocation within the same sweep phase.
> 
> This CL resolves the issue by making VerifyObjectStartBitmapIsConsistentWithPayload() support the case where an allocation point is set.
> 
> Bug: 861624
> Change-Id: I65740e0fb3e157ebf36a65673bc1293cb49b0102
> Reviewed-on: https://chromium-review.googlesource.com/1127902
> Commit-Queue: Keishi Hattori <keishi@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573280}

TBR=haraken@chromium.org,keishi@chromium.org,nhiroki@chromium.org,mlippautz@chromium.org

Change-Id: Ifb57ad68e4cb4ade99bbd6aecc09fab84d862bb9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 861624
Reviewed-on: https://chromium-review.googlesource.com/1131054
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573661}
[modify] https://crrev.com/8ce298ca906f883be59f4f299fb51607e0439793/third_party/blink/renderer/platform/heap/heap_page.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc5a5d79953f1bb39947a3c8c57353958c4f0141

commit cc5a5d79953f1bb39947a3c8c57353958c4f0141
Author: Keishi Hattori <keishi@chromium.org>
Date: Tue Jul 10 13:59:52 2018

[Reland] Oilpan: VerifyObjectStartBitmapIsConsistentWithPayload() should work when allocation point is on the page.

When a FreeListEntry is added and then used for allocation (within a finalizer) within the sweeping of a page, VerifyObjectStartBitmapIsConsistentWithPayload() can be called with an allocation point set.
This CL resolves the issue by making VerifyObjectStartBitmapIsConsistentWithPayload() support the case where an allocation point is set.

Bug: 861624
Change-Id: I9229f9489c63ad7e60cb40dcf04a773735a16861
Reviewed-on: https://chromium-review.googlesource.com/1131154
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573713}
[modify] https://crrev.com/cc5a5d79953f1bb39947a3c8c57353958c4f0141/third_party/blink/renderer/platform/heap/heap_page.cc

Cc: nhiroki@chromium.org haraken@chromium.org adithyas@chromium.org lucferron@chromium.org geoffl...@chromium.org sunn...@chromium.org st...@chromium.org lijeffrey@chromium.org kbr@chromium.org keishi@chromium.org jmad...@chromium.org jbroman@chromium.org
 Issue 860517  has been merged into this issue.

Sign in to add a comment