New issue
Advanced search Search tips

Issue 899035 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 26
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 897733



Sign in to add a comment

Oilpan sweep fails

Project Member Reported by fs...@chromium.org, Oct 25

Issue description

While trying to submit this:

https://chromium-review.googlesource.com/c/chromium/src/+/1297067

where we use AdjustAmountOfExternalAllocatedMemory() to apply memory pressure and trigger GC.

Things are failing with this stack:
[1:1:1025/141026.508906:FATAL:thread_state.cc(546)] Check failed: !IsSweepingInProgress(). 
#0 0x7f475061238f base::debug::StackTrace::StackTrace()
#1 0x7f475053af2b logging::LogMessage::~LogMessage()
#2 0x7f47461be51c blink::ThreadState::ScheduleV8FollowupGCIfNeeded()
#3 0x7f47473b0a97 blink::V8GCController::GcEpilogue()
#4 0x7f4749297c84 v8::internal::Heap::PerformGarbageCollection()
#5 0x7f4749295664 v8::internal::Heap::CollectGarbage()
#6 0x7f4749296816 v8::internal::Heap::ReportExternalMemoryPressure()
#7 0x7f4747d428aa blink::ImageBitmap::~ImageBitmap()
#8 0x7f47461b5d3b blink::NormalPage::Sweep()
#9 0x7f47461b46ea blink::NormalPageArena::LazySweepPages()
#10 0x7f47461b0909 blink::BaseArena::LazySweep()
#11 0x7f47461b4bf2 blink::NormalPageArena::OutOfLineAllocate()
#12 0x7f4747368182 blink::NormalPageArena::AllocateObject()
#13 0x7f4747367e7d blink::ThreadHeap::AllocateOnArenaIndex()
#14 0x7f4747d42ec7 blink::ImageBitmap::Create()
#15 0x7f47455b6d30 blink::OffscreenCanvasRenderingContext2D::TransferToImageBitmap()

 
Cc: haraken@chromium.org keishi@chromium.org
I will take a look tomorrow. Since this is a recursive call there's likely a check for sweeping missing that prevents us from reentering sweeping here.
If you patch my CL, you can probably get a relatively consistent repro here:
https://www.scirra.com/labs/bugs/gpumemleak/offscreencanvas/index.html


(you need to have --enable-experimental-web-platform-features)
There are 2 issues here:
1. The DCHECK is too strict as CompleteSweep() does not guarantee that sweeping is completed when recursively triggered through finalizers.
2. V8 should not trigger a GC when external memory counter is decreased (sweeping). There's only a single path that can trigger this, so we will remove it.
Status: Started (was: Assigned)
I commented on the CL. There's was a bug in the CL which actually caused us to report a positive number when you wanted to report a negative one.

V8 schedules GCs on increments (growing strategy) which is why the bug is triggered.

Fixing your CL should avoid all the bugs you discovered. My fix won't help your CL because you are reporting an increment from the destructor which is not allowed.
Nice detective work, Michael :)

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 26

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

commit ae45472b6c7e5cce7c0979bbeafbf5c098b77221
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Fri Oct 26 10:33:09 2018

AdjustAmountOfExternalAllocatedMemory: Do not trigger GCs when reducing amount

GCs should only trigger only trigger when growing external memory but
not when removing it.

- The limit is already lowered when removing memory, so possible future
  allocations check against a lowered limit.
- Memory pressure signals are already handled via an explicit V8 API.

Bug:  chromium:899035 
Change-Id: I96da5862400e06edb8c9fa47357070b3b48560a1
Reviewed-on: https://chromium-review.googlesource.com/c/1301473
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57024}
[modify] https://crrev.com/ae45472b6c7e5cce7c0979bbeafbf5c098b77221/include/v8.h

Status: Fixed (was: Started)
I am marking this as fixed because the V8 issue is actually resolved and was not triggering this case.

Sign in to add a comment