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

RESOLVED FIXED in Firefox 47

Status

()

P1
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: petruta.rasa, Assigned: gerald)

Tracking

(Blocks: 1 bug, {crash})

46 Branch
mozilla47
All
Windows
crash
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).
status-firefox44: --- → unaffected
status-firefox45: --- → unaffected
status-firefox46: affected → unaffected
status-firefox47: --- → 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)
(Assignee)

Comment 7

3 years ago
Looks more urgent than my other stuff. I'll have a look.
Assignee: nobody → gsquelart
Flags: needinfo?(gsquelart)
(Assignee)

Comment 8

3 years ago
Created attachment 8720248 [details]
MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm

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)
(Assignee)

Comment 9

3 years ago
Created attachment 8720249 [details]
MozReview Request: Bug 1242343 - p2. Blacklist msmpeg2vdec.dll 12.0.9200.16426 & .17037 - r=cpearce

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+
(Assignee)

Comment 12

3 years ago
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)
(Assignee)

Comment 13

3 years ago
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
(Assignee)

Comment 14

3 years ago
Robert is unavailable at the moment.

Jim, please let me know if you cannot review this.

Updated

3 years ago
Attachment #8720248 - Flags: review?(jmathies) → review+

Comment 15

3 years ago
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 {|
(Assignee)

Comment 16

3 years ago
(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.
(Assignee)

Comment 17

3 years ago
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)
(Assignee)

Comment 18

3 years ago
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)
(Assignee)

Comment 20

3 years ago
(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
(Assignee)

Comment 21

3 years ago
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
(Assignee)

Comment 22

3 years ago
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
(Assignee)

Comment 23

3 years ago
https://reviewboard.mozilla.org/r/35239/#review34083

(Can I "unship" this patch?)
(Assignee)

Comment 24

3 years ago
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)
(Assignee)

Comment 25

3 years ago
I meant: I don't see a way to change 'r+' to 'r?' in Review Board. I've opened bug 1252757 about this.

Comment 26

3 years ago
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+

Comment 27

3 years ago
(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)
(Assignee)

Updated

3 years ago
Crash Signature: [@ msmpeg2vdec.dll@0x25a487] → [@ msmpeg2vdec.dll@0x25a487] [@ msmpeg2vdec.dll@0x1230b2]
Summary: crash in msmpeg2vdec.dll@0x25a487 → crash in msmpeg2vdec.dll@0x25a487 and @0x1230b2

Comment 29

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/56e2ad84a18f
https://hg.mozilla.org/mozilla-central/rev/90f70787de03
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 30

3 years ago
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-
(Assignee)

Updated

3 years ago
Crash Signature: [@ msmpeg2vdec.dll@0x25a487] [@ msmpeg2vdec.dll@0x1230b2] → [@ msmpeg2vdec.dll@0x25a487] [@ msmpeg2vdec.dll@0x1230b2] [@ msmpeg2vdec.dll@0x17229d]
(Assignee)

Updated

3 years ago
Blocks: 1254389
You need to log in before you can comment on or make changes to this bug.