Help debugging a crash triggered by Receiver::reset()

[Matthew Jones]

Hey chromium-mojo,

I built a cross-platform internals page for my team that uses mojo to communicate with the browser. I've been seeing crashes from iOS on the dashboard that seem to originate from this reset call on the receiver:

(Chrome -cxx_atomic_impl.h:329) void std::__Cr::__cxx_atomic_store<unsigned char>(std::__Cr::__cxx_atomic_base_impl<unsigned char>*, unsigned char, std::__Cr::memory_order)
(Chrome -atomic_base.h:52) std::__Cr::__atomic_base<unsigned char, false>::store(unsigned char, std::__Cr::memory_order)
(Chrome -atomic_flag.cc:23) base::AtomicFlag::Set()
(Chrome -weak_ptr.cc:33) base::internal::WeakReference::Flag::Invalidate()
(Chrome -weak_ptr.cc:100) base::internal::WeakReferenceOwner::Invalidate()
(Chrome -weak_ptr.h:452) base::WeakPtrFactory<mojo::internal::BindingStateBase>::InvalidateWeakPtrs()
(Chrome -binding_state.cc:61) mojo::internal::BindingStateBase::Close()
(Chrome -receiver.h:113) mojo::Receiver<commerce::mojom::CommerceInternalsHandlerFactory, mojo::RawPtrImplRefTraits<commerce::mojom::CommerceInternalsHandlerFactory>>::reset()
(Chrome -commerce_internals_ui_base.cc:20) commerce::CommerceInternalsUIBase::BindInterface(mojo::PendingReceiver<commerce::mojom::CommerceInternalsHandlerFactory>)

...

Presumably it's safe to call .reset() on the receiver - I've seen other setups using the same pattern. Does this crash generally point to something incorrectly set up in my implementation? Should I not be trying to re-bind the receiver if it's already bound?

Thanks in advance,

Matt

[Daniel Cheng]

Is there a bug # or some crash reports that people can look at?

Daniel

--
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-moj...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAM7y2MMDkxETu8ktzz2%2BZ25OU7jXURA_XmgDBWLeZQDXc0vMow%40mail.gmail.com.

[Matthew Jones]

Ah I should have included those to begin with - the crash dashboard is here and the associated bug is here.

- Matt

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAF3XrKqa%2B42jS5DF8tccu%2BY0saUSYKaey2dGAPO_LWHpLL2oSA%40mail.gmail.com.

[Daniel Cheng]

I took a brief look at the crashes and I don't have a good theory.

It's crashing in InvalidateWeakPtrs called from here:
void BindingStateBase::Close() {
  if (!router_)
    return;

  weak_ptr_factory_.InvalidateWeakPtrs();

  endpoint_client_.reset();
  router_->CloseMessagePipe();
  router_ = nullptr;
}

Based on this, we can assume `this` in BindingStateBase probably isn't null. And the crash offset is at 0x4, which makes sense if the scoped_refptr<Flag> inside WeakReferenceOwner is somehow null (Flag is refcounted; refcounts are 32-bits, and so the atomic would be at 0x4 offset). But I don't think there's any time where we leave the Flag null anymore in WeakReferenceOwner: it's constructed with a Flag, and if we call Invalidate() explicitly, we reset it with a new Flag object... so I'm at a loss to say how this could happen.

Daniel

[Dave Tapuska]

My best guess when looking at it was that the base::Unretained(this) is incorrect here...

https://source.chromium.org/chromium/chromium/src/+/main:components/commerce/ios/browser/commerce_internals_ui.mm;l=38;drc=3e02eb658e84b46ebb53667e0a1cb52fe53b5f87;bpv=1;bpt=1

Have you tried using a WeakPtrFactory here?

dave.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAF3XrKq-b4ZzQ1OVWbfNC0OwRwiqPn7CeJ4ot%2B%3D5PTbQojvJ4g%40mail.gmail.com.

[Ken Rockot]

On Wednesday, June 26, 2024 at 7:52:20 AM UTC-7 dtap...@chromium.org wrote:

My best guess when looking at it was that the base::Unretained(this) is incorrect here...

https://source.chromium.org/chromium/chromium/src/+/main:components/commerce/ios/browser/commerce_internals_ui.mm;l=38;drc=3e02eb658e84b46ebb53667e0a1cb52fe53b5f87;bpv=1;bpt=1

Have you tried using a WeakPtrFactory here?

+1 the crashes look to me like a UAF on the CommerceInternalsUI object, hence the Receiver is in an undefined state and anything can happen.

[Matthew Jones]

Thanks for the advice, everyone! Having that pointed out, I wonder why it wasn't my first instinct... I'll update my implementation (and probably the one I referenced) and keep an eye on the dashboard.

Thanks again!

Matt