Opened 9 years ago

Closed 8 years ago

#2105 closed enhancement (implemented)

Pick up the breakpad integration

Reported by: arma Owned by: chiiph
Priority: High Milestone: Vidalia: 0.3.x
Component: Archived/Vidalia Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Matt was working on integrating
http://code.google.com/p/google-breakpad/
into Vidalia, so if it crashes on Windows we have a chance of finding a useful stack trace.

Matt, where did you get on this?

Once Matt gives us the current status, we should build a plan for getting this one moving forward again.

Child Tickets

Change History (20)

comment:1 Changed 9 years ago by cypherpunks

Why would Tor want to do this integration with microsoft? It's a privacy nightmare. Read more at http://en.wikipedia.org/wiki/Windows_Error_Reporting and possibly legally imcompatible with free software, such as Tor. See http://www.microsoft.com/whdc/winlogo/maintain/StartWER.mspx and specifically https://winqual.microsoft.com/help/developers_guide_to_wer.htm

I think you're better off not integrating with Microsoft error reporting and make vidalia start with a master process that spawns sub-processes, where the error reporting is in the master so you get better errors per windows.

comment:2 in reply to:  1 Changed 9 years ago by arma

Replying to cypherpunks:

Why would Tor want to do this integration with microsoft? It's a privacy nightmare. Read more at http://en.wikipedia.org/wiki/Windows_Error_Reporting and possibly legally imcompatible with free software, such as Tor. See http://www.microsoft.com/whdc/winlogo/maintain/StartWER.mspx and specifically https://winqual.microsoft.com/help/developers_guide_to_wer.htm

I think you're better off not integrating with Microsoft error reporting and make vidalia start with a master process that spawns sub-processes, where the error reporting is in the master so you get better errors per windows.

What does breakpad have to do with Microsoft? I think you are deeply confused.

comment:3 Changed 9 years ago by arma

Here is the answer from Matt:

<edmanm:#vidalia> arma: https://svn.vidalia-project.net/svn/vidalia/trunk/README.breakpad
<edmanm:#vidalia> it worked. someone needs to set up a place for crash dumps to go again. probably see if changes to breakpad in the last year need vidalia-side changes.

comment:4 Changed 8 years ago by chiiph

I think this would be nice for the 0.3.0-alpha release.

comment:5 Changed 8 years ago by chiiph

Milestone: Vidalia-0.3.X
Owner: changed from edmanm to chiiph
Status: newassigned

comment:6 Changed 8 years ago by chiiph

I will be working at this on my branch bug2105_breakpad.
Right now there's a working version there, but it needs more love. There are a couple of ugly things that I did just to see if it worked, so look at it with a nice eye :), but the good thing is that it should work on every platform right now.

I'm thinking that this shouldn't just be in 0.3.x-alpha, it's a good debugging tool, so it should be in both.

comment:7 Changed 8 years ago by edmanm

So a couple of comments:

1) The fact that I did not use Qt API methods for stuff in the exception handler is 'absolutely' intentional. When the minidump callback is called, the application is probably in a bad state and so you want to do as little as necessary in the handler. Calling out to complex, unnecessary libraries, such as Qt, is not a good idea.

2) You broke crash dump support on Windows systems using wide chars (unless something has significantly changed in recent versions of Breakpad).

3) I don't think this does what you think it does:

-  target_link_libraries(${vidalia_BIN} ${BREAKPAD_LIBRARIES})
+  target_link_libraries(${vidalia_BIN} ${BREAKPAD_LIBRARY_DIR})

You're trying to link a binary to a directory, rather than the actual libraries (unless you define BREAKPAD_LIBRARY_DIR to a specific file rather than a directory to search for the correct files, but that's nonsense). See FindBreakpad.cmake.

4) I don't understand this:

+  _configDialog = 0;

comment:8 in reply to:  7 ; Changed 8 years ago by chiiph

Replying to edmanm:

So a couple of comments:

First of all, remember, that code is just me testing breakpad and such.

1) The fact that I did not use Qt API methods for stuff in the exception handler is 'absolutely' intentional. When the minidump callback is called, the application is probably in a bad state and so you want to do as little as necessary in the handler. Calling out to complex, unnecessary libraries, such as Qt, is not a good idea.

Ok, I'll dig into this issue. It was just the quickest way to make it Linux friendly and test a couple of things.
Given the current state of code protection I'd bet that Qt's code is still in good shape in any platform.

2) You broke crash dump support on Windows systems using wide chars (unless something has significantly changed in recent versions of Breakpad).

I was just testing this on Linux, again, this is *not* finished at all.

3) I don't think this does what you think it does:

-  target_link_libraries(${vidalia_BIN} ${BREAKPAD_LIBRARIES})
+  target_link_libraries(${vidalia_BIN} ${BREAKPAD_LIBRARY_DIR})

You're trying to link a binary to a directory, rather than the actual libraries (unless you define BREAKPAD_LIBRARY_DIR to a specific file rather than a directory to search for the correct files, but that's nonsense). See FindBreakpad.cmake.

Yes, it does exactly what I want in the particular way I was testing it, I was using BREAKPAD_LIBRARY_DIR to point to the .a file compiled in the google-breakpad dir, The find cmake macro didn't worked in my setup, so I got tired of it and did it "by hand".

4) I don't understand this:

+  _configDialog = 0;

This is just to make it crash and see if it works as expected.

Again, this wasn't for review. This was just to say "hey, I'm looking at this, this code is wrong/ugly in several ways, but it's the branch in which I'll be working on this" :)
When I think it's ready, I'll update the ticket as needs_review.

comment:9 in reply to:  8 Changed 8 years ago by edmanm

Replying to chiiph:

Again, this wasn't for review. This was just to say "hey, I'm looking at this, this code is wrong/ugly in several ways, but it's the branch in which I'll be working on this" :)
When I think it's ready, I'll update the ticket as needs_review.

OK, sorry. But when you make statements like "there's a working version there" and "it should work on every platform now", yet clearly made some parts worse and broke others, I'm going to call you out on it. ;)

comment:10 Changed 8 years ago by chiiph

Sure, but I count with Qt working after the crash. I just haven't test it a lot yet. :)

comment:11 Changed 8 years ago by edmanm

You won't be able to test every possible crash scenario. What if the crash occurs in Qt? If your crash handler depends on Qt, you might be out of luck. There is no good reason for calling out to the Qt library from within the crash handler when it's so easy to do what little needs to be done in a crash handler without using Qt as a crutch.

comment:12 Changed 8 years ago by chiiph

With that reasoning, I can argue: what if the crash occurs _in_ the crash handler?

I understand what you mean, but arguing with myself too, because I also think I rather have a crash handler for all Vidalia crashes and none of the Qt crashes, than having no crash handler in any platform, of having to debug string and file operations done "by hand" for buffer overflows and such.

Anyway, this is kind of pointless since both of us have made pretty clear what each think. I'll process this, and we'll discuss it when I come with a solution that I'd like to stick with. What's done right now, was just me getting to know breakpad.

Also, I think that breakpad itself has operations to handle dump writing, so that should be great.

comment:13 in reply to:  12 Changed 8 years ago by edmanm

Replying to chiiph:

With that reasoning, I can argue: what if the crash occurs _in_ the crash handler?

Yes! That's exactly my point! :-) That's exactly why you want to do as little as possible in the callback. From the Breakpad design docs:

  • Use of the application heap is forbidden. The heap may be corrupt or otherwise unusable, and allocators may not function.
  • Resource allocation must be severely limited. The handler may create a new file to contain the dump, and it may attempt to launch a process to continue handling the crash.
  • Execution on the thread that caused the exception is significantly limited. The only code permitted to execute on this thread is the code necessary to transition handling to a dedicated preallocated handler thread, and the code to return from the exception handler.
  • Handlers shouldn’t handle crashes by attempting to walk stacks themselves, as stacks may be in inconsistent states. Dump generation should be performed by interfacing with the operating system’s memory manager and code module manager.
  • Library code, including runtime library code, must be avoided unless it provably meets the above guidelines. For example, this means that the STL string class may not be used, because it performs operations that attempt to allocate and use heap memory. It also means that many C runtime functions must be avoided, particularly on Windows, because of heap operations that they may perform.

Also:

Authors of both callback functions that execute within a Breakpad handler are cautioned that their code will be run at exception time, and that as a result, they should observe the same programming practices that the Breakpad handler itself adheres to. Notably, if a callback is to be used to collect additional data from an application, it should take care to read only “safe” data. This might involve accessing only static memory locations that are updated periodically during the course of normal program execution.

It doesn't take much looking at Qt code to see that the changes you've made don't conform to these guidelines. And for what? To save writing a few lines of code? I could've done that too and saved myself some time, but I didn't because it was the lazy, incorrect way to do things.

I understand what you mean, but arguing with myself too, because I also think I rather have a crash handler for all Vidalia crashes and none of the Qt crashes, than having no crash handler in any platform, of having to debug string and file operations done "by hand" for buffer overflows and such.

I don't think there's an either/or situation here. Plus, the crash handling worked fine on Windows so the statement that there was "no crash handler in any platform" is wrong. What was really missing and preventing crash dumps from being enabled by default is that someone needs to set up a place for the crash dumps to be uploaded, processed and displayed. I had set up a Socorro instance awhile ago, but man it was a pain to setup and maintain. There also needs to be an easy way for packagers to generate and upload symbol files for releases. And, yes, the crash handler callback needs to be expanded to work on more platforms than Windows. But writing to a file and launching a process are CS101 things that you can do in a safe way without relying on Qt and the heap operations it brings in. I even left some #ifdef statements in there indicating where such code for non-Windowses would go.

Also, I think that breakpad itself has operations to handle dump writing, so that should be great.

Yep, it already writes the dump itself (thankfully, since that's a decidedly non-trivial task).

comment:14 Changed 8 years ago by chiiph

You are right. Thanks for the ping-pong :)

I'll update the ticket when I have a non-idiotic implementation.

comment:15 Changed 8 years ago by chiiph

Status: assignedneeds_review

Here's a saner way of handling things. It's POSIX compliant, so it should work in OSX too (I've only tested this on Linux):
https://gitweb.torproject.org/chiiph/vidalia.git/commitdiff/75d7d200f799919ce144d7618ab646377dbd433a

Since breakpad on linux is build statically, the idea is to specify the include dir the same as before, but specify the actual lib with -DBREAKPAD_LIBRARY=/path/to/lib.a, I wasn't able to make the find method work, and for what I read with static libs it doesn't work (the general find_library method in cmake), but I might be wrong.

Regarding Socorro, or whatever medium to upload: May be it's simpler if we just change the message displayed to say something like: "Vidalia has just crashed blablabla. If you think this is a problem with Vidalia, you should fill a ticket at trac.torproject.org attaching the 192830123.dmp and 192830123.dmp.info files located at /path/to/crash/dir". And in the mean time we inform users of where we are handling problems in general (not everybody knows about this trac). Something like Socorro seems way too much for the moment.

comment:16 in reply to:  15 ; Changed 8 years ago by edmanm

Replying to chiiph:

Here's a saner way of handling things. It's POSIX compliant, so it should work in OSX too (I've only tested this on Linux):
https://gitweb.torproject.org/chiiph/vidalia.git/commitdiff/75d7d200f799919ce144d7618ab646377dbd433a

Looks like a much better approach to me.

Since breakpad on linux is build statically, the idea is to specify the include dir the same as before, but specify the actual lib with -DBREAKPAD_LIBRARY=/path/to/lib.a, I wasn't able to make the find method work, and for what I read with static libs it doesn't work (the general find_library method in cmake), but I might be wrong.

I never had trouble with the find method working with static libraries, but specifying the necessary library (since there's only one we actually need anymore) seems like a fine requirement.

Regarding Socorro, or whatever medium to upload: May be it's simpler if we just change the message displayed to say something like: "Vidalia has just crashed blablabla. If you think this is a problem with Vidalia, you should fill a ticket at trac.torproject.org attaching the 192830123.dmp and 192830123.dmp.info files located at /path/to/crash/dir". And in the mean time we inform users of where we are handling problems in general (not everybody knows about this trac). Something like Socorro seems way too much for the moment.

Meh, it doesn't matter much to me either way. Socorro was a pain to set up, but not impossible (it may have gotten better since the last time I did it). Not having a way for users to automatically upload crash dumps is a bit of a cop out, but telling users to create a trac ticket and manually upload the files is better than nothing. If you're going to do that, you should make sure the symbol files for each release are publicly available.

comment:17 in reply to:  16 ; Changed 8 years ago by chiiph

Replying to edmanm:

Replying to chiiph:
Meh, it doesn't matter much to me either way. Socorro was a pain to set up, but not impossible (it may have gotten better since the last time I did it). Not having a way for users to automatically upload crash dumps is a bit of a cop out, but telling users to create a trac ticket and manually upload the files is better than nothing. If you're going to do that, you should make sure the symbol files for each release are publicly available.

I don't mind keeping that public, but I don't understand why they need to be. In case another user wants to take a look at the crash?

Anyway, thanks a lot for the feedback, I'll add this to stable and alpha.

comment:18 in reply to:  17 Changed 8 years ago by rransom

Replying to chiiph:

Replying to edmanm:

If you're going to do that, you should make sure the symbol files for each release are publicly available.

I don't mind keeping that public, but I don't understand why they need to be. In case another user wants to take a look at the crash?

That is one reason that symbol files must be publicly available -- some users will want to inspect their own crash dumps, rather than sending a mystery blob that might contain sensitive information off to a group of people they don't know. But also keep in mind that we will need access to symbol files for several versions of Vidalia, not just the most recently released versions, and we shouldn't rely on an ‘archive’ of symbol files that isn't publicly available continuing to exist.

comment:19 Changed 8 years ago by chiiph

In my branch chiiph/bug2105_breakpad you can find the latest changes to the CrashReporter app, suggesting the user to upload the dump files and a description to a ticket in here.

comment:20 Changed 8 years ago by chiiph

Resolution: implemented
Status: needs_reviewclosed

This has been merged both to master and alpha branches.

This will be available first in 0.3.0-alpha, and later on in 0.2.13.

Note: See TracTickets for help on using tickets.