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

Issue 820893 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Oilpan GC may let scheduler access LocalFrame that's already detached

Project Member Reported by yutak@chromium.org, Mar 12 2018

Issue description

Original issue discussed at https://chromium-review.googlesource.com/c/chromium/src/+/923688

The Patch Set 12 gave the following crash:

  Received signal 11 SEGV_MAPERR 000000000010
  0   Chromium Framework                  0x000000011ec4b9cc 
  base::debug::StackTrace::StackTrace(unsigned long) + 28
  1   Chromium Framework                  0x000000011ec4b801 base::debug::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, void*) + 2401
  2   libsystem_platform.dylib            0x00007fff76ddff5a _sigtramp + 26
  3   ???                                 0x00007fb43f522b08 0x0 + 140412133190408
  4   Chromium Framework                  0x00000001233340cc blink::WebLocalFrameImpl::LocalRoot() + 28
  5   Chromium Framework                  0x00000001239da1b4 blink::ChromeClientImpl::GetWebLayerTreeView(blink::LocalFrame*) + 20
  6   Chromium Framework                  0x00000001239fbdf7 blink::Page::RequestBeginMainFrameNotExpected(bool) + 183
  7   Chromium Framework                  0x000000011e47ccf0 blink::scheduler::RendererSchedulerImpl::OnPendingTasksChanged(bool) + 464
  8   Chromium Framework                  0x000000011e45a813 blink::scheduler::IdleHelper::OnIdleTaskPostedOnMainThread() + 83
  9   Chromium Framework                  0x000000011e45c5b2 blink::scheduler::IdleHelper::OnIdleTaskPosted() + 162
  10  Chromium Framework                  0x000000011e45f397 blink::scheduler::SingleThreadIdleTaskRunner::PostNonNestableIdleTask(base::Location const&, base::OnceCallback<void (base::TimeTicks)>) + 39
  11  Chromium Framework                  0x000000011e4603fa blink::scheduler::WebSchedulerImpl::PostNonNestableIdleTask(base::Location const&, base::OnceCallback<void (double)>) + 346
  12  Chromium Framework                  0x000000011e3b58b0 blink::ThreadState::ScheduleIdleGC() + 192
  13  Chromium Framework                  0x000000011e3b65e7 blink::ThreadState::ScheduleGCIfNeeded() + 999
  14  Chromium Framework                  0x000000011e3abbbf blink::NormalPageArena::OutOfLineAllocate(unsigned long, unsigned long) + 431
  15  Chromium Framework                  0x000000011e3cfd49 blink::ThreadHeap::AllocateOnArenaIndex(blink::ThreadState*, unsigned long, int, unsigned long, char const*) + 313
  16  Chromium Framework                  0x0000000122673aa0 blink::RemoteWindowProxy::Create(v8::Isolate*, blink::RemoteFrame&, scoped_refptr<blink::DOMWrapperWorld>) + 80
  17  Chromium Framework                  0x000000012267390f blink::WindowProxyManager::CreateWindowProxy(blink::DOMWrapperWorld&) + 111
  18  Chromium Framework                  0x00000001226737cf blink::WindowProxyManager::WindowProxyManager(blink::Frame&, blink::WindowProxyManager::FrameType) + 175
  19  Chromium Framework                  0x00000001232f2fb8 blink::RemoteFrame::RemoteFrame(blink::RemoteFrameClient*, blink::Page&, blink::FrameOwner*) + 120
  20  Chromium Framework                  0x00000001232f2424 blink::RemoteFrame::Create(blink::RemoteFrameClient*, blink::Page&, blink::FrameOwner*) + 100
  21  Chromium Framework                  0x0000000123207bb6 blink::WebRemoteFrameImpl::InitializeCoreFrame(blink::Page&, blink::FrameOwner*, WTF::AtomicString const&) + 22
  22  Chromium Framework                  0x00000001231ed5f6 blink::WebFrame::Swap(blink::WebFrame*) + 1062
  23  Chromium Framework                  0x00000001247297ec content::RenderFrameImpl::OnSwapOut(int, bool, content::FrameReplicationState const&) + 380
  <...>

dcheng explained:

  OK I think I understand the crash. It is rather tricky. What is happening is this:
  1) //content is swapping a local frame (old local frame) to a remote frame (new remote frame).
  2) At this point, Page::MainFrame() still points to old local frame.
  3) WebFrame::Swap() calls Detach() on the old local frame.
  4) WebFrame::Swap() initializes the core frame of the new remote frame.
  5) In the middle of allocating objects for the new remote frame, Oilpan schedules an idle GC.
  6) This causes the scheduler to notify all WebViewSchedulers: 
  https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc?rcl=ad6860b020ad4ecfa42c78cc31217954bc24fe2a&l=2241
  7) This calls into Page::RequestBeginMainFrameNotExpected.
  8) Which calls into ChromeClientImpl::GetWebLayerTreeView() with the old local frame.
  9) Converting LocalFrame -> WebLocalFrame returns nullptr because the LocalFrame is already detached (doing this lookup requires that Frame::Client() not be null--but old local frame is already detached)

  It seems weird (and a little dangerous) that this work is triggered in the middle of allocation. I am not sure how hard it is to change.


 
Cc: kraynov@chromium.org
Thanks for analysing this. I think a simple fix would be to change RSI::OnPendingTasksChanged to issue a PostTask for subscribing to the BeginFrameNotExpectedSoon notifications. This should happen relatively rarely so the extra latency isn't an issue.

We'll take a look.
Owner: altimin@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by dcheng@chromium.org, Mar 13 2018

Would it be reasonable to avoid calling OnIdleTaskPostedOnMainThread synchronously here and just always post a task? It seems like there are other IdleHelper::Delegate::OnPendingTasksChanged methods that could be invoked, and it seems like it'd be safer to minimize the amount of code that runs in the middle of allocations.
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 16 2018

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

commit 1bfa23fdee284c09f24c0ea1ec805939a96daa27
Author: Alexander Timin <altimin@chromium.org>
Date: Fri Mar 16 13:30:38 2018

[scheduler] Issue RequestBeginMainFrameNotExpected call asynchronously.

Post a control task to call RequestBeginMainFrameNotExpected
asynchronously to avoid calling it when frame tree is dirty.

R=yutak@chromium.org,dcheng@chromium.org,haraken@chromium.org
BUG= 820893 

Change-Id: I375aa46180f20f053c13563df910d3813c22938f
Reviewed-on: https://chromium-review.googlesource.com/962787
Reviewed-by: Yuta Kitamura <yutak@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543683}
[modify] https://crrev.com/1bfa23fdee284c09f24c0ea1ec805939a96daa27/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/1bfa23fdee284c09f24c0ea1ec805939a96daa27/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/1bfa23fdee284c09f24c0ea1ec805939a96daa27/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
[modify] https://crrev.com/1bfa23fdee284c09f24c0ea1ec805939a96daa27/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment