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

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Petruta Horea [:phorea], Desktop Test Engineering]

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.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Is this a specific broken DLL version that is broken?

Comment 2

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

All the crashes in the past 28 days are on arch amd64 with msmpeg2vdec.dll version 12.0.9200.16426.

Comment 4

[Petruta Horea [:phorea], Desktop Test Engineering]

Setting status-flags based on Socorro info (only Nightly builds are affected).

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(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?

Comment 6

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

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.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Looks more urgent than my other stuff. I'll have a look.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 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/

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 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/

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=588b30e837a8

Comment 11

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

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?

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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/

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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/

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Robert is unavailable at the moment.

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

Comment 15

[Jim Mathies [:jimm]]

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 {|

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(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.

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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?

Comment 18

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 19

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

Let's blacklist more versions to reduce crashes.

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(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 21

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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/

Comment 22

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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/

Comment 23

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

(Can I "unship" this patch?)

Comment 24

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 25

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

[Jim Mathies [:jimm]]

Comment on attachment 8720248 [details]
MozReview Request: Bug 1242343 - p1. ConstructSystem32Path from LoadLibrarySystem32 - r?jimm

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

Comment 27

[Jim Mathies [:jimm]]

(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.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56e2ad84a18f
https://hg.mozilla.org/integration/mozilla-inbound/rev/90f70787de03

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56e2ad84a18f
https://hg.mozilla.org/mozilla-central/rev/90f70787de03

Comment 30

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

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.