Improve filtering for false positive crashes...

Improve filtering for false positive crashes

...by inspecting if the module with the crash has the topmost handler
in the SEH chain.  If that's the case then the exception will very
likely be handled. This covers the case of IsBadReadPtr adn family
without any special code. It also lets us address IsValidInterface,
an internal helper in urlmon.

BUG=64348
TEST=covered by existing unit tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69066

[tommi (sloooow) - chröme, 2010-12-09 01:38:00]

lgtm! very nice

[stoyan, 2010-12-09 09:58:22]

Despite my comments, you can go ahead and submit.

http://codereview.chromium.org/5622006/diff/1/chrome_frame/crash_reporting/ve...
File chrome_frame/crash_reporting/vectored_handler-impl.h (right):

http://codereview.chromium.org/5622006/diff/1/chrome_frame/crash_reporting/ve...
chrome_frame/crash_reporting/vectored_handler-impl.h:173: static bool
ShouldIgnoreException(const EXCEPTION_POINTERS* exceptionInfo,
Hm.. if a module had installed SEH filter it does not mean it will handle
everything in it.

http://codereview.chromium.org/5622006/diff/1/chrome_frame/crash_reporting/ve...
chrome_frame/crash_reporting/vectored_handler-impl.h:179:
::GetModuleHandleEx(flags, reinterpret_cast<LPCWSTR>(address),
Beware - IIRC GetModuleHandle obtains loader lock.

http://codereview.chromium.org/5622006/diff/1/chrome_frame/crash_reporting/ve...
chrome_frame/crash_reporting/vectored_handler-impl.h:183:
::GetModuleHandleEx(flags, reinterpret_cast<LPCWSTR>(seh_record->Handler),
if crashing_module == NULL you don't have to call this, because of the check in
the next line.
I suspect GetModuleHandle installs SEH too, because the address passed can be
bogus i.e. seh_record->Handler could be anything; it may create some interesting
cases.

[amit, 2010-12-09 17:58:59]

Thanks Stoyan for raising good points. Please take another look.

http://codereview.chromium.org/5622006/diff/1/chrome_frame/crash_reporting/ve...
File chrome_frame/crash_reporting/vectored_handler-impl.h (right):

http://codereview.chromium.org/5622006/diff/1/chrome_frame/crash_reporting/ve...
chrome_frame/crash_reporting/vectored_handler-impl.h:173: static bool
ShouldIgnoreException(const EXCEPTION_POINTERS* exceptionInfo,
On 2010/12/09 09:58:23, stoyan wrote:
> Hm.. if a module had installed SEH filter it does not mean it will handle
> everything in it.

True, there's a chance that the module's SEH filter will not handle the
exception. I guess it boils down to the following cases:
1. Crashing module is npchrome_frame.dll. If we have SEH installed, we will do
the right thing in the handler
2. Crashing module is x.dll and npchrome_frame.dll is not on stack. It's is very
unlikely that this is our fault.
3. Crashing module is x.dll and npchrome_frame.dll is on the stack. If x.dll is
has the top SEH filter, then it follows the expected exception pattern (very
often like urlmon!IsValidInterface). From the crashes, I haven't seen anything
indicating npchrome_frame.dll is at fault in this kind of scenario. Also, in
areas where we expect to have issues, we have SEH filters installed. 

So my sense is this fix will allow us to suppress a category of false positives
without too much risk of hiding true crashes.

http://codereview.chromium.org/5622006/diff/1/chrome_frame/crash_reporting/ve...
chrome_frame/crash_reporting/vectored_handler-impl.h:179:
::GetModuleHandleEx(flags, reinterpret_cast<LPCWSTR>(address),
On 2010/12/09 09:58:23, stoyan wrote:
> Beware - IIRC GetModuleHandle obtains loader lock.

Good point, changed it to use VirtualQuery

http://codereview.chromium.org/5622006/diff/1/chrome_frame/crash_reporting/ve...
chrome_frame/crash_reporting/vectored_handler-impl.h:183:
::GetModuleHandleEx(flags, reinterpret_cast<LPCWSTR>(seh_record->Handler),
On 2010/12/09 09:58:23, stoyan wrote:
> if crashing_module == NULL you don't have to call this, because of the check
in
> the next line.
> I suspect GetModuleHandle installs SEH too, because the address passed can be
> bogus i.e. seh_record->Handler could be anything; it may create some
interesting
> cases.

Yupp, added the check for crashing_module and changed to VirtualQuery