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

Issue 828537 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug

Blocking:
issue 757440



Sign in to add a comment

CachedMatchedProperties inconsistent marking state

Project Member Reported by mlippautz@chromium.org, Apr 3 2018

Issue description

Could be 
- false positive
- missing write barrier
- weak handling that is not safe for incremental marking

Collected using
  is_component_build = true
  is_debug = true
  symbol_level = 2
  enable_blink_heap_verification = true
  enable_blink_heap_incremental_marking = true

Test
  fast/dom/shadow/content-pseudo-element-reprojection.html

crash log for renderer (pid <unknown>):
STDOUT: <empty>
STDERR: [1:1:0403/203703.925186:FATAL:MarkingVerifier.h(69)] Check failed: child_header->IsMarked(). 
STDERR: #0 0x7f46ebb2b55d base::debug::StackTrace::StackTrace()
STDERR: #1 0x7f46ebb29b1c base::debug::StackTrace::StackTrace()
STDERR: #2 0x7f46ebbb05aa logging::LogMessage::~LogMessage()
STDERR: #3 0x7f46da284e45 blink::MarkingVerifier::VerifyChild()
STDERR: #4 0x7f46da284b51 blink::MarkingVerifier::Visit()
STDERR: #5 0x7f46dc20f6ae blink::Visitor::Trace<>()
STDERR: #6 0x7f46dc20f630 blink::Visitor::Trace<>()
STDERR: #7 0x7f46dc210a8e blink::CachedMatchedPropertiesHashTraits::TraceInCollection<>()
STDERR: #8 0x7f46dc2109bf WTF::TraceNoWeakHandlingInCollectionHelper<>::Trace<>()
STDERR: #9 0x7f46dc21098d WTF::TraceInCollectionTrait<>::Trace<>()
STDERR: #10 0x7f46dc21095d blink::TraceCollectionIfEnabled<>::Trace<>()
STDERR: #11 0x7f46dc210921 WTF::TraceInCollectionTrait<>::Trace<>()
STDERR: #12 0x7f46dc2108dd blink::TraceCollectionIfEnabled<>::Trace<>()
STDERR: #13 0x7f46dc21088a WTF::TraceInCollectionTrait<>::Trace<>()
STDERR: #14 0x7f46dc2107ed blink::TraceTrait<>::Trace<>()
STDERR: #15 0x7f46da2846b4 blink::MarkingVerifier::VerifyObject()
STDERR: #16 0x7f46da27da7c blink::NormalPage::VerifyMarking()
STDERR: #17 0x7f46da27d873 blink::NormalPageArena::VerifyMarking()
STDERR: #18 0x7f46da26bc2e blink::ThreadHeap::VerifyMarking()
STDERR: #19 0x7f46da2985c7 blink::ThreadState::VerifyMarking()
STDERR: #20 0x7f46da297f06 blink::ThreadState::MarkPhaseEpilogue()
STDERR: #21 0x7f46da294ddb blink::ThreadState::IncrementalMarkingFinalize()
STDERR: #22 0x7f46da294593 blink::ThreadState::RunScheduledGC()
STDERR: #23 0x7f46da296367 blink::ThreadState::SafePoint()
STDERR: #24 0x7f46d9d23f8e blink::GCTaskObserver::DidProcessTask()
STDERR: #25 0x7f46da397109 blink::scheduler::WebThreadBase::TaskObserverAdapter::DidProcessTask()
STDERR: #26 0x7f46da36a5bf blink::scheduler::TaskQueueManagerImpl::NotifyDidProcessTask()
STDERR: #27 0x7f46da36a04d blink::scheduler::TaskQueueManagerImpl::DidRunTask()
STDERR: #28 0x7f46da37add7 blink::scheduler::internal::ThreadControllerImpl::DoWork()
STDERR: #29 0x7f46da37db21 _ZN4base8internal13FunctorTraitsIMN5blink9scheduler8internal20ThreadControllerImplEFvNS4_19SequencedTaskSource8WorkTypeEEvE6InvokeIRKNS_7WeakPtrIS5_EEJRKS7_EEEvS9_OT_DpOT0_
STDERR: #30 0x7f46da37da85 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN5blink9scheduler8internal20ThreadControllerImplEFvNS6_19SequencedTaskSource8WorkTypeEERKNS_7WeakPtrIS7_EEJRKS9_EEEvOT_OT0_DpOT1_
STDERR: #31 0x7f46da37d9fd _ZN4base8internal7InvokerINS0_9BindStateIMN5blink9scheduler8internal20ThreadControllerImplEFvNS5_19SequencedTaskSource8WorkTypeEEJNS_7WeakPtrIS6_EES8_EEEFvvEE7RunImplIRKSA_RKNSt3__15tupleIJSC_S8_EEEJLm0ELm1EEEEvOT_OT0_NSJ_16integer_sequenceImJXspT1_EEEE
STDERR: #32 0x7f46da37d90c _ZN4base8internal7InvokerINS0_9BindStateIMN5blink9scheduler8internal20ThreadControllerImplEFvNS5_19SequencedTaskSource8WorkTypeEEJNS_7WeakPtrIS6_EES8_EEEFvvEE3RunEPNS0_13BindStateBaseE
STDERR: #33 0x7f46ebada1ae _ZNO4base12OnceCallbackIFvvEE3RunEv
STDERR: #34 0x7f46ebb2f43f base::debug::TaskAnnotator::RunTask()
STDERR: #35 0x7f46ebbcf109 base::internal::IncomingTaskQueue::RunTask()
STDERR: #36 0x7f46ebbd82c5 base::MessageLoop::RunTask()
STDERR: #37 0x7f46ebbd8548 base::MessageLoop::DeferOrRunPendingTask()
STDERR: #38 0x7f46ebbd8879 base::MessageLoop::DoWork()
STDERR: #39 0x7f46ebbda8b7 base::MessagePumpDefault::Run()
STDERR: #40 0x7f46ebbd7a8c base::MessageLoop::Run()
STDERR: #41 0x7f46ebc8ebed base::RunLoop::Run()
STDERR: #42 0x7f46ea14a215 content::RendererMain()
STDERR: #43 0x7f46ea575391 content::RunZygote()
STDERR: #44 0x7f46ea5760de content::RunNamedProcessTypeMain()
STDERR: #45 0x7f46ea578604 content::ContentMainRunnerImpl::Run()
STDERR: #46 0x7f46ea56ead5 content::ContentServiceManagerMainDelegate::RunEmbedderProcess()
STDERR: #47 0x7f46e018fdfc service_manager::Main()
STDERR: #48 0x7f46ea574cd5 content::ContentMain()
STDERR: #49 0x00000061d155 main
STDERR: #50 0x7f46cfb482b1 __libc_start_main
STDERR: #51 0x00000061d02a _start
STDERR: 
 
Cc: hpayer@chromium.org
Status: Started (was: Assigned)
Finally managed to debug this using a few local modifications (for documentation):
- Smaller step size
- Reduce #callbacks used to check for the step size
- Avoid any other GCs being scheduled
- Avoid sweeping in the out of line allocation path

This way we always force incremental marking with very small step sizes. We could upstream a few of those changes for the stress mode.

The actual problem:
1. Start incremental marking
2. Allocate backing store for hashmap using ephemeron marking
3. Backing store is marked black.
4. Add ephemeron callbacks using the front object (HeapHashMap)
5. Trigger an operation that drops the backing store
6. GC finalization checks the HeapHashMap for a table and doesn't find one, bailing out.
7. The verifier finds a backing store that is marked and none of its reachable objects are marked. (all black->white edges)

The backing store should not be referenced anywhere else but it is still a violation of our the marking invariant.
The backing store is marked black in the step 3 (which means that it's pushed to the marking queue). Then why are the entries of the backing store not marked?


We just discussed this and were wondering the exact same thing :)

Will now check the write barrier paths because this should fall out from our architecture.
Correction: the write barriers looks fine. The post marking callback doesn't.

Scenario:
1. Start incremental marking
2. Register delayed marking for ephemeron table.
3. Add ephemeron callbacks using the front object (HeapHashMap)
5. Trigger an operation that drops the backing store
6. GC finalization checks the HeapHashMap for a table and doesn't find one, bailing out.
7. Post marking callback still marks the backing store using MarkNoTracing
8. The verifier finds a backing store that is marked and none of its reachable objects are marked. (all black->white edges)

Will think of a way to keep the consistent state and how to write a test.
I think if the conservative stack scanner would mark the backing store itself and not rely on it being marked through the "post marking worklist", then we could move the delayed marking step to the ephemeron processing.

- This would remove the "post_marking_worklist" completely.
- Keep the marking state consistent.

Will try to write some tests for this scenario now.
Thanks Michael for digging into this!

Maybe should we run a transitive closure after the post marking? Currently post marking calls MarkNoTracing and doesn't trace into entries.

If you can remove the post marking, that should be the best (although I'm not 100% sure if we can).

It needs to be balanced with regular weak processing (no ephemerons). I will try to go this way first.
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 6 2018

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

commit 73e7ae2870242ecb195b14318700e23e77df0945
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Fri Apr 06 18:00:02 2018

[oilpan] Merge post marking processing into weak processing

Post marking processing assumed that the backing store for weak
HashTable's remains unchanged during marking which is not correct in the
presence of incremental marking.

Since post marking was just marking the header of the backing store this
can be merged into the weak processing steps.

This CL
- Remove Heap::post_marking_worklist_ completely, avoid any memory
  consumption it might require.
- Merges WeakProcessing and PostMarkingProcessing as the post marking
  steps were only used for weak HashTables.

Bug:  chromium:828537 ,  chromium:757440 
Change-Id: I0d0b7552786659becf039419bff3701e7bb72aee
Reviewed-on: https://chromium-review.googlesource.com/999485
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548854}
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/heap/Heap.cpp
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/heap/Heap.h
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/heap/HeapAllocator.h
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/heap/IncrementalMarkingTest.cpp
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/heap/MarkingVerifier.h
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/heap/MarkingVisitor.cpp
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/heap/MarkingVisitor.h
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/heap/ThreadState.cpp
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/heap/ThreadState.h
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/heap/Visitor.h
[modify] https://crrev.com/73e7ae2870242ecb195b14318700e23e77df0945/third_party/WebKit/Source/platform/wtf/HashTable.h

Status: Fixed (was: Started)

Sign in to add a comment