New issue
Advanced search Search tips

Issue 757879 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug

Blocking:
issue 596231
issue 736929



Sign in to add a comment

Double binding in ChromeLanguageDetectionClient::BindContentTranslateDriver

Project Member Reported by siggi@chromium.org, Aug 22 2017

Issue description

I tried to run a dcheck_always_on SyzyASAN build locally, which asserts about 10s in.
Looks like the culprit is here:

void BindingStateBase::BindInternal(
    ScopedMessagePipeHandle handle,
    scoped_refptr<base::SingleThreadTaskRunner> runner,
    const char* interface_name,
    std::unique_ptr<MessageReceiver> request_validator,
    bool passes_associated_kinds,
    bool has_sync_methods,
    MessageReceiverWithResponderStatus* stub,
    uint32_t interface_version) {
  DCHECK(!router_);  << HERE

  auto sequenced_runner =
      GetTaskRunnerToUseFromUserProvidedTaskRunner(std::move(runner));
  MultiplexRouter::Config config =
      passes_associated_kinds
          ? MultiplexRouter::MULTI_INTERFACE
          : (has_sync_methods
                 ? MultiplexRouter::SINGLE_INTERFACE_WITH_SYNC_METHODS
                 : MultiplexRouter::SINGLE_INTERFACE);
  router_ =
      new MultiplexRouter(std::move(handle), config, false, sequenced_runner);
  router_->SetMasterInterfaceName(interface_name);

  endpoint_client_.reset(new InterfaceEndpointClient(
      router_->CreateLocalEndpointHandle(kMasterInterfaceId), stub,
      std::move(request_validator), has_sync_methods,
      std::move(sequenced_runner), interface_version));
}

0:000> kc
  *** Stack trace for last set context - .thread/.cxr resets it
 # 
00 chrome_67660000!base::debug::BreakDebugger
01 chrome_67660000!logging::LogMessage::~LogMessage
02 chrome_67660000!mojo::internal::BindingStateBase::BindInternal
03 chrome_67660000!mojo::internal::BindingState<translate::mojom::ContentTranslateDriver,mojo::RawPtrImplRefTraits<translate::mojom::ContentTranslateDriver> >::Bind
04 chrome_67660000!mojo::Binding<translate::mojom::ContentTranslateDriver,mojo::RawPtrImplRefTraits<translate::mojom::ContentTranslateDriver> >::Bind
05 chrome_67660000!ChromeLanguageDetectionClient::BindContentTranslateDriver
06 chrome_67660000!base::internal::FunctorTraits<void (__cdecl*)(mojo::InterfaceRequest<device::mojom::UsbChooserService>,content::RenderFrameHost *),void>::Invoke<mojo::InterfaceRequest<device::mojom::UsbChooserService>,content::RenderFrameHost *>
07 chrome_67660000!base::internal::InvokeHelper<0,void>::MakeItSo
08 chrome_67660000!base::internal::Invoker<base::internal::BindState<void (__cdecl*)(mojo::InterfaceRequest<translate::mojom::ContentTranslateDriver>,content::RenderFrameHost *)>,void __cdecl(mojo::InterfaceRequest<translate::mojom::ContentTranslateDriver>,content::RenderFrameHost *)>::RunImpl<void (__cdecl*const &)(mojo::InterfaceRequest<translate::mojom::ContentTranslateDriver>,content::RenderFrameHost *),std::tuple<> const &>
09 chrome_67660000!base::internal::Invoker<base::internal::BindState<void (__cdecl*)(mojo::InterfaceRequest<translate::mojom::ContentTranslateDriver>,content::RenderFrameHost *)>,void __cdecl(mojo::InterfaceRequest<translate::mojom::ContentTranslateDriver>,content::RenderFrameHost *)>::Run
0a chrome_67660000!base::Callback<void __cdecl(mojo::InterfaceRequest<translate::mojom::ContentTranslateDriver>,content::RenderFrameHost *),1,1>::Run
0b chrome_67660000!service_manager::CallbackBinder<device::mojom::UsbDeviceManager,content::RenderFrameHost *>::RunCallback
0c chrome_67660000!service_manager::CallbackBinder<translate::mojom::ContentTranslateDriver,content::RenderFrameHost *>::BindInterface
0d chrome_67660000!service_manager::BinderRegistryWithArgs<service_manager::BindSourceInfo const &>::BindInterface
0e chrome_67660000!service_manager::BinderRegistryWithArgs<service_manager::BindSourceInfo const &>::TryBindInterface
0f chrome_67660000!ChromeContentBrowserClient::BindInterfaceRequestFromFrame
10 chrome_67660000!content::RenderFrameHostImpl::GetInterface
11 chrome_67660000!service_manager::mojom::InterfaceProviderStubDispatch::Accept
12 chrome_67660000!service_manager::mojom::InterfaceProviderStub<mojo::RawPtrImplRefTraits<service_manager::mojom::InterfaceProvider> >::Accept
13 chrome_67660000!mojo::InterfaceEndpointClient::HandleValidatedMessage
14 chrome_67660000!mojo::FilterChain::Accept
15 chrome_67660000!mojo::InterfaceEndpointClient::HandleIncomingMessage
16 chrome_67660000!mojo::internal::MultiplexRouter::ProcessIncomingMessage
17 chrome_67660000!mojo::internal::MultiplexRouter::Accept
18 chrome_67660000!mojo::FilterChain::Accept
19 chrome_67660000!mojo::Connector::ReadSingleMessage
1a chrome_67660000!mojo::Connector::ReadAllAvailableMessages
1b chrome_67660000!mojo::Connector::OnHandleReadyInternal
1c chrome_67660000!base::internal::FunctorTraits<void (__thiscall mojo::Connector::*)(unsigned int),void>::Invoke
1d chrome_67660000!base::internal::InvokeHelper<0,void>::MakeItSo
1e chrome_67660000!base::internal::Invoker<base::internal::BindState<void (__thiscall mojo::Connector::*)(unsigned int),base::internal::UnretainedWrapper<mojo::Connector> >,void __cdecl(unsigned int)>::RunImpl<void (__thiscall mojo::Connector::*const &)(unsigned int),std::tuple<base::internal::UnretainedWrapper<mojo::Connector> > const &,0>
1f chrome_67660000!base::internal::Invoker<base::internal::BindState<void (__thiscall mojo::Connector::*)(unsigned int),base::internal::UnretainedWrapper<mojo::Connector> >,void __cdecl(unsigned int)>::Run
20 chrome_67660000!base::Callback<void __cdecl(enum device::UMABluetoothDiscoverySessionOutcome),1,1>::Run
21 chrome_67660000!device::RunDiscoverySessionErrorCallback
22 chrome_67660000!base::internal::FunctorTraits<void (__cdecl*)(base::Callback<void __cdecl(unsigned int),1,1> const &,unsigned int,mojo::HandleSignalsState const &),void>::Invoke
23 chrome_67660000!base::internal::InvokeHelper<0,void>::MakeItSo
24 chrome_67660000!base::internal::Invoker<base::internal::BindState<void (__cdecl*)(base::Callback<void __cdecl(unsigned int),1,1> const &,unsigned int,mojo::HandleSignalsState const &),base::Callback<void __cdecl(unsigned int),1,1> >,void __cdecl(unsigned int,mojo::HandleSignalsState const &)>::RunImpl<void (__cdecl*const &)(base::Callback<void __cdecl(unsigned int),1,1> const &,unsigned int,mojo::HandleSignalsState const &),std::tuple<base::Callback<void __cdecl(unsigned int),1,1> > const &,0>
25 chrome_67660000!base::internal::Invoker<base::internal::BindState<void (__cdecl*)(base::Callback<void __cdecl(unsigned int),1,1> const &,unsigned int,mojo::HandleSignalsState const &),base::Callback<void __cdecl(unsigned int),1,1> >,void __cdecl(unsigned int,mojo::HandleSignalsState const &)>::Run
26 chrome_67660000!base::Callback<void __cdecl(unsigned int,mojo::HandleSignalsState const &),1,1>::Run
27 chrome_67660000!mojo::SimpleWatcher::OnHandleReady
28 chrome_67660000!base::internal::FunctorTraits<void (__thiscall midi::MidiScheduler::*)(midi::MidiManagerClient *,unsigned int,base::Callback<void __cdecl(void),1,1> const &),void>::Invoke
29 chrome_67660000!base::internal::InvokeHelper<1,void>::MakeItSo<void (__thiscall midi::MidiScheduler::*const &)(midi::MidiManagerClient *,unsigned int,base::Callback<void __cdecl(void),1,1> const &),base::WeakPtr<midi::MidiScheduler> const &,midi::MidiManagerClient * const &,unsigned int const &,base::Callback<void __cdecl(void),1,1> const &>
2a chrome_67660000!base::internal::Invoker<base::internal::BindState<void (__thiscall mojo::SimpleWatcher::*)(int,unsigned int,mojo::HandleSignalsState const &),base::WeakPtr<mojo::SimpleWatcher>,int,unsigned int,mojo::HandleSignalsState>,void __cdecl(void)>::RunImpl<void (__thiscall mojo::SimpleWatcher::*const &)(int,unsigned int,mojo::HandleSignalsState const &),std::tuple<base::WeakPtr<mojo::SimpleWatcher>,int,unsigned int,mojo::HandleSignalsState> const &,0,1,2,3>
2b chrome_67660000!base::internal::Invoker<base::internal::BindState<void (__thiscall mojo::SimpleWatcher::*)(int,unsigned int,mojo::HandleSignalsState const &),base::WeakPtr<mojo::SimpleWatcher>,int,unsigned int,mojo::HandleSignalsState>,void __cdecl(void)>::Run

 

Comment 1 by siggi@chromium.org, Aug 22 2017

Blocking: 596231

Comment 2 by siggi@chromium.org, Aug 22 2017

Labels: -Pri-3 Pri-2
Owner: siggi@chromium.org
Status: Started (was: Untriaged)
Looks like a straightforward case of double-binding to the service. I think the fix ought to be pretty simple.

Comment 3 by w...@chromium.org, Aug 22 2017

Blocking: 736929
Cc: siggi@chromium.org
Labels: -Pri-2 M-62 Pri-1
Owner: martis@chromium.org

Comment 4 by w...@chromium.org, Aug 22 2017

Reviewing siggi's CL, it seems that the ChromeLanguageDetectionClient doesn't cope with multiple render frames within a single WebContents; we should fix that before we run the experiment outlined in issue 736929.

Comment 5 by martis@chromium.org, Aug 22 2017

Hi all,

This is my mistake; we had "deduced" that we'd only need one connection (because of the check here https://cs.chromium.org/chromium/src/chrome/browser/language/chrome_language_detection_client.cc), but that is clearly not true.

I believe the fix will be to use a BindingSet, although I don't yet fully understand why it is required: can there be multiple main render frames per web contents?

Working on that fix now.

Comment 6 by martis@chromium.org, Aug 22 2017

Cc: martis@chromium.org
 Issue 757736  has been merged into this issue.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 23 2017

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

commit 156f8f09114e82f5d6c9abf35fa2afce7023b052
Author: Michael Martis <martis@chromium.org>
Date: Wed Aug 23 04:05:13 2017

Allowed the language detection client to bind to multiple render frames.

ChromeLanguageDetectionClient replaces ContentTranslateDriver as the endpoint
for language detection IPCs. ContentTranslateDriver maintains a set of bindings
for this IPC, but we thought we could simplify things by using a single binding.
This change reverts to a binding set, as ChromeLanguageDetectionClient has
begun causing crashes.

Bug:  757879 
Change-Id: Iec3308b631ed32b4d123e13b4273b7299de63677
Reviewed-on: https://chromium-review.googlesource.com/627521
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Michael Martis <martis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496584}
[modify] https://crrev.com/156f8f09114e82f5d6c9abf35fa2afce7023b052/chrome/browser/language/chrome_language_detection_client.cc
[modify] https://crrev.com/156f8f09114e82f5d6c9abf35fa2afce7023b052/chrome/browser/language/chrome_language_detection_client.h

Comment 8 by siggi@chromium.org, Aug 23 2017

This no longer repros for me in a ToT local build.

Comment 9 by martis@chromium.org, Aug 24 2017

Status: Verified (was: Started)
Now that https://chromium-review.googlesource.com/c/chromium/src/+/623038 has been submitted, browser tests (e.g. https://cs.chromium.org/chromium/src/chrome/browser/translate/translate_manager_browsertest.cc) have been enabled that will fail if this issue reoccurs.

Marking resolved.

Comment 10 by siggi@chromium.org, Aug 24 2017

Labels: Hotlist-dcheck

Sign in to add a comment