crash in msmpeg2vdec.dll@0x25a487 and @0x1230b2

RESOLVED FIXED in Firefox 47

Status

()

defect
P1
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: petruta.rasa, Assigned: gerald)

Tracking

({crash})

46 Branch
mozilla47
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 unaffected, firefox45 unaffected, firefox46 unaffected, firefox47 fixed)

Details

(crash signature)

Attachments

(2 attachments)

This bug was filed from the Socorro interface and is 
report bp-ad834f25-b4c4-41d3-a56a-0b0752160122.
=============================================================

Follow-up from bug 1205083 to track the remaining crashes.
Is this a specific broken DLL version that is broken?
Flags: needinfo?(cpearce)
Priority: -- → P2
All the crashes in the past 28 days are on arch amd64 with msmpeg2vdec.dll version 12.0.9200.16426.
Flags: needinfo?(cpearce)
Duplicate of this bug: 1242767
Setting status-flags based on Socorro info (only Nightly builds are affected).
(In reply to PTO until Feb 23 - Chris Pearce (:cpearce) from comment #2)
> All the crashes in the past 28 days are on arch amd64 with msmpeg2vdec.dll
> version 12.0.9200.16426.

We should blacklist the DLL and fall back to the Adobe decoder. Can you find someone to do that?
Flags: needinfo?(cpearce)
Priority: P2 → P1
Gerald: Can you look at this? One way to do this would be to add a check on the msmpeg2vdec.dll version number against a blacklist in WMFDecoderModule::SupportsMimeType() when it's called for H.264 types.
Flags: needinfo?(cpearce) → needinfo?(gsquelart)
Looks more urgent than my other stuff. I'll have a look.
Assignee: nobody → gsquelart
Flags: needinfo?(gsquelart)
Split system32 path construction from LoadLibrarySystem32.

Review commit: https://reviewboard.mozilla.org/r/35239/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35239/
Attachment #8720248 - Flags: review?(robert.strong.bugs)
When checking for H264 availability, answer 'no' when the installed
msmpeg2vdec.dll is version 12.0.9200.16426, as it appeared to crash a lot.

Review commit: https://reviewboard.mozilla.org/r/35241/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35241/
Attachment #8720249 - Flags: review?(cpearce)
Comment on attachment 8720249 [details]
MozReview Request: Bug 1242343 - p2. Blacklist msmpeg2vdec.dll 12.0.9200.16426 & .17037 - r=cpearce

https://reviewboard.mozilla.org/r/35241/#review31959

::: dom/media/platforms/wmf/WMFDecoderModule.cpp:198
(Diff revision 1)
> +  if (IsH264DecoderBlacklisted()) {

All the crash reports we have are on amd64 (I think?), so we can only blacklist in 64bit builds, right?
Attachment #8720249 - Flags: review?(cpearce) → review+
Comment on attachment 8720248 [details]
MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35239/diff/1-2/
Attachment #8720248 - Attachment description: MozReview Request: Bug 1242343 - ConstructSystem32Path from LoadLibrarySystem32 - r?rstrong → MozReview Request: Bug 1242343 - ConstructSystem32Path from LoadLibrarySystem32 - r?jimm
Attachment #8720248 - Flags: review?(robert.strong.bugs) → review?(jmathies)
Comment on attachment 8720249 [details]
MozReview Request: Bug 1242343 - p2. Blacklist msmpeg2vdec.dll 12.0.9200.16426 & .17037 - r=cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35241/diff/1-2/
Attachment #8720249 - Attachment description: MozReview Request: Bug 1242343 - Blacklist msmpeg2vdec.dll 12.0.9200.16426 - r?cpearce → MozReview Request: Bug 1242343 - Blacklist msmpeg2vdec.dll 12.0.9200.16426 - r=cpearce
Robert is unavailable at the moment.

Jim, please let me know if you cannot review this.
Attachment #8720248 - Flags: review?(jmathies) → review+
Comment on attachment 8720248 [details]
MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm

https://reviewboard.mozilla.org/r/35239/#review33993

::: xpcom/base/nsWindowsHelpers.h:153
(Diff revision 2)
>    // If GetSystemPath fails we accept that we'll load the DLLs from the

I don't understand this comment, where are we calling 'GetSystemPath', is this comment out of date?

::: xpcom/base/nsWindowsHelpers.h:155
(Diff revision 2)
> -  GetSystemDirectoryW(systemPath, MAX_PATH + 1);
> +  GetSystemDirectoryW(aSystemPath, aSize);

Lets add an assert for an invalid aSystemPath on entry.

Also lets set the first char to null before calling GetSystemDirectoryW, or check the return value of GetSystemDirectoryW instead.

::: xpcom/base/nsWindowsHelpers.h:171
(Diff revision 2)
> -    systemPath[MAX_PATH] = L'\0';
> +  else {

nit, use |} else {|
(In reply to Jim Mathies [:jimm] from comment #15)
> Comment on attachment 8720248 [details]
[...]

Thank you for the thorough review. I was hoping to get a rubber stamp on this quick refactor! But you're right, we might as well tidy up this code, especially as it is now more public.
To be safe with all the changes, I'll push another review soon, thanks in advance.
Signature [@ msmpeg2vdec.dll@0x1230b2 ] (originally followed in bug 1219319 comment 10, which was deemed duplicate of bug 120583*, which landed in 43) still shows crashes in 44.0a1 and 47.0a1 (but none in between!?)
https://crash-stats.mozilla.com/report/list?signature=msmpeg2vdec.dll%400x1230b2#tab-sigsummary

* Bug 120583 comment 40 (which follows signature [@ msmpeg2vdec.dll@0x25a487 ]) noticed crashes in 45.0a1 and 46.0a1, and gave rise to this bug here (comment 0).

Now, crashes with signature @0x1230b2 are also all on amd64, but the DLL version is 12.0.9200.17037 (very close behind 12.0.9200.16426, which we are blacklisting here).
Should we also blacklist it, or investigate further?
What do you think, Chris?
Flags: needinfo?(cpearce)
That DLL 12.0.9200.17037 is also involved in all crashes @0x17229d:
https://crash-stats.mozilla.com/signature/?version=47.0a1&signature=msmpeg2vdec.dll%400x17229d#reports
(Though it's only a sample of 2 reports at this time.)

So blacklisting it could take care of a few more crashes too.
Let's blacklist more versions to reduce crashes.
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #19)
> Let's blacklist more versions to reduce crashes.

Cool, thanks.
I'll carry over the r+ as it's a tiny change. Feel free to have a look at the change on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d3ccf215f3b
Comment on attachment 8720248 [details]
MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35239/diff/2-3/
Attachment #8720248 - Attachment description: MozReview Request: Bug 1242343 - ConstructSystem32Path from LoadLibrarySystem32 - r?jimm → MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm
Comment on attachment 8720249 [details]
MozReview Request: Bug 1242343 - p2. Blacklist msmpeg2vdec.dll 12.0.9200.16426 & .17037 - r=cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35241/diff/2-3/
Attachment #8720249 - Attachment description: MozReview Request: Bug 1242343 - Blacklist msmpeg2vdec.dll 12.0.9200.16426 - r=cpearce → MozReview Request: Bug 1242343 - p2. Blacklist msmpeg2vdec.dll 12.0.9200.16426 & .17037 - r=cpearce
Comment on attachment 8720248 [details]
MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm

(I don't see a way to change 'r+' to 'r-' in Review Board)

Jim, please re-review as it's an extensive re-write of ConstructSystem32Path().
Flags: needinfo?(jmathies)
Attachment #8720248 - Flags: review+ → review?(jmathies)
I meant: I don't see a way to change 'r+' to 'r?' in Review Board. I've opened bug 1252757 about this.
Comment on attachment 8720248 [details]
MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm

https://reviewboard.mozilla.org/r/35239/#review34129
Attachment #8720248 - Flags: review?(jmathies) → review+
(In reply to Gerald Squelart [:gerald] from comment #25)
> I meant: I don't see a way to change 'r+' to 'r?' in Review Board. I've
> opened bug 1252757 about this.

yeah reviewboard is still pretty painful. I find myself avoiding it more and more.
Flags: needinfo?(jmathies)
Crash Signature: [@ msmpeg2vdec.dll@0x25a487] → [@ msmpeg2vdec.dll@0x25a487] [@ msmpeg2vdec.dll@0x1230b2]
Summary: crash in msmpeg2vdec.dll@0x25a487 → crash in msmpeg2vdec.dll@0x25a487 and @0x1230b2
https://hg.mozilla.org/mozilla-central/rev/56e2ad84a18f
https://hg.mozilla.org/mozilla-central/rev/90f70787de03
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8720248 [details]
MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm

Request to uplift to 46 (currently Aurora)

Approval Request Comment
[Feature/regressing bug #]: MP4 playback on Win64
[User impact if declined]: Crashes if the system's MP4 decoding DLL is a 'bad' one
[Describe test coverage new/current, TreeHerder]: Lots of media tests (which exercise that code on our win64 tries), aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ab107d63ed
[Risks and why]: Small risk, it is a fairly small patch with lots of checks and fallbacks.
[String/UUID change made/needed]: None

Note: The 1st patch attached to this bug applies as-is on Aurora, but with an offset warning due to other unrelated changes higher up in that file; it's safe.
Attachment #8720248 - Flags: approval-mozilla-aurora?
Comment on attachment 8720248 [details]
MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm

Fix for mp4 crash, looks ok on try. Please uplift to aurora
Attachment #8720248 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8720248 [details]
MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm

After talking with gerald, let's hold off on uplift and re-think this next week for beta.
Attachment #8720248 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
Crash Signature: [@ msmpeg2vdec.dll@0x25a487] [@ msmpeg2vdec.dll@0x1230b2] → [@ msmpeg2vdec.dll@0x25a487] [@ msmpeg2vdec.dll@0x1230b2] [@ msmpeg2vdec.dll@0x17229d]
Blocks: 1254389
You need to log in before you can comment on or make changes to this bug.