1126088 - Firefox crash in OOM | large | mozalloc_abort(char const*) | mozalloc_handle_oom(unsigned long) | moz_xmalloc | mozilla::MP4Stream::BlockingReadIntoCache(long long, unsigned long, mozilla::Monitor*)

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-d115f97e-aadf-4582-9624-f51bc2150121.
=============================================================

Seen while looking at crash stats and in the explosive report as well. These crashes are seen on both Windows and Mac. Should this crash be affecting Mac users if it is not turned on in Beta?

Some comments in the Mac crashes:

Firefox crashes when watching long YouTube videos and trying to skip about.
Playing a You-tube video--this has been the same problem (different video) during the last 4 or so crashes.  


Frame 	Module 	Signature 	Source
0 	libmozalloc.dylib 	mozalloc_abort(char const*) 	memory/mozalloc/mozalloc_abort.cpp
1 	libmozalloc.dylib 	mozalloc_handle_oom(unsigned long) 	memory/mozalloc/mozalloc_oom.cpp
2 	libmozalloc.dylib 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp
3 	XUL 	mozilla::MP4Stream::BlockingReadIntoCache(long long, unsigned long, mozilla::Monitor*) 	obj-firefox/x86_64/dist/include/mozilla/mozalloc.h
4 	XUL 	mp4_demuxer::MP4Sample* mozilla::InvokeAndRetry<mp4_demuxer::MP4Demuxer, mp4_demuxer::MP4Sample*>(mp4_demuxer::MP4Demuxer*, mp4_demuxer::MP4Sample* (mp4_demuxer::MP4Demuxer::*)(), mozilla::MP4Stream*, mozilla::Monitor*) 	dom/media/fmp4/MP4Reader.cpp
5 	XUL 	mozilla::MP4Reader::Update(mp4_demuxer::TrackType) 	dom/media/fmp4/MP4Reader.cpp
6 	XUL 	nsRunnableMethodImpl<void (mozilla::MP4Reader::*)(mp4_demuxer::TrackType), mp4_demuxer::TrackType, true>::Run() 	obj-firefox/x86_64/dist/include/nsThreadUtils.h
7 	XUL 	mozilla::MediaTaskQueue::Runner::Run() 	dom/media/MediaTaskQueue.cpp
8 	XUL 	nsThreadPool::Run() 	xpcom/threads/nsThreadPool.cpp
9 	XUL 	_ZThn8_N12nsThreadPool3RunEv 	obj-firefox/x86_64/xpcom/threads/Unified_cpp_xpcom_threads0.cpp
10 	XUL 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
11 	XUL 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
12 	XUL 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
13 	XUL 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
14 	XUL 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp
15 	libnss3.dylib 	_pt_root 	nsprpub/pr/src/pthreads/ptthread.c
Ø 16 	libsystem_pthread.dylib 	libsystem_pthread.dylib@0x32fb 	
Ø 17 	libsystem_pthread.dylib 	libsystem_pthread.dylib@0x3278 	
Ø 18 	libsystem_pthread.dylib 	libsystem_pthread.dylib@0x14b0 	
19 	libnss3.dylib 	libnss3.dylib@0x203a3f 	

Show/hide other threads

Comment 1

[Jean-Yves Avenard [:jya]]

As InvokeAndRetry cache the data it reads, it potentially allocates the entire amount of data we are about to re-read.

doubling the memory usage.

would be nice to be able to instead pin the internal memory blocks as used by the SourceBufferResource.

Comment 2

[Jean-Yves Avenard [:jya]]

With the first part of bug 1096089, we don't ever attempt to demux a frame that we know we don't yet have (with the exception of decoding the first frame, which can be changed easily)...

So the issue of blocking reads should be mostly gone, as we always have all the compressed/unmuxed data in memory.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #1)
> As InvokeAndRetry cache the data it reads, it potentially allocates the
> entire amount of data we are about to re-read.
> 
> doubling the memory usage.

Yes - it's a kludgy solution designed to get us through 36.
 
> would be nice to be able to instead pin the internal memory blocks as used
> by the SourceBufferResource.

The problem is that in the non-MSE case, the backing store is in the MediaCache, which doesn't actually give us the ability to reliably pin data smaller than one cache block in size.

Post-36, we should just rip out this whole thing and replace it with the demuxer strategy you proposed several weeks ago (where we just demux as the data arrives, and put all the frames into a tree).

Comment 4

[Jean-Yves Avenard [:jya]]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #3)
> Post-36, we should just rip out this whole thing and replace it with the
> demuxer strategy you proposed several weeks ago (where we just demux as the
> data arrives, and put all the frames into a tree).

It's a big task, I doubt it will be ready for 37 or even 38.

In the mean time, we can fairly easily implement Pin for SourceBufferResource, where no data would be evicted while pinned, but instead eviction requests queued, and only evicted once unpinned. so now no need to use a temporary cache, we know the old data will always be there.

For non-mp4, the old strategy of simply using a proper blocking read should be sufficient (as it worked fine before)

Comment 5

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> In the mean time, we can fairly easily implement Pin for
> SourceBufferResource, where no data would be evicted while pinned, but
> instead eviction requests queued, and only evicted once unpinned. so now no
> need to use a temporary cache, we know the old data will always be there.

This would work, yes, but see below.

> For non-mp4, the old strategy of simply using a proper blocking read should
> be sufficient (as it worked fine before)

We never did it before. We just had a race condition. I don't think we should reintroduce blocking reads while holder the demuxer monitor.

What we _could_ do would be to have two different sets of behavior for MP4Stream, depending on whether the underlying MediaResource can properly implement Pin or not.

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> With the first part of bug 1096089, we don't ever attempt to demux a frame
> that we know we don't yet have (with the exception of decoding the first
> frame, which can be changed easily)...
> 
> So the issue of blocking reads should be mostly gone, as we always have all
> the compressed/unmuxed data in memory.

More on this...

As we only now ever attempt to demux data we know we have in memory, we never reach BlockingReadIntoCache as the previous call to Demux*Sample will always succeed unless there's an actual error.

So seeing this crash once bug 1096089 gets uplifted should be gone

Comment 7

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> As we only now ever attempt to demux data we know we have in memory, we
> never reach BlockingReadIntoCache as the previous call to Demux*Sample will
> always succeed unless there's an actual error.
> 
> So seeing this crash once bug 1096089 gets uplifted should be gone

Only for the non-MSE case, right?

Anyway, this should be trivial to fix - let me just do it.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Fallibly allocate MP4Stream CacheBlocks. v1

This should do it. Flagging njn to make sure I got the fallible |new| piece
right - note that this thing will be deallocated using delete [] via the
nsAutoArrayPtr.

Comment 9

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=635a8b9c9147

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8555052 [details] [diff] [review]
Fallibly allocate MP4Stream CacheBlocks. v1

Review of attachment 8555052 [details] [diff] [review]:
-----------------------------------------------------------------

The allocation stuff looks fine.

::: dom/media/fmp4/MP4Stream.h
@@ +80,5 @@
> +      mBuffer = new ((fallible_t())) uint8_t[mCount];
> +      return !!mBuffer;
> +    }
> +
> +    char* Buffer() { MOZ_ASSERT(mBuffer.get()); return reinterpret_cast<char*>(mBuffer.get()); }

Nit: Probably better to avoid having multiple statements on a single line.

Also, the mixture of |char| and |uint8_t| used for this field is a bit weird, though I see it's pre-existing.

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8555052 [details] [diff] [review]
Fallibly allocate MP4Stream CacheBlocks. v1

Review of attachment 8555052 [details] [diff] [review]:
-----------------------------------------------------------------

I was confused at first with the extra parenthesis (and MDN doesn't include them in their examples)

Comment 12

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4056ebeecf61

Comment 13

[Nicholas Nethercote [inactive]]

jya was talking about the extra parens here:

+ mBuffer = new ((fallible_t())) char[mCount];

Oh well.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> jya was talking about the extra parens here:
> 
> + mBuffer = new ((fallible_t())) char[mCount];
> 
> Oh well.

They are necessary, and how things are done elsewhere in the tree as well. If I don't, I get:

 0:00.77 /files/mozilla/repos/a/dom/media/fmp4/MP4Stream.h:80:22: error: cannot allocate function type 'mozilla::fallible_t ()' with new
 0:00.78       mBuffer = new (fallible_t()) char[mCount];

Can you explain why it's necessary?

Comment 15

[Jean-Yves Avenard [:jya]]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #14)
> (In reply to Nicholas Nethercote [:njn] from comment #13)
> > jya was talking about the extra parens here:
> > 
> > + mBuffer = new ((fallible_t())) char[mCount];
> > 
> > Oh well.
> 
> They are necessary, and how things are done elsewhere in the tree as well.
> If I don't, I get:
> 
>  0:00.77 /files/mozilla/repos/a/dom/media/fmp4/MP4Stream.h:80:22: error:
> cannot allocate function type 'mozilla::fallible_t ()' with new
>  0:00.78       mBuffer = new (fallible_t()) char[mCount];
> 
> Can you explain why it's necessary?

Yeah, I got that error too when I attempted to remove the extra parenthesis.
The example I was referring to was:
https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation 
" fArray = new (fallible_t()) Foo[];"

Funny, because the usual std::nothrow commonly used with the new operator, doesn't require those parenthesis; yet fallible_t has the exact same definition as nothrow and does

Comment 16

[Mike Hommey [:glandium]]

Nothing should be using fallible_t(). They should be using fallible. Like so:
new (fallible) char[mCount];

Comment 17

[Mike Hommey [:glandium]]

(In reply to Jean-Yves Avenard [:jya] from comment #15)
> Funny, because the usual std::nothrow commonly used with the new operator,
> doesn't require those parenthesis; yet fallible_t has the exact same
> definition as nothrow and does

fallible_t has the same definition as nothrow_t, not nothrow. You don't do new (std::nothrow_t()), you do new (std::nothrow).

And yes, we do lack a global definition of fallible.

Comment 18

[Nicholas Nethercote [inactive]]

So something like this is necessary:

    static const fallible_t fallible = fallible_t();
    mArgv = new (fallible) JS::Heap<JS::Value>[argc];

Sorry, bholley, I should have passed this review on to glandium.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4056ebeecf61

Comment 20

[Bobby Holley (:bholley)]

(In reply to Nicholas Nethercote [:njn] from comment #18)
> So something like this is necessary:
> 
>     static const fallible_t fallible = fallible_t();
>     mArgv = new (fallible) JS::Heap<JS::Value>[argc];
> 
> Sorry, bholley, I should have passed this review on to glandium.

glandium, does this mean that my code as-landed is wrong? What does it do? Can you update the docs to say the right thing?

Comment 21

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #18)
> So something like this is necessary:
> 
>     static const fallible_t fallible = fallible_t();

without static.

>     mArgv = new (fallible) JS::Heap<JS::Value>[argc];
> 
> Sorry, bholley, I should have passed this review on to glandium.

Comment 22

[Nicholas Nethercote [inactive]]

bholley, I just filed bug 1126593 to fix up the fallible new mess in general, so there's no need for you to do anything extra here.

Comment 23

[Mike Hommey [:glandium]]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #20)
> (In reply to Nicholas Nethercote [:njn] from comment #18)
> > So something like this is necessary:
> > 
> >     static const fallible_t fallible = fallible_t();
> >     mArgv = new (fallible) JS::Heap<JS::Value>[argc];
> > 
> > Sorry, bholley, I should have passed this review on to glandium.
> 
> glandium, does this mean that my code as-landed is wrong? What does it do?
> Can you update the docs to say the right thing?

It's not wrong per se. In practice, it does the same thing, and it should even generate the same machine code, assuming the compilers are smart.

Comment 24

[Ralph Giles (:rillian)]

Comment on attachment 8555052 [details] [diff] [review]
Fallibly allocate MP4Stream CacheBlocks. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Crashes viewing YouTube video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This changes memory allocation for mp4 playback on Windows/Mac, so there's some risk of side-effects. It is isolated though.
[String/UUID change made/needed]: None.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f655a983cfc8
https://hg.mozilla.org/releases/mozilla-beta/rev/f421202b153f

Comment 26

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro shows zero crashes with these signatures ([1] & [2]) after the fix landed.

[1] - https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=OOM+%7C+large+%7C+mozalloc_abort%28char+const%2A%29+%7C+mozalloc_handle_oom%28unsigned+long%29+%7C+moz_xmalloc+%7C+mozilla%3A%3AMP4Stream%3A%3ABlockingReadIntoCache%28long+long%2C+unsigned+long%2C+mozilla%3A%3AMonitor%2A%29#tab-reports
[2] - https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=OOM+%7C+large+%7C+mozalloc_abort%28char+const%2A+const%29+%7C+mozalloc_handle_oom%28unsigned+int%29+%7C+moz_xmalloc+%7C+mozilla%3A%3AMP4Stream%3A%3ABlockingReadIntoCache%28__int64%2C+unsigned+int%2C+mozilla%3A%3AMonitor%2A%29#tab-reports