Closed Bug 1279136 Opened 6 years ago Closed 5 years ago

Crash in @0x0 | CEnum::GetAdapterDisplayModeImpl

Categories

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

x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: n.nethercote, Assigned: mattwoodrow)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-7c498652-daae-4db2-bb67-b4a342160602.
=============================================================

1,368 crashes with this signature in the past 7 days. All on Windows 7. Looks like problems starting D3D9.

Top five versions:

> 1  38.8.0esr  828  60.53 %
> 2  46.0.1     129   9.43 %
> 3  45.1.1esr  117   8.55 %
> 4  45.1.0esr  69    5.04 %
> 5  43.0       65    4.75 %

Top five CPUs:

> 1   GenuineIntel family 6 model 61 stepping 4 | 4   631   46.13 %
> 2   GenuineIntel family 6 model 58 stepping 9 | 4   309   22.59 %
> 3   GenuineIntel family 6 model 60 stepping 3 | 4   134    9.80 %
> 4   GenuineIntel family 6 model 69 stepping 1 | 4   106    7.75 %
> 5   GenuineIntel family 6 model 42 stepping 7 | 4    86    6.29 %

dvander, any ideas?
Flags: needinfo?(dvander)
All the crashes have Intel graphic adapters.
Video related, probably a duplicate of one of the bugs being handled as we speak.
Component: Graphics → Audio/Video
Flags: needinfo?(dvander)
Component: Audio/Video → Audio/Video: Playback
There are a lot of driver versions affected, all Intel.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> There are a lot of driver versions affected, all Intel.

That could be the result of the signature itself; something like this, on Nvidia, looks to be the same issue: https://crash-stats.mozilla.com/report/index/b02af673-32e2-4c3b-a510-b03c52160623
Interestingly enough, when calling CreateDeviceEx, we ran afoul of this:

"(Parameter hFocusWindow) - The focus window alerts Direct3D when an application switches from foreground mode to background mode. For full-screen mode, the window specified must be a top-level window. For windowed mode, this parameter may be NULL only if the hDeviceWindow member of pPresentationParameters is set to a valid, non-NULL value..."

From what I can tell, in https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/wmf/DXVA2Manager.cpp#308, we have both hFocusWindow parameter (third parameter to CreateDeviceEx), as well as hDeviceWindow in params set to nullptr.

But my knowledge of this code is 10 minutes old, so I'm probably misreading it.
Flags: needinfo?(matt.woodrow)
And, yes, we have a comment in our code that says "we're doing this even though the documentation says we shouldn't, but so is Chrome, so we should be OK".  Still, these crashes look like they're a result of a nullptr...
Given that we crash in GetAdapterDisplayMode, and we probably only call that because we pass D3DFMT_UNKNOWN, I wonder what would happen if we called GetAdapterDisplayMode ourselves, then pass the result to CreateDevice.  If we'd crash in GetAdapterDisplayMode when we make the call ourselves.  It would at least give us some more information.
Or even more interestingly, call GetDisplayMode instead.
The offending change was to make accelerated decoding work under e10s.
See Also: → 1217185
It's not obvious that GetAdapterDisplayMode would depend on the window, so I don't see how that could crash.

It's quite believable able that passing D3DFMT_UNKNOWN is why it's being called though, so we could just try passing a normal value (doesn't particularly matter, we never actually present with this device) to see if that helps.

I double checked chrome and they seem to be passing exactly the same parameters (including D3DFMT_UNKNOWN), though they sometimes skip this code entirely and use the device contained within ANGLE. It's not clear how often each code path runs, so maybe this isn't actually affecting them.
Flags: needinfo?(matt.woodrow)
Crash volume for signature '@0x0 | CEnum::GetAdapterDisplayModeImpl':
 - nightly (version 50): 0 crash from 2016-06-06.
 - aurora  (version 49): 1 crash from 2016-06-07.
 - beta    (version 48): 20 crashes from 2016-06-06.
 - release (version 47): 640 crashes from 2016-05-31.
 - esr     (version 45): 5921 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          0          0          0          0
 - aurora           1          0          0          0          0          0          0
 - beta             4          1          2          4          3          4          2
 - release         98         80        100         96         86        101         20
 - esr           1049        604        574        724        723        678        377

Affected platform: Windows
It seems pretty reasonable to believe that creating the device with D3DFMT_UNKNOWN is what is triggering the driver to call GetAdapterDisplayMode to figure it out, and then that crashes. The docs explicitly say that if BackBufferFormat is set to D3DFMT_UNKNOWN then it will be modified to contain the actual format during the call to CreateDevice(Ex).

We don't have similar crashes with the primary d3d9 device, and that uses ARGB as the format.

We never actually present anything with this device, we only use it for video decoding so the format shouldn't matter.
Assignee: nobody → matt.woodrow
Attachment #8777635 - Flags: review?(cpearce)
Comment on attachment 8777635 [details] [diff] [review]
d3d9-create-device-crash

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

Please use mozreview. It's easier on reviewers.
Attachment #8777635 - Flags: review?(cpearce) → review+
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e283da610b05
Specify a format for our D3D9 video device to avoid the driver needing to compute one. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/e283da610b05
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
The good news is, this crash seems to have gone away on nightly.  The bad news - the last build that has the crash is 20160802, so if we fixed it, it wasn't this that fixed it :)  If I had to guess, I'd guess bug 1289640 took care of it.
Hi Matt, Milan, which fix should we consider uplifting to Aurora50? This or bug 1289640 or both?
Flags: needinfo?(milan)
Flags: needinfo?(matt.woodrow)
Pretty sure this one didn't move the needle, but it seems like a safe enough fix.  Bug 1289640 is likely the one that did take care of it, but that's a larger change, and there are still some open questions (e.g., bug 1289640 comment 21), so I'm not sure we want the uplift quite yet.  Tough spot - let's see what Matt thinks.
Flags: needinfo?(milan)
I definitely don't want to uplift bug 1289640 yet, probably not at all. As Milan said, way too many open questions.

We can uplift this, but if it hasn't made much difference then there isn't much point.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> I definitely don't want to uplift bug 1289640 yet, probably not at all. As
> Milan said, way too many open questions.
> 
> We can uplift this, but if it hasn't made much difference then there isn't
> much point.

Ok. Thanks Milan and Matt. I agree, this is now a wontfix for 49 and 50.
You need to log in before you can comment on or make changes to this bug.