Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#5380 closed Bug (fixed)

MPC-HC is unable to create dump file

Reported by: gcraistlin Owned by: Underground78
Priority: normal Milestone: 1.7.9
Component: General Version: 1.7.8
Severity: normal Keywords:
Cc: Underground78, xhmikosr, kasper93 Evaluation: diagnosed

Description

I don't know how to force MPC-HC to crash. The steps below crash MPC-HC in WinXP but not in Win7. So if you don't have access to WinXP you have to force MPC-HC to crash some other way.

  1. Download and open the test DVD (it's the same as in ticket 5378): http://rusfolder.com/43468326.
  2. Press Left (the default button for "Jump Backward Medium" action).

MPC-HC will freeze (it is expected and not the fault of MPC-HC) and maybe show the error (see attach). Anyway, it'll create zero-length %AppData%\MPC-HC\mpc-hc.exe.1.7.8.162.dmp - instead of "normal" dump file.

Attachments (3)

MPC-HC_dump_error.png (2.7 KB) - added by gcraistlin 5 years ago.
mpc-hc.png.png (391.9 KB) - added by gcraistlin 5 years ago.
mpc-hc.exe.1.7.8.173.dmp (114.6 KB) - added by gcraistlin 5 years ago.

Download all attachments as: .zip

Change History (32)

Changed 5 years ago by gcraistlin

Attachment: MPC-HC_dump_error.png added

comment:1 Changed 5 years ago by Underground78

Cc: Underground78 added
Evaluation: need info
Status: newevaluated

I don't think this is an easily reproducible bug. In most cases the dump will be created successfully. In some rare cases, the dump creation will indeed fail because the crash corrupted too many things to even be able to create the dump. It's not really something we can control but it should be pretty rare anyway.

However your case is pretty strange because of "error 0". Usually 0 is the code that means no error so there might be something fishy going on. Can you try this test build and tell me if the error message is the same?

comment:2 Changed 5 years ago by gcraistlin

I don't think this is an easily reproducible bug.

Zero-length dump file is reproducible on WinXP virtual machine (just checked it).

With the test build, the error code is 2147942487. I've discovered how to get the error message on my working system for sure: it's the question of the time elevator position (see attach). Though I can't reproduce the same on the virtual WinXP machine.

FYI:

 Volume in drive C has no label.
 Volume Serial Number is BCB2-500E

 Directory of C:\Documents and Settings\Raistlin\Application Data\MPC-HC

16.04.2015  22:18    <DIR>          .
16.04.2015  22:18    <DIR>          ..
16.04.2015  22:06                91 default.mpcpl
09.04.2014  00:47            53 193 mpc-hc.exe.1.7.3.201.dmp
04.07.2014  01:35            56 603 mpc-hc.exe.1.7.5.195.dmp
13.04.2015  17:17                 0 mpc-hc.exe.1.7.8.152.dmp
16.04.2015  13:23                 0 mpc-hc.exe.1.7.8.162.dmp
16.04.2015  22:06                 0 mpc-hc.exe.1.7.8.171.dmp
               6 File(s)        109 887 bytes
               2 Dir(s)  74 226 016 256 bytes free

Changed 5 years ago by gcraistlin

Attachment: mpc-hc.png.png added

comment:3 Changed 5 years ago by Underground78

This test build will crash when clicking on the "Donate" menu item, is the minidump correctly created in this case?

comment:4 Changed 5 years ago by gcraistlin

No, zero-length again.

comment:5 Changed 5 years ago by Underground78

Interesting. Could you download this dll file, put it in the same folder than the test build v2 and try again?

comment:6 Changed 5 years ago by gcraistlin

This time OK.

Changed 5 years ago by gcraistlin

Attachment: mpc-hc.exe.1.7.8.173.dmp added

comment:7 Changed 5 years ago by Underground78

Cc: xhmikosr added
Evaluation: need infodiagnosed

@XhmikosR: It would be better that we distribute our own dgbhelp.dll with MPC-HC. It's what is recommended by MS and it's the only way to ensure that everything works fine on all platforms. However it would add an extra ~400kb to the packages.

Otherwise we will need to add fallback code to produce less interesting dumps but more widely supported.

comment:8 Changed 5 years ago by xhmikosr

Some thoughts.

  1. 400KB with LZMA2?
  2. How are we going to deploy this? I mean, will we add code to get it from the SDK on build time or commit the DLLs? I really hope the former :P
  3. Perhaps it's time to rethink about using https://drdump.com/crash-reporting-system like TortoiseSVN?

comment:9 Changed 5 years ago by Underground78

  1. Exactly 370KB with LZMA2 for the 32-bit DLL.
  2. That I don't know because I'm not sure from where we should get it. I resolved to download it from TortoiseGit repository because MS website is crappy...
  3. Well it would require the same DLL plus some others so the impact on size would be greater. Also I don't know if we can fine tune the way the dumps are created (but possibly it's possible). More importantly I don't know how it would integrate in our current workflow. I mean a dump is really useful but it's even better if we have an associated ticket to interact with the user. We would probably get more dumps but I'm not sure how well we would be able to take advantage of that.

comment:10 Changed 5 years ago by kasper93

It's what is recommended by MS and it's the only way to ensure that everything works fine on all platforms.

Which platforms you have in mind? I believe on Vista+ included dgbhelp.dll in windows dir should be new enough for our purpose.

That I don't know because I'm not sure from where we should get it. I resolved to download it from TortoiseGit repository because MS website is crappy...

According to Microsoft they ship redistributable packages in Windows SDK.

C:\Program Files (x86)\Windows Kits\8.1\Debuggers\x{86,64}\dbghelp.dll

Only dbghelp.dll from Windows directory shouldn't be redistributed.

But we would have to get dbghelp.dll from Windows 7 SDK, http://go.microsoft.com/fwlink/p/?linkid=84137 See section "Standalone Debugging Tools for debugging Windows XP and Windows Vista".

Last edited 5 years ago by kasper93 (previous) (diff)

comment:11 Changed 5 years ago by xhmikosr

Personally, I'm mostly interested about the way we're going to deploy this. Pre-Vista is something I don't think we should really care at this point.

Also, not sure what we should do with ZIP/7z packages so we need to make sure things work regardless.

comment:12 Changed 5 years ago by Underground78

Well in this case it was XP so it's not so important but who knows how things will go in the future. Although it will probably work in most cases, we are blindly using a function from a DLL we are basically told by MS not to use. It still doesn't seem like a very good idea to me.

@kasper93: I checked that but here the version I have in this folder is not the redistribuable one apparently (the version info don't match)...

On a side note, I have looked at bit more into the DrDump thingy. It sure does look like a nice tool but there are some things that would need more investigations:

  • integration with Trac
  • possibility to at least ask the user for his email and extra info when sending the dump
  • need for a symbol server.

If you guys think it's forth more investigations, I will contact them to get more info. The thing I'm afraid is that it's better suited for programs that repetitively crash at the same places because users that get an identified crash will be notified with more info. However it seems that we often have crash affecting a low number of people and in this case it would be particularly useful that we can contact the first (and possibly unique) reporter.

comment:13 Changed 5 years ago by xhmikosr

I agree we should make sure we check things before calling it.

1) I don't think we need to integrate it with Trac.
2) Not sure about user info.
3) As a symbol server we can use our server can't we?

comment:14 Changed 5 years ago by Underground78

I agree we should make sure we check things before calling it.

The problem is that we can't really check things. MS doesn't provide the changelog for the DLL bundled with Windows, only for the redistributable one. So we cannot even check the version and decide what to do based on that. :/

About DrDump my main concern is for rare crash that would require a sample file. If we cannot ask the user for his email (keeping that optional of course), the only way to get more info is to hope that the crash happens again so that an user can be requested for more info.

comment:15 Changed 5 years ago by kasper93

I checked that but here the version I have in this folder is not the redistribuable one apparently (the version info don't match)...

I can't speak for Microsoft, but it looks like they haven't updated "DbgHelp Versions" page for quite some time. Note that last listed version is 5 year old, before that new versions were shipped more often. I will try to find some info on this. It looks almost like they abandon redist dbghelp. Maybe it is not required anymore. Maybe they unified it. I don't know (yet)...

About DrDump my main concern is for rare crash that would require a sample file. If we cannot ask the user for his email (keeping that optional of course), the only way to get more info is to hope that the crash happens again so that an user can be requested for more info.

I have exactly the same concerns. This platform might be great to detect most obvious (trivial) crashes. We develop media player and it works with various external files, devices. I think it will be hard for us to fix not obvious crashes without user help. And most importantly validate the fix. If we would have support for such platforms and dumps would be uploaded automatically affected user wouldn't even know where how to contact us about the crash. Currently we explain what has happened and what user should do to help us fix it.

comment:16 Changed 5 years ago by kasper93

Although it will probably work in most cases, we are blindly using a function from a DLL we are basically told by MS not to use.

Where you read this? I cannot find this statement.

One thing in our crash handler is missing though. We should report error also to Microsoft. Being that we catch many driver and DShow internal crashes we can't hide them from MS.

If you implement a custom routine for unhandled exceptions, you are strongly urged to use the ReportFault function in the exception handler to also send an automated minidump to WER. The ReportFault function handles all of the issues of connecting to and sending the minidump to WER.

Ref: https://msdn.microsoft.com/en-us/library/windows/desktop/ee416349.aspx

Last edited 5 years ago by kasper93 (previous) (diff)

comment:17 Changed 5 years ago by Underground78

Well I cannot find it again and basically stuff I read at one place contradict what I've read just before at another place... As far as I can tell they changed the redistribution stuff so probably they unified things. I can only guess that though.

About sending stuff to Windows, DrDump seems able to manage that automatically. I think I will contact them to see if we can ask the user for extra info before sending the report. If it's possible I think we can have pretty much everything we need.

comment:18 Changed 5 years ago by kasper93

As far as I can tell they changed the redistribution stuff so probably they unified things. I can only guess that though.

My guess would be the same.

About sending stuff to Windows, DrDump seems able to manage that automatically.

Ok. In the mean time I made a patch for our current crash handler, it is really just a call to this function. Also translate error code to string. And fix this 0 error code. This probably was due to CloseHandle() call clearing error code, correct?

I think I will contact them to see if we can ask the user for extra info before sending the report. If it's possible I think we can have pretty much everything we need.

Ok, keep us posted. DrDump is not critical for our needs, but it probably would be nice addition. I wonder if we find many new crashes.

comment:19 Changed 5 years ago by kasper93

Cc: kasper93 added

comment:20 Changed 5 years ago by Underground78

This probably was due to CloseHandle() call clearing error code, correct?

Yes, it's what I've fixed locally.

Ok, keep us posted.

I have sent the message a few hours ago, we will see if we get an answer. I wonder if they would be willing to accommodate for our needs in exchange for advertising that we are using their solution. Even if we don't get more dumps, it would be easier for our users. Also it is quite nice in case we have a nasty regression like we had sometimes. Once the bug is fixed, DrDump can show a message for the users that are still getting this issue (for example to suggest updating).

comment:21 Changed 5 years ago by Underground78

I got some answers from the DrDump guy:

  • It's possible to add extra info to the report just before it's sent and/or to redirect the user to a custom page.
  • He is ok with us uploading the binaries and PDB for all public releases, including the nightlies.
  • A full dump is always requested the first time a bug is encountered.

I've done some preliminary testing and the integration really is straightforward. I was able to have MPC-HC sending some reports really quickly. Few things I've noticed so far:

  • It seems a full dump is always requested even when one was already uploaded. I have reported that since I think it's a bug.
  • We will probably have to use custom parameters for full dump creation because by default they are really huge (even compressed) and uploading them takes a long time. Users might not like that.
  • Minidumps are really as basic as possible (just the stack) since they are used to detect whether the crash was encountered before so they are not enough for any real debugging. This is why the above is really important. The best would be to have an intermediate class of dump and only require real full dump when needed but as far as I know it's not possible (yet).
  • Apparently DrDump requires to have dbghelp.dll in the same folder than the executable. The full package is 600KB when compressed using LZMA2 ultra, 250KB is we can skip dbghelp.dll.
  • If the main dll (crashrpt.dll) is available, the crash reporter is automatically and silently disabled. I think that if we go with the DrDump solution, we should only add the include in the repo and use build.user.bat to define the path to the actual SDK. This way we avoid having binaries in our repo and having people redistributing custom builds with the crash reporter.

@kasper93: The DrDump guy thinks we would get plenty of new crash reports being that reporting would be much easier. If we choose to go with his solution, we will see if that was marketing only. :p

Anyway, tell me if you think we should go down that road or not.

comment:22 Changed 5 years ago by kasper93

It seems a full dump is always requested even when one was already uploaded. I have reported that since I think it's a bug.

We need to have it sorted, because depending on MPC-HC state dumps can get huge like you noticed in next point. And no matter how we tweak that it will still be big. I bet 2/3 people would just close the upload if random crash happens.

OT: I like how madshi done this in his madExcept (which he uses for madVR). Very detailed crash report is generated and send to the developer. He even create sceenshot when the crash occured. User is asked for decription and contact information.

Apparently DrDump requires to have dbghelp.dll in the same folder than the executable. The full package is 600KB when compressed using LZMA2 ultra, 250KB is we can skip dbghelp.dll.

Fell free to send me an email if you need help integrating and/or testing this.
I personally don't mind shipping the dll, but my concern is that we don't know which version we need. I mean 6.12 which is 5-years old? Sure it works, but does it really the one to work on Windows 8 or 10? Were there any internal changes that are significant? We don't know that. This is probably not the big issue, but I'm bit concerned here.

If the main dll (crashrpt.dll) is available, the crash reporter is automatically and silently disabled. I think that if we go with the DrDump solution, we should only add the include in the repo and use build.user.bat to define the path to the actual SDK. This way we avoid having binaries in our repo and having people redistributing custom builds with the crash reporter.

I agree. Only official packages should have reporting enabled.

The DrDump guy thinks we would get plenty of new crash reports being that reporting would be much easier. If we choose to go with his solution, we will see if that was marketing only. :p

You know. It is not like we don't have crashes. It is more like they are mostly out of our code. But this will definitely help to lower time to detect bug. Right now we need to wait for user to report issue, with automatic upload we probably will be able to detect crashes quicker. Which is always a good thing.

Anyway, tell me if you think we should go down that road or not.

I think we can try it. I don't know about DrDump reliability, but it looks ok. Definitely easier way than developing our own custom solution.

EDIT:

I would use latest dbghelp.dll from SDK.

Last edited 5 years ago by kasper93 (previous) (diff)

comment:23 Changed 5 years ago by Underground78

We need to have it sorted, because depending on MPC-HC state dumps can get huge like you noticed in next point. And no matter how we tweak that it will still be big. I bet 2/3 people would just close the upload if random crash happens.

In fact there is an heuristic to decide whether a full dump should be uploaded. Basically it will try to gather a few full dumps with the same stack trace for easier debugging. Currently it will try to gather 10 but we can ask for this value to be lowered.

I have also been experimenting with the dump options. We clearly cannot use real full dumps because they would be really huge even compressed (like more than 150MB). However with the compression and the automated process, I think it's safe enough to add some options to our current default dump:

  • MiniDumpWithIndirectlyReferencedMemory to get some possibly interesting chunks of memory
  • MiniDumpWithDataSegs to have the content of the global variables.

On my test case the dump is less than 1MB when compressed.

OT: I like how madshi done this in his madExcept (which he uses for madVR). Very detailed crash report is generated and send to the developer. He even create sceenshot when the crash occured. User is asked for decription and contact information.

We are limited in size for extra info but the description and contact info should fit without problem as far as I understand. If we have those, we can create a ticket and contact the users so extra infos.

I would use latest dbghelp.dll from SDK.

As far as I know, the dbghelp.dll distributed by DrDump is the latest DLL from the debugging tools so the latest official redistributable and with an official changelog. I think it should be safe enough to use it.

comment:24 Changed 5 years ago by kasper93

I think it's safe enough to add some options to our current default dump:

Ok. We can tweak options over time once we get dumps we will know what information is missing.

We are limited in size for extra info but the description and contact info should fit without problem as far as I understand. If we have those, we can create a ticket and contact the users so extra infos.

It should be fine. We can even ask users to create ticket when crash happens. We will get auto uploaded dumps and possibly user reports if they want to help.

As far as I know, the dbghelp.dll distributed by DrDump is the latest DLL from the debugging tools so the latest official redistributable and with an official changelog. I think it should be safe enough to use it.

And now I'm lost. Because the dbghelp.dll form debugging tools doesn't match the version form official changelog. But let's leave it for now it might be not that important as long as it work.

comment:25 Changed 5 years ago by Underground78

I need some options about some things:

  1. Do you think we should keep our own minidump code? I don't really think we should to be honest.
  2. Should we have a command line switch to inhibit the crash reporter? Note that removing/renaming the "CrashReporter" (or whatever name we choose) sub-folder is enough to disable it.

comment:26 Changed 5 years ago by Underground78 <underground78@…

Owner: set to Underground78 <underground78@…>
Resolution: fixed
Status: evaluatedclosed

In cdbdc9:

Replace our custom minidump generation code by Doctor Dump.

Doctor Dump is a more advanced crash reporter with crash report uploading and a server-side application to sort and keep track of them.

See ticket #5336 also fixes #5380.

comment:27 Changed 5 years ago by Underground78

Milestone: next release

The new crash reporter should be more resilient than the old method which should help for cases like the one you reported. It will be in the nightly builds as of version 1.7.8.183.

comment:28 Changed 5 years ago by xhmikosr

Owner: changed from Underground78 <underground78@… to Underground78

comment:29 Changed 5 years ago by thevbm

Milestone: next release1.7.9

Milestone renamed

Note: See TracTickets for help on using tickets.