Crash in CMFPropertyStore::CreatePropertyStore

RESOLVED FIXED in Firefox -esr52

Status

()

defect
P1
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: philipp, Assigned: kikuo)

Tracking

({crash})

47 Branch
mozilla57
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 wontfix, firefox48 wontfix, firefox49 wontfix, firefox-esr45 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox-esr52 fixed, firefox55 wontfix, firefox56 fixed, firefox57 fixed)

Details

(crash signature)

Attachments

(4 attachments)

This bug was filed from the Socorro interface and is 
report bp-c247f6cb-8978-45ec-b5a6-291bc2160608.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	mfplat.dll 	CMFPropertyStore::CreatePropertyStore(CMFPropertyStore**) 	
1 	mfplat.dll 	LFPoolObjectLockAcquire 	
2 	mfplat.dll 	MFLfx::CPoolLockT<0, 1>::Lock(int) 	
3 	mfplat.dll 	MFLfx::CAutoLockT<MFLfx::CPoolLockT<0, 1>, 1>::CAutoLockT<MFLfx::CPoolLockT<0, 1>, 1>(MFLfx::CPoolLockT<0, 1>*) 	
4 	mfplat.dll 	CWorkQueue::AddThread() 	
5 	mfplat.dll 	CWorkQueue::Initialize(int, long (*)(CWorkQueueClock*, CCompletionPort**)) 	
6 	mfplat.dll 	CMFPresentationDescriptor::Release() 	
7 	mfplat.dll 	CPlatform::Initialize(unsigned long, unsigned long) 	
8 	mfplat.dll 	MFStartup 	
9 	xul.dll 	mozilla::wmf::MFStartup() 	dom/media/platforms/wmf/WMFUtils.cpp:217
10 	xul.dll 	mozilla::CanCreateMFTDecoder 	dom/media/platforms/wmf/WMFDecoderModule.cpp:142
11 	xul.dll 	mozilla::CanCreateWMFDecoder<CLSID_CMSAACDecMFT> 	dom/media/platforms/wmf/WMFDecoderModule.cpp:162
12 	xul.dll 	mozilla::WMFDecoderModule::SupportsMimeType(nsACString_internal const&) 	dom/media/platforms/wmf/WMFDecoderModule.cpp:220
13 	xul.dll 	mozilla::HasGMPFor 	dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp:105
14 	xul.dll 	mozilla::GMPDecoderModule::UpdateUsableCodecs() 	dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp:151
15 	nss3.dll 	PR_Unlock 	nsprpub/pr/src/threads/combined/prulock.c:328
16 	xul.dll 	nsAppShell::ProcessNextNativeEvent(bool) 	widget/windows/nsAppShell.cpp:402

this crash is exclusive to windows 7 & occurring on startup in 75% of cases. it has been around for a while on a low level, but judging from early 47.0 data its volume may be on the increase in 47.

in correlation data, a few modules stick out for this signature:

  CMFPropertyStore::CreatePropertyStore|EXCEPTION_ACCESS_VIOLATION_READ (26 crashes)
    100% (26/26) vs.   1% (126/13230) SortServer2003Compat.dll
    100% (26/26) vs.   2% (214/13230) shunimpl.dll
    100% (26/26) vs.   2% (290/13230) AcGenral.dll
    100% (26/26) vs.   5% (618/13230) AcLayers.dll
    100% (26/26) vs.   6% (817/13230) sfc.dll
    100% (26/26) vs.   8% (1108/13230) sfc_os.dll
Component: Audio/Video → Audio/Video: Playback
Crash volume for signature 'CMFPropertyStore::CreatePropertyStore':
 - nightly(version 50):0 crashes from 2016-06-06.
 - aurora (version 49):0 crashes from 2016-06-07.
 - beta   (version 48):155 crashes from 2016-06-06.
 - release(version 47):811 crashes from 2016-05-31.
 - esr    (version 45):33 crashes from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0       0       0       0       0       0       0
 - aurora        0       0       0       0       0       0       0
 - beta         22      48      14      29      26       8       5
 - release     139      90     114     113     107      80      92
 - esr           0      12       0       0       1       4       4

Affected platform: Windows
Crash volume for signature 'CMFPropertyStore::CreatePropertyStore':
 - nightly (version 51): 0 crashes from 2016-08-01.
 - aurora  (version 50): 0 crashes from 2016-08-01.
 - beta    (version 49): 33 crashes from 2016-08-02.
 - release (version 48): 154 crashes from 2016-07-25.
 - esr     (version 45): 47 crashes from 2016-05-02.

Crash volume on the last weeks (Week N is from 08-22 to 08-28):
            W. N-1  W. N-2  W. N-3
 - nightly       0       0       0
 - aurora        0       0       0
 - beta          6      23       3
 - release      44      45      27
 - esr           7       6       3

Affected platform: Windows

Crash rank on the last 7 days:
           Browser     Content   Plugin
 - nightly
 - aurora
 - beta    #3607
 - release #400
 - esr     #3170
Flags: needinfo?(cpearce)
Assignee: nobody → cpearce
All crashes are on Win7.

Approximate half the crashes have main threads crashing with stacks like this:

CMFPropertyStore::CreatePropertyStore
LFPoolObjectLockAcquire
MFLfx::CPoolLockT<T>::Lock
MFLfx::CAutoLockT<T>::CAutoLockT<T>
CWorkQueue::AddThread
CWorkQueue::Initialize
CMFPresentationDescriptor::Release
CPlatform::Initialize
MFStartup
mozilla::wmf::MFStartup
mozilla::CanCreateMFTDecoder
mozilla::CanCreateWMFDecoder<T>
mozilla::WMFDecoderModule::SupportsMimeType
mozilla::HasGMPFor
mozilla::GMPDecoderModule::UpdateUsableCodecs
PR_Unlock
nsAppShell::ProcessNextNativeEvent 

examples:
13ca20ff-c4e1-4634-a195-974172160919
fb5a3f35-f8c5-4e49-9215-e92842160921

This would be in response to adding a GMP dir, and is setting up the GMP decoding (which is a code path that's disabled by default, and no use on most Win7 machines anyway. This is a startup crash, and the comments indicate it makes Firefox totally and repeatedly fail to start.

The next most common group (about a third) has non-main threads crashing starting up WMF:
CMFPropertyStore::CreatePropertyStore
LFPoolObjectLockAcquire
MFLfx::CPoolLockT<T>::Lock
MFLfx::CAutoLockT<T>::CAutoLockT<T>
CCompletionPort::OnStartThread
CWorkQueue::CThread::ThreadMain
CWorkQueue::CThread::ThreadFunc
_itow_s
_endthreadex
BaseThreadInitThunk
__RtlUserThreadStart
_RtlUserThreadStart 

The examples I looked at had main threads inside the other two main signatures. i.e. in this sig, we're crashing on a WMF thread rather than the main thread, and in the other two sigs we crash on the main thread, but the WMF thread doesn't crash.

examples:
1c866c70-cc86-444a-a2e3-791972160918
8b5dcbcd-464c-4890-8f7f-349192160919


The last main group have crashing main threads such as:

CMFPropertyStore::CreatePropertyStore
LFPoolObjectLockAcquire
MFLfx::CPoolLockT<T>::Lock
MFLfx::CAutoLockT<T>::CAutoLockT<T>
CWorkQueue::AddThread
CWorkQueue::Initialize
CMFPresentationDescriptor::Release
CPlatform::Initialize
MFStartup
mozilla::wmf::MFStartup
mozilla::WMFDecoderModule::Startup
mozilla::PDMFactory::StartupPDM
mozilla::PDMFactory::CreatePDMs
mozilla::PDMFactory::PDMFactory
mozilla::MP4Decoder::CanHandleMediaType
mozilla::DecoderTraits::IsMP4TypeAndEnabled
mozilla::DecoderTraits::CanHandleMediaType
mozilla::dom::HTMLMediaElement::GetCanPlay 


Some of the crashes don't have symbols for their MF.dll, but not all.
Flags: needinfo?(cpearce)
One of the crashes has a comment to the effect of "running in win95 compatibility mode". If I configure that, I can reproduce the crash:

https://crash-stats.mozilla.com/report/index/dae2943c-17d4-4a16-b7d3-f14a12161004
Comment on attachment 8797341 [details]
Bug 1279171 - Add Windows Version checks in wmf::MFStartup() and PDMFactory::CreatePDMs().

https://reviewboard.mozilla.org/r/82940/#review81670
Attachment #8797341 - Flags: review?(matt.woodrow) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b60523131a1c
Add Windows Version checks in wmf::MFStartup() and PDMFactory::CreatePDMs(). r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/b60523131a1c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8797341 [details]
Bug 1279171 - Add Windows Version checks in wmf::MFStartup() and PDMFactory::CreatePDMs().

Approval Request Comment
[Feature/regressing bug #]: Audio/Video playback on Windows
[User impact if declined]: Users who are running Firefox in "Windows 95 compatibility mode" will crash on startup or shortly after, or some time after.
[Describe test coverage new/current, TreeHerder]: We've got lots of media playback mochitests
[Risks and why]: Low; this just adds a Windows version check and disables audio/video playback if Windows isn't running Vista or later, i.e. if we're not in compatibility mode or running on a version of Windows before Windows has system audio/video codecs we can use. (Previously we'd just use the presence of the codec DLLs to determine whether we can play video, but they're present when running in compatibility mode, but they crash when used).
[String/UUID change made/needed]: None.

Note: I had a lot of trouble running in compatibility mode to test this patch, but I was able to reproduce the crash this way. Usually I'd just crash trying to start Firefox in compatibility mode, even with this patch. So if the crash goes away on beta after we land this patch, we should consider investigating how many of our users are running in Win95 compatibility mode, and potentially we should detect that and add whether we're in compatibility mode to crash reports, and if it's showing up a lot, we should do warn the user and tell them to disable compatibility mode or just disable it for them.
Attachment #8797341 - Flags: approval-mozilla-beta?
Attachment #8797341 - Flags: approval-mozilla-aurora?
Comment on attachment 8797341 [details]
Bug 1279171 - Add Windows Version checks in wmf::MFStartup() and PDMFactory::CreatePDMs().

Crash fix, Aurora51+, Beta50+
Attachment #8797341 - Flags: approval-mozilla-beta?
Attachment #8797341 - Flags: approval-mozilla-beta+
Attachment #8797341 - Flags: approval-mozilla-aurora?
Attachment #8797341 - Flags: approval-mozilla-aurora+
Looks like we're still seeing crashes with 50.0b6.

https://mozilla.github.io/stab-crashes/correlations.html?product=Firefox&channel=beta&signature=CMFPropertyStore::CreatePropertyStore
Most of the crashes are happening on Windows 7 and in safe mode.

I wouldn't worry too much though, as the reports are being generated
by a small number of installations.
Looks like most of the crashes have the main thread in mozilla::HasGMPFor. So we might make this go away when we fix Bug 1312540.
(In reply to Chris Pearce (:cpearce) from comment #10)
> Note: I had a lot of trouble running in compatibility mode to test this
> patch, but I was able to reproduce the crash this way. Usually I'd just
> crash trying to start Firefox in compatibility mode, even with this patch.

IsVistaOrLater() uses VerifyVersionInfo() that is not affected by the compatibility mode[1]. You need to use GetVersionEx() to detect the compatibility mode.

Also, even if it is "fixed", the affected users will not be able to playback MP4 videos (although it is better than the startup crash). We should inform users of unsetting the compatibility mode.

> we should do warn the user and tell them to disable compatibility mode or
> just disable it for them.

I agree.

[1] https://code.msdn.microsoft.com/windowsapps/CppCheckOSVersion-ee00c7db
Reopening because the patch has no effect.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1334870
Crash reports are still mentioning old versions of Windows. Why not use an API that includes the compatibility mode, like GetVersionEx?
Flags: needinfo?(cpearce)
Anthony: Someone should try dmajor's suggestion in comment 18.
Assignee: cpearce → nobody
Flags: needinfo?(cpearce) → needinfo?(ajones)
Blake - can you find someone to fix the above patch (already landed) to use GetVersionEx instead of WindowsVersionIsVistaOrLater?
Flags: needinfo?(ajones) → needinfo?(bwu)
Kikuo,
Can you help on this?
Flags: needinfo?(bwu) → needinfo?(kikuo)
Assignee: nobody → kikuo
Flags: needinfo?(kikuo)
Checked the code now, we're not checking windows version since Bug 1324183

The thing to do here will be
1) extending the ability of version checking in mfbt/WindowsVersion.h
2) Adding the version check back in PDMFactory / MFStratup while creating WMFDecoderModule.
GetVersionEx was deprecated and may be alerted or unavailable for the release after Windows 8.1. [1]
I'll use APIs from versionhelpers.h [2] which will also take compatibility mode into account.

[1] https://msdn.microsoft.com/en-Us/library/windows/desktop/ms724451(v=vs.85).aspx
[2] https://msdn.microsoft.com/en-us/library/windows/desktop/ms724429(v=vs.85).aspx
(In reply to Kilik Kuo [:kikuo] from comment #23)
> GetVersionEx was deprecated and may be alerted or unavailable for the
> release after Windows 8.1. [1]
> I'll use APIs from versionhelpers.h [2] which will also take compatibility
> mode into account.

Since VersionHelper functions use VerifyVersionInfo internally, they do NOT take into account compatibility mode. The documentation is wrong (as the MSDN always is).

You will have to resort to GetVersionEx even though the documentation warns. It is practically impossible for MS to change or remove GetVersionEx because too many apps use this function.
(In reply to Kilik Kuo [:kikuo] from comment #23)
> [2] https://msdn.microsoft.com/en-us/library/windows/desktop/ms724429(v=vs.85).aspx

I compiled this sample and verified that the compatibility mode did NOT affect the result.
Yes, unfortunately, I tried that today and got the same result as you mentioned.
And on my laptop (win10), GetVersionEx still works correctly, so I'll go in that way.
Comment on attachment 8900567 [details]
Bug 1279171 - Do not use WMF when application is in Windows pre-2000 compatibility mode.

https://reviewboard.mozilla.org/r/171974/#review177236

According to crash reports, this crash signature happens only on Win7.
Per my testing, FF only crashes when the compatibility mode is set to Win 95/98/ME.
So I preferred to narrow the restriction for that.
Attachment #8900567 - Flags: review?(matt.woodrow)
Comment on attachment 8900567 [details]
Bug 1279171 - Do not use WMF when application is in Windows pre-2000 compatibility mode.

https://reviewboard.mozilla.org/r/171974/#review177354

::: mfbt/WindowsVersion.h:18
(Diff revision 2)
>  
>  namespace mozilla {
>  
>  inline bool
> +IsWin7AndPre2000Compatible() {
> +  bool isWin7 = IsNotWin7PreRTM() && !IsWin8OrLater();

Why do you exclude pre-RTM Win7?

::: mfbt/WindowsVersion.h:27
(Diff revision 2)
> +
> +  OSVERSIONINFOEX info;
> +  ZeroMemory(&info, sizeof(OSVERSIONINFOEX));
> +
> +  info.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
> +  bool success = GetVersionEx((LPOSVERSIONINFO) &info);

Please suppress warning C4996. Otherwise builds will fail when WARNINGS_AS_ERRORS is enabled.
Comment on attachment 8900567 [details]
Bug 1279171 - Do not use WMF when application is in Windows pre-2000 compatibility mode.

https://reviewboard.mozilla.org/r/171974/#review177622

I can't review changes to WindowsVersion.h, maybe try dmajor or frojdnj?

r=me for the WMF specific changes though.
(In reply to Masatoshi Kimura [:emk] from comment #30)
> Comment on attachment 8900567 [details]
> Bug 1279171 - Do not use WMF when application is in Windows pre-2000
> compatibility mode.
> 
> https://reviewboard.mozilla.org/r/171974/#review177354
> 
> ::: mfbt/WindowsVersion.h:18
> (Diff revision 2)
> >  
> >  namespace mozilla {
> >  
> >  inline bool
> > +IsWin7AndPre2000Compatible() {
> > +  bool isWin7 = IsNotWin7PreRTM() && !IsWin8OrLater();
> 
> Why do you exclude pre-RTM Win7?
> 

I checked those report(in the past 6 months, picked around 100 crashes randomly), the crash signature only happened on Win OS version "6.1.7600" or "6.1.7601".
That's why I decided not to include pre-RTM Win 7 builds. (build number <= 7100)

> ::: mfbt/WindowsVersion.h:27
> (Diff revision 2)
> > +
> > +  OSVERSIONINFOEX info;
> > +  ZeroMemory(&info, sizeof(OSVERSIONINFOEX));
> > +
> > +  info.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
> > +  bool success = GetVersionEx((LPOSVERSIONINFO) &info);
> 
> Please suppress warning C4996. Otherwise builds will fail when
> WARNINGS_AS_ERRORS is enabled.

Thanks! will address that.
Comment on attachment 8900567 [details]
Bug 1279171 - Do not use WMF when application is in Windows pre-2000 compatibility mode.

https://reviewboard.mozilla.org/r/171974/#review179036

r=me assuming my understanding of the situation is correct.  I would like to see the comment suggested below before the patch lands.

::: mfbt/WindowsVersion.h:16
(Diff revision 3)
>  inline bool
> +IsWin7AndPre2000Compatible() {

This function could use a comment as to why this function exists and why we're doing unusual things (e.g. we're using `GetVersionEx`, with the corresponding `#pragma` dances, instead of using `VerifyVersionInfo` as all the other functions in this file do).

My impression from reading the bug is that Win7 can actually run apps in some sort of Win2000 compatibility mode...so even though we're using newer Win7 APIs, some things still act as though we're on Win2000?  Is that a more-or-less correct summary?
Attachment #8900567 - Flags: review?(nfroyd) → review+
Comment on attachment 8900567 [details]
Bug 1279171 - Do not use WMF when application is in Windows pre-2000 compatibility mode.

https://reviewboard.mozilla.org/r/171974/#review179036

> This function could use a comment as to why this function exists and why we're doing unusual things (e.g. we're using `GetVersionEx`, with the corresponding `#pragma` dances, instead of using `VerifyVersionInfo` as all the other functions in this file do).
> 
> My impression from reading the bug is that Win7 can actually run apps in some sort of Win2000 compatibility mode...so even though we're using newer Win7 APIs, some things still act as though we're on Win2000?  Is that a more-or-less correct summary?

Thanks for the suggestion, comment added.
And yes, the summary is correct, theoretically.
Comment on attachment 8900567 [details]
Bug 1279171 - Do not use WMF when application is in Windows pre-2000 compatibility mode.

Set r+ from Comment 31
Attachment #8900567 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8900567 [details]
Bug 1279171 - Do not use WMF when application is in Windows pre-2000 compatibility mode.

https://reviewboard.mozilla.org/r/171974/#review180858

Fix try build error.
Attachment #8900567 - Flags: review+
Pushed by kikuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a674f7cbf8c
Do not use WMF when application is in Windows pre-2000 compatibility mode. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/0a674f7cbf8c
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
I assume we want this on at least Beta (maybe ESR52 also). Looks like we'll need rebased patches for that, though.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #42)
> I assume we want this on at least Beta (maybe ESR52 also). Looks like we'll
> need rebased patches for that, though.

Thanks, SGTM, I'll prepare patches for Beta & ESR52.
Flags: needinfo?(kikuo)
Hi Matt, Nathan,

I'd like to uplift this fix to 56_BETA & 52_ESR.
Could you review these 2 patches ? Thanks.
Attachment #8905351 - Flags: review?(nfroyd)
Attachment #8905351 - Flags: review?(matt.woodrow)
Attachment #8905352 - Flags: review?(nfroyd)
Attachment #8905352 - Flags: review?(matt.woodrow)
Comment on attachment 8905351 [details] [diff] [review]
Bug1279171_56_BETA.patch

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

r=me for the WindowsVersion.h change.
Attachment #8905351 - Flags: review?(nfroyd) → review+
Comment on attachment 8905352 [details] [diff] [review]
Bug1279171_52_ESR.patch

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

r=me for the WindowsVersion.h change.
Attachment #8905352 - Flags: review?(nfroyd) → review+
Attachment #8905351 - Flags: review?(matt.woodrow) → review+
Attachment #8905352 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8905351 [details] [diff] [review]
Bug1279171_56_BETA.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1324183
[User impact if declined]:
Crash happens when user launches 32-bit Firefox on Win 7 with compatibility mode 95/98/ME.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, though we stop the crash, but 32-bits Firefox actually cannot do anything on those mode in current nightly. (compatibility mode lower than VISTA)
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Low
[Why is the change risky/not risky?]: We avoid the use of components that results to crash at startup in specific and rare compatibility mode. 
And by my understanding, those mode(95/98/ME) are no longer supported now.
[String changes made/needed]: N/A
Attachment #8905351 - Flags: approval-mozilla-beta?
Comment on attachment 8905352 [details] [diff] [review]
Bug1279171_52_ESR.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Crash happens when user launches 32-bit Firefox on Win 7 with compatibility mode (95/98/ME).
Fix Landed on Version: 57
Risk to taking this patch (and alternatives if risky): 
We avoid the use of components that results to crash at startup in specific and rare compatibility mode . Not sure if those components (WMF libraries) are used by other functionalities (i.e. graphics), so FF may not render correctly, but it's better than getting a crash at startup.
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8905352 - Flags: approval-mozilla-esr52?
Comment on attachment 8905351 [details] [diff] [review]
Bug1279171_56_BETA.patch

Avoid a startup crash. We can give a shot on Beta56. Beta56+.
Attachment #8905351 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8905352 [details] [diff] [review]
Bug1279171_52_ESR.patch

avoid crash in media code when running in some windows compat mode, esr52.4+
Attachment #8905352 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8900567 - Flags: review?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.