839031 - crash in mozilla::wmf::MFCreateSourceReaderFromByteStream @ mfflac with FLAC 1.2.0

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Scoobidiver (away)]

It has been hit by two users in 21.0a1/20130206 and occurs with FLAC 1.2.0.
The current FLAC version is 1.2.1. See http://sourceforge.net/projects/flac/

Assuming it's a regression, the regression range might be:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2360c3c46aca&tochange=bc108d2ce8d1
I suspect bug 804387 or bug 836076.

Signature 	mfflac.dll@0x5555 More Reports Search
UUID	cf737cd6-bb99-4179-bde2-753282130207
Date Processed	2013-02-07 11:45:09
Uptime	117
Last Crash	5.0 hours before submission
Install Age	6.1 hours since version was first installed.
Install Time	2013-02-07 05:40:35
Product	Firefox
Version	21.0a1
Build ID	20130206031027
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 30 stepping 5
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x68a0, AdapterSubsysID: 043a1028, AdapterDriverVersion: 8.763.0.0
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
Processor Notes 	sp-processor07.phx1.mozilla.com_2973:2008
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x68a0
Total Virtual Memory	4294836224
Available Virtual Memory	3070312448
System Memory Use Percentage	50
Available Page File	9017135104
Available Physical Memory	3192606720

Frame 	Module 	Signature 	Source
0 	mfFLAC.dll 	mfFLAC.dll@0x5555 	
1 	mfreadwrite.dll 	CMFSourceReader::CreateInstance 	
2 	mfreadwrite.dll 	CMFSourceReader::CreateInstanceFromByteStream 	
3 	mfreadwrite.dll 	CMFSourceReader::CreateInstanceFromObject 	
4 	mfreadwrite.dll 	CMFReadWriteClassFactory::CreateInstanceFromObject 	
5 	mfreadwrite.dll 	ATL::AtlComPtrAssign 	
6 	mfreadwrite.dll 	MFCreateSourceReaderFromMediaSource 	
7 	xul.dll 	mozilla::wmf::MFCreateSourceReaderFromByteStream 	content/media/wmf/WMFUtils.cpp:328
8 	xul.dll 	mozilla::WMFReader::ReadMetadata 	content/media/wmf/WMFReader.cpp:438
9 	xul.dll 	mozilla::MediaDecoderStateMachine::DecodeMetadata 	content/media/MediaDecoderStateMachine.cpp:1796
10 	xul.dll 	mozilla::MediaDecoderStateMachine::DecodeThreadRun 	content/media/MediaDecoderStateMachine.cpp:481
11 	xul.dll 	nsRunnableMethodImpl<void 	obj-firefox/dist/include/nsThreadUtils.h:367
12 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
13 	xul.dll 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:265
14 	nspr4.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:395
15 	nspr4.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:90
16 	msvcr100.dll 	_callthreadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314
17 	msvcr100.dll 	_threadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292
18 	kernel32.dll 	BaseThreadInitThunk 	
19 	ntdll.dll 	__RtlUserThreadStart 	
20 	ntdll.dll 	_RtlUserThreadStart

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mfflac.dll%400x5555

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we should be using FLAC at all here.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ah, I can reproduce this by installing the flac MFT ftom http://mfflac.sourceforge.net/

We're crashing inside MFCreateSourceReaderFromByteStream(). The codec whitelist relies on the source reader being created as the source reader sniffs the media types.

The mfFLAC.dll is calling IMFByteStream::Read() (the synchronous version) which I haven't implemented since MS's WMF decoders don't call it. I guess I should implement IMFByteStream::Read(), as then the codec whitelist will work as expected.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That sounds worth doing. However, I also think we should be aggressive about blacklisting codec DLLs that we don't want to be used in the Firefox process. That will reduce our footprint and attack surface.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

The crash we're seeing is when our backend is trying to load MP3's when the FLAC WMF codec is intalled.

We get decoded frames from an IMFSourceReader. The IMFSourceReader gets demuxed undecoded samples from an IMFMediaSource object. In the case relevant to us, an IMFMediaSource objects reads raw bytes from Necko via our implementation of IMFByteStream that wraps MediaResource.

WMF creates an IMFMediaSource object for an IMFByteStream by using the SourceResolver. The SourceResolver checks if the IMFByteStream implements IMFAttributes and has the MF_BYTESTREAM_CONTENT_TYPE attribute. If so, it looks in the Registry for an IMFByteStreamHandler that handles that MIME type. The entry for that MIME type contains the CLSID of an IMFByteStreamHandler which WMF instantiates. The IMFByteStreamHandler instance then sniffs the IMFByteStream, and if it can demux the stream it does so, otherwise the handler reports failure to WMF, which presumably tries other handlers in some sort of order until it finds one which can handle the stream.

Note that this means that if an IMFByteStream reports an incorrect MIME type, WMF's SourceResolver will instantiate the IMFByteStreamHandler for the reported MIME type, and when the handler fails, the SourceResolver will keep searching for an IMFByteStreamHandler for the stream. This means that if a file of a Content-Type which we don't want to support is served with a Content-Type that we do support, we'll still end up instantiating that codec's IMFByteStreamHandler and running its sniffing code, if it's installed.

The only way that I can see of preventing the IMFByteStreamHandler from being invoked would be to remove the registry entry which maps the MIME type to that IMFByteStreamHandler. Obviously we don't want to do this permanently, and I'm not aware of an API to do this temporarily for our application only. We can also blacklist their DLLs.

Because our IMFByteStream doesn't implement IMFAttributes and report its Content-Type, WMF's SourceResolver is instantiating IMFByteStreamHandlers until it can find one that handles the stream. For MP3s the FLAC IMFByteStreamHandler (i.e. the FLAC "deumxer" object) is being invoked before the MP3 handler is, and FLAC's IMFByteStreamHandler is calling IMFByteStream::Read() which we don't implement and FLAC's handler crashes because it can't handle that properly.

Once we implement IMFByteStream::Read() properly, it looks like FLAC's IMFByteStreamHandler is not reporting that it can't handle the byte stream to WMF properly (it reads the entire stream, presumably looking for magic bytes). So when FLAC's handler is supposed to sniff the MP3 file the SourceResolver thinks FLAC's handler can handle the file, but because the handler doesn't output any data or errors, WMF assumes the MP3 stream is FLAC, but corrupt and doesn't try any other handlers, so the MP3 doesn't play.

So, fix this bug and to reduce our attach surface we should:

1. Implement IMFByteStream::Read() on WMFByteStream, so that other IMFByteStreamHandlers don't trip up on this in future.
2. Implement IMFAttributes on WMFByteStream and set the MF_BYTESTREAM_CONTENT_TYPE attribute. This means that WMF's SourceResolver should be able to instantiate the correct IMFByteStreamHandler on its first attempt, for streams that are served with or are sniffed for the correct Content-Type. This may speed up source resolving (i.e. loading) too.
3. Whitelist WMF decoders that we want to enable, so that even if we end up instantiating a codec's IMFByteStreamHandler, we block it's decoders from being created and run. We can do this using IMFMFTEnum and IMFPluginControl.
4. On a case-by-case basis, we can also block the DLLs for codecs which we don't want to load the IMFByteStreamHandler.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1 v1: Implement WMFByteStream::Read()

Implement WMFByteStream::Read(), and rename AsyncReadRequest to ReadRequest.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2 v1: Store contentType on MediaResource

Store ContentType on MediaResource rather than having a master copy in nsHTMLMediaElement. We mostly use it in MediaResource anyway, and it seems logical to keep it there.

We need the content type visible to WMFByteStream so that we can have the WMFByteStream's IMFAttribute report it to the SourceResolver as I described above.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3 v1: Implement IMFAttributes on WMFByteStream

Implement IMFAttributes on WMFByteStream, report content type as attribute.

I just create an IMFAttribute object and add the content-type attribute to that, and forward the interface's calls to the object. That's the recommended approach to implementing this interface.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 4 v1: Whitelist WMF codecs

Enumerate the decoders on the system, and disable all except the H.264, AAC and MP3 decoders.

I also removed a call to wmf::MFShutdown() from LoadDLLs which wasn't needed, since we'd not have called wmf::MFStartup() yet on all code paths to there.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 5: Blacklist FLAC codec DLL

We should disable the FLAC decoder since it still has the bug where it reads the entire stream looking for magic bytes and doesn't report that it can't play when it sniffs.

We'll hit this if someone serves a file of an unsupported type with a supported Content-Type, the SourceResolver will still instantiate the FLAC decoder which will try to sniff.

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 712752 [details] [diff] [review]
Patch 1 v1: Implement WMFByteStream::Read()

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

::: content/media/wmf/WMFByteStream.h
@@ +79,4 @@
>  private:
>  
>    // Locks the MediaResource and performs the read. This is a helper
>    // for ProcessReadRequest().

This comment is kind of out of date, then, since you use this method to implement the sync read.

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 712755 [details] [diff] [review]
Patch 2 v1: Store contentType on MediaResource

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

::: content/media/BufferMediaResource.h
@@ +135,5 @@
>    }
>  
>    bool IsTransportSeekable() MOZ_OVERRIDE { return true; }
>  
> +  virtual const nsACString& GetContentType() const MOZ_OVERRIDE {

nit: local style is to put the brace on its own line.

::: content/media/MediaResource.h
@@ +397,5 @@
>    {
>      MOZ_COUNT_DTOR(BaseMediaResource);
>    }
>  
> +  virtual const nsACString& GetContentType() const MOZ_OVERRIDE {

nit: brace on its own line.

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 712757 [details] [diff] [review]
Patch 3 v1: Implement IMFAttributes on WMFByteStream

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

::: content/media/wmf/WMFByteStream.cpp
@@ +193,5 @@
>    if (aIId == IID_IUnknown) {
>      return DoGetInterface(static_cast<IMFByteStream*>(this), aInterface);
>    }
> +  if (aIId == IID_IMFAttributes) {
> +    return DoGetInterface(static_cast<IMFAttributes*>(this), aInterface); 

nit: trailing space.

@@ +590,5 @@
> +}
> +
> +STDMETHODIMP
> +WMFByteStream::Compare(IMFAttributes* pTheirs, 
> +                       MF_ATTRIBUTES_MATCH_TYPE MatchType, 

nit: trailing spaces *2

::: content/media/wmf/WMFByteStream.h
@@ +13,5 @@
>  #include "mozilla/ReentrantMonitor.h"
>  #include "mozilla/Attributes.h"
>  #include "nsAutoPtr.h"
> +#include "mozilla/RefPtr.h"
> + 

nit: trailing space.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #5)
> Created attachment 712752 [details] [diff] [review]
> Patch 1 v1: Implement WMFByteStream::Read()
> 
> Implement WMFByteStream::Read(), and rename AsyncReadRequest to ReadRequest.

It seems that this patch made the orange that I spent weeks fixing earlier re-appear:
https://tbpl.mozilla.org/?tree=Try&rev=5684b8fdf456

I will investigate...

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 712759 [details] [diff] [review]
Patch 4 v1: Whitelist WMF codecs

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

::: content/media/wmf/WMFUtils.cpp
@@ +204,5 @@
> +static bool
> +IsSupportedDecoder(const GUID& aDecoderGUID)
> +{
> +  return aDecoderGUID == CLSID_CMSH264DecoderMFT ||
> +         aDecoderGUID == CLSID_CMSAACDecMFT ||

CLSID_CMSAACDecMFT isn't defined in the Windows7.0 SDK, so I'll need to DEFINE_GUID this if it's not already defined.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 712752 [details] [diff] [review]
Patch 1 v1: Implement WMFByteStream::Read()

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

::: content/media/wmf/WMFByteStream.cpp
@@ +481,3 @@
>  {
>    LOG("WMFByteStream::Read()");
> +  ReadRequest request(mOffset, aBuffer, aBufferLength);

I should be taking the lock here before accessing mOffset. D'oh!

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

There's a patch in bug 830171 to fix the orange I'm seeing on Try/tbpl. I imagine we won't get these patches all landed before the next uplift, so we'll need to request Aurora approval once these have been green on m-c for a few days.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83e2c0975a62
https://hg.mozilla.org/integration/mozilla-inbound/rev/e50df4a601e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/40f6ab9cf2f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/48bf677726b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/987a4c1c0a68

Comment 18

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/83e2c0975a62
https://hg.mozilla.org/mozilla-central/rev/e50df4a601e4
https://hg.mozilla.org/mozilla-central/rev/40f6ab9cf2f3
https://hg.mozilla.org/mozilla-central/rev/48bf677726b8
https://hg.mozilla.org/mozilla-central/rev/987a4c1c0a68

Comment 19

[Philip Chee]

https://tbpl.mozilla.org/php/getParsedLog.php?id=19883074&tree=Thunderbird-Trunk#error0

WMFUtils.obj : error LNK2001: unresolved external symbol _CLSID_CMP3DecMediaObject
WMFUtils.obj : error LNK2001: unresolved external symbol _CLSID_CMSH264DecoderMFT
xul.dll : fatal error LNK1120: 2 unresolved externals

Thunderbird and SeaMonkey build with webRTC disabled.

Comment 20

[Makoto Kato [:m_kato]]

(In reply to Philip Chee from comment #19)
> https://tbpl.mozilla.org/php/getParsedLog.php?id=19883074&tree=Thunderbird-
> Trunk#error0
> 
> WMFUtils.obj : error LNK2001: unresolved external symbol
> _CLSID_CMP3DecMediaObject
> WMFUtils.obj : error LNK2001: unresolved external symbol
> _CLSID_CMSH264DecoderMFT
> xul.dll : fatal error LNK1120: 2 unresolved externals
> 
> Thunderbird and SeaMonkey build with webRTC disabled.

We need link wmcodecdspuuid even if no MOZ_WEBRTC_IN_LIBXUL.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 6: link wmcodecdspuuid

Oops! We can link wmcodecdspuuid in toolkit/library, where we link mfuuid.

Comment 22

[Makoto Kato [:m_kato]]

Comment on attachment 715888 [details] [diff] [review]
Patch 6: link wmcodecdspuuid

thanks!

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed the seamonkey/comm-central fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d91f463c35a

My earlier patches are on Aurora now, do we need this changeset on other branches?

Comment 24

[Philip Chee]

> My earlier patches are on Aurora now, do we need this changeset on other branches?
I think this needs to be landed on whichever branches the original patchsets have landed. But don't quote me on this.

Comment 25

[Justin Wood (:Callek)]

Comment on attachment 715888 [details] [diff] [review]
Patch 6: link wmcodecdspuuid

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug
User impact if declined: Thunderbird and SeaMonkey unable to build; Firefox unable to build if WebRT is not enabled
Testing completed (on m-c, etc.): Patch is on m-c with no negatives
Risk to taking this patch (and alternatives if risky): None known
String or UUID changes made by this patch: None

Comment 26

[Justin Wood (:Callek)]

(In reply to Philip Chee from comment #24)
> > My earlier patches are on Aurora now, do we need this changeset on other branches?
> I think this needs to be landed on whichever branches the original patchsets
> have landed. But don't quote me on this.

It does need to land on aurora, flagged for approval.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d91f463c35a

Comment 28

[Alex Keybl [:akeybl]]

Comment on attachment 715888 [details] [diff] [review]
Patch 6: link wmcodecdspuuid

I'm not sure when MOZ_WMF is/isn't used, so just making sure we verify this bug specifically (no quickly reproducible regressions using Flac on Windows).

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Alex Keybl [:akeybl] from comment #28)
> Comment on attachment 715888 [details] [diff] [review]
> Patch 6: link wmcodecdspuuid
> 
> I'm not sure when MOZ_WMF is/isn't used, so just making sure we verify this
> bug specifically (no quickly reproducible regressions using Flac on Windows).

So this is clear: MOZ_WMF is defined in Windows builds only. This patch just prevents a build error which occurs when MOZ_WEBRTC is not also defined (which only occurs in comm-central). The error occurs because I didn't try building on a tree with MOZ_WEBRTC disabled, since m-c has MOZ_WEBRTC enabled, and MOZ_WEBRTC links a library that MOZ_WMF needs but didn't link itself.

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Alex Keybl [:akeybl] from comment #28)
> Comment on attachment 715888 [details] [diff] [review]
> Patch 6: link wmcodecdspuuid

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffe9335853f8

Comment 31

[Justin Wood (:Callek)]

(In reply to Makoto Kato from comment #20)
> (In reply to Philip Chee from comment #19)
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=19883074&tree=Thunderbird-
> > Trunk#error0
> > 
> > WMFUtils.obj : error LNK2001: unresolved external symbol
> > _CLSID_CMP3DecMediaObject
> > WMFUtils.obj : error LNK2001: unresolved external symbol
> > _CLSID_CMSH264DecoderMFT
> > xul.dll : fatal error LNK1120: 2 unresolved externals
> > 
> > Thunderbird and SeaMonkey build with webRTC disabled.
> 
> We need link wmcodecdspuuid even if no MOZ_WEBRTC_IN_LIBXUL.

And actually this patch wasn't enough, we're still broken on trunk (when we re-enabled WebRTC, by backing out our workaround)

Comment 32

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20100101 Firefox/21.0

Reproduced this crash on Nightly (buildID: 20130206031027).

STR:
1. Install mfFLAC.msi from http://sourceforge.net/projects/mfflac/files/latest/download?source=files
2. Open Firefox and load http://archive.org/details/testmp3testfile
3. Click the play button from the mp3 test sample.

Verified as fixed in Firefox 21 beta 3 (buildID: 20130416200523).

No new crashes were found in Socorro: 
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mfflac.dll%400x5555&reason_type=contains&date=04%2F17%2F2013%2013%3A20%3A42&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mfflac.dll%400x5555#reports