New issue
Advanced search Search tips

Issue 831118 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

ScriptPromiseResolver consistency issue

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

Issue description

Another issue that flakes every now and then on ToT and is not reproducible so far.

The DCHECK seems to verify consistency of the state during destructor. Not sure what is off here.

Failing test:
  media/crash-in-media-moved-to-newdocument.html

Build:
  Regular linux release build with DCHECKs

crash log for renderer (pid <unknown>):
STDOUT: <empty>
STDERR: [1:1:0410/031559.473181:FATAL:script_promise_resolver.h(57)] Check failed: state_ == kDetached || !is_promise_called_ || !GetScriptState()->ContextIsValid() || !GetExecutionContext() || GetExecutionContext()->IsContextDestroyed(). 
STDERR: #0 0x7ff5b0ef655d base::debug::StackTrace::StackTrace()
STDERR: #1 0x7ff5b0ef4b1c base::debug::StackTrace::StackTrace()
STDERR: #2 0x7ff5b0f7b92a logging::LogMessage::~LogMessage()
STDERR: #3 0x7ff5a12ee5b1 blink::ScriptPromiseResolver::~ScriptPromiseResolver()
STDERR: #4 0x7ff5a132e574 blink::GarbageCollectedFinalized<>::FinalizeGarbageCollectedObject()
STDERR: #5 0x7ff5a132e545 blink::FinalizerTraitImpl<>::Finalize()
STDERR: #6 0x7ff5a132e525 blink::FinalizerTrait<>::Finalize()
STDERR: #7 0x7ff59f8aaee1 blink::HeapObjectHeader::Finalize()
STDERR: #8 0x7ff59f8b21b8 blink::NormalPage::Sweep()
STDERR: #9 0x7ff59f8ac189 blink::BaseArena::SweepUnsweptPage()
STDERR: #10 0x7ff59f8ac744 blink::BaseArena::CompleteSweep()
STDERR: #11 0x7ff59f8c5d73 blink::ThreadState::EagerSweep()
STDERR: #12 0x7ff59f8c5699 blink::ThreadState::PreSweep()
STDERR: #13 0x7ff59f8c525a blink::ThreadState::IncrementalMarkingFinalize()
STDERR: #14 0x7ff59f8c4a03 blink::ThreadState::RunScheduledGC()
STDERR: #15 0x7ff59f8c67d7 blink::ThreadState::SafePoint()
STDERR: #16 0x7ff59f3bddae blink::GCTaskObserver::DidProcessTask()
STDERR: #17 0x7ff59f9c9f29 blink::scheduler::WebThreadBase::TaskObserverAdapter::DidProcessTask()
STDERR: #18 0x7ff59f99c30f blink::scheduler::TaskQueueManagerImpl::NotifyDidProcessTask()
STDERR: #19 0x7ff59f99bd9d blink::scheduler::TaskQueueManagerImpl::DidRunTask()
STDERR: #20 0x7ff59f9ad327 blink::scheduler::internal::ThreadControllerImpl::DoWork()
STDERR: #21 0x7ff59f9b0091 _ZN4base8internal13FunctorTraitsIMN5blink9scheduler8internal20ThreadControllerImplEFvNS4_19SequencedTaskSource8WorkTypeEEvE6InvokeIRKNS_7WeakPtrIS5_EEJRKS7_EEEvS9_OT_DpOT0_
STDERR: #22 0x7ff59f9afff5 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN5blink9scheduler8internal20ThreadControllerImplEFvNS6_19SequencedTaskSource8WorkTypeEERKNS_7WeakPtrIS7_EEJRKS9_EEEvOT_OT0_DpOT1_
STDERR: #23 0x7ff59f9aff6d _ZN4base8internal7InvokerINS0_9BindStateIMN5blink9scheduler8internal20ThreadControllerImplEFvNS5_19SequencedTaskSource8WorkTypeEEJNS_7WeakPtrIS6_EES8_EEEFvvEE7RunImplIRKSA_RKNSt3__15tupleIJSC_S8_EEEJLm0ELm1EEEEvOT_OT0_NSJ_16integer_sequenceImJXspT1_EEEE
STDERR: #24 0x7ff59f9afe7c _ZN4base8internal7InvokerINS0_9BindStateIMN5blink9scheduler8internal20ThreadControllerImplEFvNS5_19SequencedTaskSource8WorkTypeEEJNS_7WeakPtrIS6_EES8_EEEFvvEE3RunEPNS0_13BindStateBaseE
STDERR: #25 0x7ff5b0ea51ae _ZNO4base12OnceCallbackIFvvEE3RunEv
STDERR: #26 0x7ff5b0efa698 base::debug::TaskAnnotator::RunTask()
STDERR: #27 0x7ff5b0f9a529 base::internal::IncomingTaskQueue::RunTask()
STDERR: #28 0x7ff5b0fa3947 base::MessageLoop::RunTask()
STDERR: #29 0x7ff5b0fa3bb8 base::MessageLoop::DeferOrRunPendingTask()
STDERR: #30 0x7ff5b0fa3ee9 base::MessageLoop::DoWork()
STDERR: #31 0x7ff5b0fa5f27 base::MessagePumpDefault::Run()
STDERR: #32 0x7ff5b0fa311c base::MessageLoop::Run()
STDERR: #33 0x7ff5b1059e6d base::RunLoop::Run()
STDERR: #34 0x7ff5af521d45 content::RendererMain()
STDERR: #35 0x7ff5af95f271 content::RunZygote()
STDERR: #36 0x7ff5af95ffbe content::RunNamedProcessTypeMain()
STDERR: #37 0x7ff5af9624e4 content::ContentMainRunnerImpl::Run()
STDERR: #38 0x7ff5af957f75 content::ContentServiceManagerMainDelegate::RunEmbedderProcess()
STDERR: #39 0x7ff5a54bcdfc service_manager::Main()
STDERR: #40 0x7ff5af95ebb5 content::ContentMain()
STDERR: #41 0x00000061b155 main
STDERR: #42 0x7ff595189f45 __libc_start_main
STDERR: #43 0x00000061b02a _start
STDERR: 
 

Comment 1 by peria@chromium.org, Apr 12 2018

Cc: peria@chromium.org
Reproduces on ToT

rev:
  6c2e15ce3aaf1b7eeb067fca773632c1a11aeff1

gn args:
  is_component_build = true
  is_debug = true
  use_goma = true
  symbol_level = 2
  enable_blink_heap_incremental_marking = true

cmd line:
  out/Debug/content_shell --enable-blink-features=HeapIncrementalMarkingStress --run-layout-test media/crash-in-media-moved-to-newdocument.html

                                              

Alright, this is what is happening

1. HTMLMediaElement::playForBindings: creates a PromiseResolver and stores it in play_promise_resolvers_.
2. HTMLMediaElement::RejectPlayPromises: copies over play_promise_resolvers_ to play_promise_reject_list_ and clears play_promise_resolvers_
3. HTMLMediaElement::RejectPlayPromisesInternal: calls Reject on the resolver and clears play_promise_reject_list_

That seems good. Now the Reject() part:
3a. Calls into ScriptPromiseResolver::ResolveOrReject
3b. We reach ScriptForbiddenScope::IsScriptForbidden() 
3c. Because the script is forbidden we call timer_.StartOneShot(...) which returns to the event loop

At the event loop we start a GC and immediately finalize. The ScriptPromiseResolver will be gone because there's nothing keeping it alive.

The questionable part is:
https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h?q=ScriptPromiseResolver&dr=CSs&l=154

Kentaro, can you help triage this? This does not seem like a GC issue.
Btw, a regular (idle) GC at the event loop can also happen on ToT; it's just very unlikely.
Cc: tzik@chromium.org dcheng@chromium.org
Owner: dcheng@chromium.org
+dcheng +tzik

I'd say that this is a design bug of TaskRunnerTimer.

1) TaskRunnerTimer is created with a |this| pointer. Note that the |this| pointer is not wrapped with WrapPersistent.
2) timer.StartOneShot() is called.
3) The |this| object is garbage-collected.
4) The timer fires and crashes.

My preference would be to deprecate TaskRunnerTimer and replace it with PostTask. Then we can use WrapPersistent.

Any thoughts?

Comment 6 by tzik@chromium.org, Apr 27 2018

I think TaskRunnerTimer doesn't cause the crash in that scenario.
 - If |this| is already swept and its destructor is called, timer_ is cancelled.
 - If |this| is not marked in a marking phase, and waiting for a lazy sweep phase, TaskRunnerTimer detects that with TimerIsObjectAliveTrait<>::IsHeapObjectAlive() and cancel the invocation.

The dtor of ScriptPromiseResolver checks a few consistency conditions [1]. Maybe something is missing there?

Oilpan definitelly doesn't keep the object alive, which might actually be ok considering that everything seems to be cancelled. 

[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h?q=ScriptPromiseResolver&dr=CSs&l=49

Comment 8 by tzik@chromium.org, Apr 27 2018

PostTask will work fine. But, I think it's easier to just add KeepAliveWhilePending() at StartOneShot().
Owner: tzik@chromium.org
tzik: Thanks for looking into this bug! Would you mind handling it?

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 30 2018

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

commit 8c40146ad5239f58f1426b25ed58cca9a7f57017
Author: tzik <tzik@chromium.org>
Date: Mon Apr 30 00:36:17 2018

Keep alive ScriptPromiseResolver on a delayed resolution

If a ScriptPromiseResolver is requested to resolve in a ScriptForbiddenScope,
it delays the resolution. However, as no one retains a reference to the
SPR instance, the GC may collect it before the promise is fully resolved.

Bug:  831118 
Change-Id: Ic8b506d808a7d7cf851e25fe2fb6e7d3aa7464c0
Reviewed-on: https://chromium-review.googlesource.com/1034046
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554684}
[modify] https://crrev.com/8c40146ad5239f58f1426b25ed58cca9a7f57017/third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h
[modify] https://crrev.com/8c40146ad5239f58f1426b25ed58cca9a7f57017/third_party/blink/renderer/bindings/core/v8/script_promise_resolver_test.cc

Comment 11 by tzik@chromium.org, Apr 30 2018

Status: Fixed (was: Available)

Sign in to add a comment