1430931 - appear.in hits MOZ_CRASH(ArrayBufferInputStream not thread-safe)

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Byron Campen [:bwc]]

Looks like bug 1426875 broke appear.in somehow. Likely related to the media stack, but hard to say for sure yet.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

baku, can you look?

Comment 3

[Andrea Marchesini [:baku]]

bwc, do we have a crash report? That would be help to know what is going on here.

Comment 4

[Byron Campen [:bwc]]

I do, but it does not actually show the stack that caused the crash, it shows something that happened later:

https://crash-stats.mozilla.com/report/index/9dda7edd-d302-43ff-88ad-e11db0180117

I am also not getting a stack on stdout/err. Let me see if I can reproduce on linux.

Comment 5

[Byron Campen [:bwc]]

No luck reproducing on linux ASAN.

Comment 6

[Andrea Marchesini [:baku]]

(In reply to Byron Campen [:bwc] from comment #4)
> I do, but it does not actually show the stack that caused the crash, it
> shows something that happened later:

This crash report doesn't show too much. But do you maybe have an STR? Are you able to reproduce this crash?

Comment 7

[Byron Campen [:bwc]]

I am able to reproduce 100% reliably on debug OS X builds, by opening a tab-to-tab call on appear.in (go to appear.in, create a room, follow the prompts, and once you're in, copy the link and open it in an additional tab, again following the prompts).

Comment 8

[Nils Ohlmeier [:drno]]

Making it P1 until we know what is going on here.

Comment 9

[Byron Campen [:bwc]]

Ok, here's a stack. It looks like this is a case of letting go of the last reference to something on the wrong thread.

  * frame #0: 0x000000010001e1a5 libmozglue.dylib`::MOZ_CrashOOL(aFilename="", aLine=46, aReason="ArrayBufferInputStream not thread-safe") + 37 at Assertions.cpp:33
    frame #1: 0x000000010254b760 XUL`nsAutoOwningThread::AssertCurrentThreadOwnsMe(this=<unavailable>, msg=<unavailable>) const + 48 at nsISupportsImpl.cpp:46
    frame #2: 0x0000000102650ac0 XUL`ArrayBufferInputStream::Release() [inlined] void nsAutoOwningThread::AssertOwnership<39>(this=<unavailable>, aMsg=<unavailable>) [39]) const + 32 at nsISupportsImpl.h:69
    frame #3: 0x0000000102650ab4 XUL`ArrayBufferInputStream::Release(this=0x000000012331e130) + 20 at ArrayBufferInputStream.cpp:13
    frame #4: 0x00000001025b734c XUL`nsTArray_Impl<nsMultiplexInputStream::StreamData, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) [inlined] nsCOMPtr<nsIInputStream>::~nsCOMPtr() + 220 at nsCOMPtr.h:426
    frame #5: 0x00000001025b7328 XUL`nsTArray_Impl<nsMultiplexInputStream::StreamData, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) [inlined] nsCOMPtr<nsIInputStream>::~nsCOMPtr() at nsCOMPtr.h:423
    frame #6: 0x00000001025b7328 XUL`nsTArray_Impl<nsMultiplexInputStream::StreamData, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) [inlined] nsMultiplexInputStream::StreamData::~StreamData() + 79 at nsMultiplexInputStream.cpp:60
    frame #7: 0x00000001025b72d9 XUL`nsTArray_Impl<nsMultiplexInputStream::StreamData, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) [inlined] nsMultiplexInputStream::StreamData::~StreamData() at nsMultiplexInputStream.cpp:60
    frame #8: 0x00000001025b72d9 XUL`nsTArray_Impl<nsMultiplexInputStream::StreamData, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) [inlined] nsTArrayElementTraits<nsMultiplexInputStream::StreamData>::Destruct(nsMultiplexInputStream::StreamData*) + 27 at nsTArray.h:530
    frame #9: 0x00000001025b72be XUL`nsTArray_Impl<nsMultiplexInputStream::StreamData, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) [inlined] nsTArray_Impl<nsMultiplexInputStream::StreamData, nsTArrayInfallibleAllocator>::DestructRange(aStart=<unavailable>, aCount=1) at nsTArray.h:1994
    frame #10: 0x00000001025b72be XUL`nsTArray_Impl<nsMultiplexInputStream::StreamData, nsTArrayInfallibleAllocator>::RemoveElementsAt(this=0x0000000116f627e0, aStart=<unavailable>, aCount=1) + 78 at nsTArray.h:2047
    frame #11: 0x00000001025b71cd XUL`nsMultiplexInputStream::~nsMultiplexInputStream() [inlined] nsTArray_Impl<nsMultiplexInputStream::StreamData, nsTArrayInfallibleAllocator>::Clear() + 221 at nsTArray.h:1720
    frame #12: 0x00000001025b71b5 XUL`nsMultiplexInputStream::~nsMultiplexInputStream() [inlined] nsTArray_Impl<nsMultiplexInputStream::StreamData, nsTArrayInfallibleAllocator>::~nsTArray_Impl() at nsTArray.h:853
    frame #13: 0x00000001025b71b5 XUL`nsMultiplexInputStream::~nsMultiplexInputStream() [inlined] nsTArray<nsMultiplexInputStream::StreamData>::~nsTArray(this=0x0000000116f627e0) at nsTArray.h:2221
    frame #14: 0x00000001025b71b5 XUL`nsMultiplexInputStream::~nsMultiplexInputStream() [inlined] nsTArray<nsMultiplexInputStream::StreamData>::~nsTArray(this=0x0000000116f627e0) at nsTArray.h:2221
    frame #15: 0x00000001025b71b5 XUL`nsMultiplexInputStream::~nsMultiplexInputStream(this=0x0000000116f62740) + 197 at nsMultiplexInputStream.cpp:80
    frame #16: 0x00000001025a61ee XUL`nsMultiplexInputStream::Release() [inlined] nsMultiplexInputStream::~nsMultiplexInputStream(this=0x0000000116f62740) + 94 at nsMultiplexInputStream.cpp:79
    frame #17: 0x00000001025a61e6 XUL`nsMultiplexInputStream::Release(this=0x0000000116f62740) + 86 at nsMultiplexInputStream.cpp:126
    frame #18: 0x000000010268c17e XUL`nsAsyncStreamCopier::~nsAsyncStreamCopier() [inlined] nsAsyncStreamCopier::~nsAsyncStreamCopier(this=0x000000012238c580) + 14 at nsAsyncStreamCopier.cpp:82
    frame #19: 0x000000010268c179 XUL`nsAsyncStreamCopier::~nsAsyncStreamCopier(this=0x000000012238c580) + 9 at nsAsyncStreamCopier.cpp:82
    frame #20: 0x00000001025b8fe6 XUL`nsAStreamCopier::Process(this=<unavailable>) + 886 at nsStreamUtils.cpp:408
    frame #21: 0x00000001025b6f5f XUL`nsAStreamCopier::Run(this=0x0000000128b22d40) + 15 at nsStreamUtils.cpp:447
    frame #22: 0x00000001025b705d XUL`non-virtual thunk to nsAStreamCopier::Run(this=<unavailable>) + 13 at nsStreamUtils.cpp:445
    frame #23: 0x00000001025e90b7 XUL`nsThread::ProcessNextEvent(this=0x0000000114aff450, aMayWait=<unavailable>, aResult=0x00007000004ab757) + 2119 at nsThread.cpp:1040
    frame #24: 0x00000001025fa6df XUL`NS_ProcessNextEvent(aThread=<unavailable>, aMayWait=true) + 47 at nsThreadUtils.cpp:517
    frame #25: 0x00000001026eeab4 XUL`mozilla::net::nsSocketTransportService::Run(this=0x0000000102091820) + 1220 at nsSocketTransportService2.cpp:961
    frame #26: 0x00000001026ef72d XUL`non-virtual thunk to mozilla::net::nsSocketTransportService::Run(this=<unavailable>) + 13 at nsSocketTransportService2.cpp:831
    frame #27: 0x00000001025e90b7 XUL`nsThread::ProcessNextEvent(this=0x0000000114aff450, aMayWait=<unavailable>, aResult=0x00007000004abde7) + 2119 at nsThread.cpp:1040
    frame #28: 0x00000001025fa6df XUL`NS_ProcessNextEvent(aThread=<unavailable>, aMayWait=false) + 47 at nsThreadUtils.cpp:517
    frame #29: 0x0000000102b96f0b XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x0000000114a8f300, aDelegate=0x0000000102028e90) + 235 at MessagePump.cpp:334
    frame #30: 0x0000000102b4c005 XUL`MessageLoop::Run() [inlined] MessageLoop::RunHandler(this=<unavailable>) + 69 at message_loop.cc:319
    frame #31: 0x0000000102b4c000 XUL`MessageLoop::Run(this=<unavailable>) + 64 at message_loop.cc:299
    frame #32: 0x00000001025e694d XUL`nsThread::ThreadFunc(aArg=<unavailable>) + 381 at nsThread.cpp:423
    frame #33: 0x0000000102249940 libnss3.dylib`_pt_root(arg=0x000000010201ea00) + 288 at ptthread.c:201
    frame #34: 0x00007fff9628d99d libsystem_pthread.dylib`_pthread_body + 131
    frame #35: 0x00007fff9628d91a libsystem_pthread.dylib`_pthread_start + 168
    frame #36: 0x00007fff9628b351 libsystem_pthread.dylib`thread_start + 13

Comment 10

[Byron Campen [:bwc]]

Yeah, this looks like we're sending a ref to a nsIMultiplexInputStream over to STS, and it is released there.

Comment 11

[Andrea Marchesini [:baku]]

Note: I don't think the regression is related to bug 1426875 because that is related to TCPSocket and that API is disabled by pref.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: arrayBuffer.patch

The problem here is that ArrayBufferInputStream is not marked as thread-safe. Any nsIInputStream should be thread-safe because, often, the reading happens on separate threads.

The stream is actually read on an I/O thread, but, when deleted on that thread, it crashs because the refcounting is now thread-safe.

If you see, in ::SetData() a full copy of the array buffer content is taken.
I just added an assertion to be sure that SetData() is called on the owning thread.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Does the patch actually fix the appear.in issue? I assume the patch just fixes some debug build only crash.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8943878 [details] [diff] [review]
arrayBuffer.patch

So, this is fine, but I doubt this fixes the issue bwc is seeing.

Comment 15

[Andrea Marchesini [:baku]]

bwc, is this a debug only issue? Are you able to reproduce this crash on a opt build?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9746b74a13b1
mark nsIArrayBufferInputStream as thread-safe, r=smaug

Comment 17

[Byron Campen [:bwc]]

(In reply to Andrea Marchesini [:baku] from comment #11)
> Note: I don't think the regression is related to bug 1426875 because that is
> related to TCPSocket and that API is disabled by pref.

It is. The previous revision is fine 5 times out of 5, whereas the revision from bug 1426875 crashes 5 times out of 5. It is being caused by the code here:

https://hg.mozilla.org/mozilla-central/rev/ad809e54748e#l1.107

This change causes that nsIMultiplexInputStream to be dispatched to STS, where it turns out to be the last ref standing, causing a bunch of things (including some ArrayBufferInputStream) to be destroyed on STS.

Comment 18

[Byron Campen [:bwc]]

(In reply to Andrea Marchesini [:baku] from comment #15)
> bwc, is this a debug only issue? Are you able to reproduce this crash on a
> opt build?

This has only crashed on debug for me.

Comment 19

[Byron Campen [:bwc]]

The patch landed on m-i has fixed the problem for me. I am assuming the "leave-open" was left for me to clear if it fixed the problem.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #11)
> Note: I don't think the regression is related to bug 1426875 because that is
> related to TCPSocket and that API is disabled by pref.

You probably found this out after saying this, but webrtc uses TCPSocket:
https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/media/mtransport/nr_socket_prsock.cpp#1980

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

IIRC the thread-safety checks are debug-only, so crashing on debug is to be expected.  And on non-debug it could do Bad Things if you're unlucky.

Thread-safe refcounts are noticeably more expensive.

Comment 22

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9746b74a13b1