Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue: Invalid ZIP file crashes NBug #28

Closed
shaxxx opened this issue Nov 15, 2013 · 10 comments
Closed

Issue: Invalid ZIP file crashes NBug #28

shaxxx opened this issue Nov 15, 2013 · 10 comments

Comments

@shaxxx
Copy link
Contributor

shaxxx commented Nov 15, 2013

When exception is thrown, after UI dialog is displayed NBug creates ZIP file in specified location. If internal exception occurs in NBug, it will leave invalid ZIP file on disk.
On every subsequent application start NBug will try to load invalid ZIP file, generate IO Exception, crash and warn user about it's internal exception. Note that empty ZIP file is handled just fine.

Proposed solution:

Create ZIP file at temporary location (or memory) and move it to the right location once when serialization is done or ensure that ZIP archive is always valid even if NBug crashes.

soygul added a commit that referenced this issue Nov 18, 2013
@soygul
Copy link
Owner

soygul commented Nov 18, 2013

Can you post the details of a sample IOException you get. If I didn't skip something, ad5501b must have resolved the issue. Can you test it?

Thanks.

@shaxxx
Copy link
Contributor Author

shaxxx commented Nov 19, 2013

Ok, I've pulled last commit and issue is not resolved.

I get System.IO.InvalidDataException thrown from ZipStorer.Open method, and NBug Internal Exception Viewer opens with Error detail. If users clicks on quit application will never start, invalid ZIP file will stay on disk, user will try to start the app again and internal exception viewer will come up again creating endless loop and making application unusable.

It's really easy to test it, take any non zip file, name it Exception_*.zip, and put it in the folder with the app.

You were close with the last commit, but we need to actually delete invalid zip file first because after Logger.Error DeleteCurrentReportFile is never called. Just change order of the lines 88 and 89 in Dispatcher.cs

                        Logger.Error("An exception occurred while extraction report data from zip file. Check the inner exception for details.", exception);
                        storer.DeleteCurrentReportFile();

to

                        storer.DeleteCurrentReportFile();
                        Logger.Error("An exception occurred while extraction report data from zip file. Check the inner exception for details.", exception);

This way Internal Exception Viewer is displayed only once and there is no loop (question is should it be displayed at all since we know what kind of exception occurred, why it happened, and we're pretty sure it's not in fact NBug Internal Exception).

I've noticed earlier that Logger.Error not only logs, but also throws error which IMHO is a really not transparent and should be re factored in the future.

@shaxxx
Copy link
Contributor Author

shaxxx commented Nov 19, 2013

I was also thinking about decorating all Logger methods with DebuggerStepThroughAttribute since it would make debugging so much easier. I can do it, just didn't want to on my own.

soygul added a commit that referenced this issue Nov 19, 2013
@soygul
Copy link
Owner

soygul commented Nov 19, 2013

Logger.Error throws error only if release mode is not set to true, though it is best to prevent the loop in debug mode too so I committed your fix.

@soygul
Copy link
Owner

soygul commented Nov 19, 2013

You'll notice that Settings.ThrowExceptions will cause NBug to throw or swallow internal exceptions. Settings.HandleExceptions will cause it to handle unhandled exceptions or swallow them. So if Settings.ThrowExceptions = false, Logger.Error will never throw exceptions (same goes for release mode = true). Have a look in here: https://github.com/soygul/NBug/blob/master/NBug/Settings.cs#L275

If you set Settings.HandleExceptions = false, logger methods will never be invoked so they will never appear in your stack trace.

@soygul soygul closed this as completed Nov 19, 2013
@shaxxx
Copy link
Contributor Author

shaxxx commented Nov 19, 2013

Thanks for the fix.
I know that I can set NBug to swallow exceptions, but default settings are fine, and the way it works in internal exception handling is good, I was just referring to separation of concerns, the fact that something called logger should do just that, simply log the errors.

Once again, thanks for the help & support.
Regards.

@soygul
Copy link
Owner

soygul commented Nov 19, 2013

If you have the time to decorate the logger functions and test to see there are no side affects, please do so:) I'll be off for the week and might forget this completely..

@pmarienfeld
Copy link

Hi Teoman

I recently stumbled upon the same issue with an invalid exception zip file. Furthermore subsequent valid exception zip files are not being sent anymore once an invalid file is stored in the path. Would you mind releasing a new version (1.2.3) along with this fix to the nuget gallery. Thanks.

Best regards
Philip

@soygul
Copy link
Owner

soygul commented Aug 10, 2014

I don't have Windows & Visual Studio at the moment so I can't do that unfortunately. You'll have to compile and use it yourself.

@pmarienfeld
Copy link

Ok. So I'll go with a local build then until you're back on the .NET track. Thanks for your quick response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants