587406 - Exceptions on windows 64 are not fatal

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Joe Drew (not getting mail)]

Basically all exceptions on WOW64 (Windows 32 bit applications running on Windows 64) are non-fatal. This means that when we try to fire a _debugbreak() in our OOM handler, instead of breaking, WOW64 eats the exception and returns us to the event loop. This means that when we dereference a null pointer, Windows 64 returns us to the event loop. We are left in an utterly invalid state, unable to draw or progress any further, left spinning in the event loop in a futile attempt to get any work done.

This is bad.

Comment 1

[Joe Drew (not getting mail)]

This, I believe, only happens when we call from our code back into Windows code, back into our code. 

Mozilla(1) -> Windows -> Mozilla(2)

When Mozilla(2) throws an exception, for example by running out of memory and intentionally crashing, Windows catches it and silently unwinds back out, leaving us (most often) inside the event loop.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This has been bothering me for a long time, and it now makes more sense that WOW64 is involved.  http://www.nynaeve.net/?p=202 might be useful (mention of KiUserApcDispatcher providing a SEH frame, and that dispatcher being involved in kernel->user dispatch).  Also another reference mentions that vectored exception handling overwrites structured, so that might also be related.

It's probably worth trying to wrap our main() in a SEH handler and see what happens.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I should say -- I don't know that this needs to block beta4.  It's been happening for a while now, maybe even on previous versions of firefox, and is only becoming an issue because more and more people are running 64-bit versions of windows.  But it should block beta5.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Agreed with comment 3

Comment 6

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #3)
> I should say -- I don't know that this needs to block beta4.  It's been
> happening for a while now, maybe even on previous versions of firefox, and is
> only becoming an issue because more and more people are running 64-bit versions
> of windows.  But it should block beta5.

If you guys are planning on turning on D2D by default then this needs to be fixed soon as it is leading to quite a common issue among testers see Bug 586909 for example.

This kind of issue makes it difficult to test D2D in the short term and since, as I understand it, the whole point of enabling D2D now was to try and get some user coverage this is going to hurt that.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

I can't reproduce this with CrashMe on Win64

Comment 8

[Jeff Muizelaar [:jrmuizel]]

I wonder if this only happens on crashes that happen during paint

Comment 9

[(not currently active) Ted Mielczarek]

Comment 1 implies that it only happens when we're being called back by Windows, so inside a WM_PAINT seems likely. (CrashMe is just triggered off a menu item, which is serviced via the event loop, but probably not via a Windows message).

Comment 10

[(no longer active)]

This behavior is documented in MSDN:

http://msdn.microsoft.com/en-us/library/ms633573%28VS.85%29.aspx

It appears to me that we need to hook up our crash reporter with our window proc(s), and just invoke it and terminate the process in case of a fatal exception.

Comment 11

[(not currently active) Ted Mielczarek]

We use SEH around a11y calls currently:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessNodeWrap.cpp#181
http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessNodeWrap.cpp#584

Comment 12

[Jeff Muizelaar [:jrmuizel]]

I think this should probably block enabling D2D by default, otherwise we stand to miss out on bunching of crash feedback and giving people a bad experience at the same time.

Comment 13

[(no longer active)]

(In reply to comment #11)
> We use SEH around a11y calls currently:
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessNodeWrap.cpp#181
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessNodeWrap.cpp#584

This seems exactly like what we need to be doing here:

http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessNodeWrap.cpp#584

Except that we should not return EXCEPTION_CONTINUE_SEARCH I think.  I think the right thing to do here is to call TerminateProcess instead (provided that the crash reporter can handle that!)

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That's pretty ridiculous.

But, we need to test -- if we add an exception handler and then TerminateProcess, we'd still like to know what the actual crash was.  Can we somehow extend breakpad to pass it to the exception structure?  Otherwise, this is just going to be a black hole "something crashed in the WndProc" type of report, since we won't have a stack or anything.

Comment 15

[Benjamin Smedberg]

Attachment: Report exceptions from nsWindow::WindowProc, rev. 1

Comment 16

[(not currently active) Ted Mielczarek]

vlad: the examples I linked to (and bsmedberg's patch) call nsICrashReporter::WriteMinidumpForException, which takes an EXCEPTION_POINTERS* argument, which we get from the SEH catch block, so crash reports will show up with the crashing thread stack shown from wherever the original crash was.

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 466370 [details] [diff] [review]
Report exceptions from nsWindow::WindowProc, rev. 1

Looks sensible. If we could figure out a reliable way to test this (even as a Litmus test) that would be awesome.

Comment 18

[(no longer active)]

Comment on attachment 466370 [details] [diff] [review]
Report exceptions from nsWindow::WindowProc, rev. 1

This doesn't seem quite like the right approach to me.  Why are we converting all of the possible exception codes into crashes?

Comment 19

[Benjamin Smedberg]

Isn't everything that __except catches a crash if you don't catch it? Why wouldn't we catch all of them?

Comment 20

[Jeff Muizelaar [:jrmuizel]]

This seems to work for me. At least it quickly exited firefox instead of partially starting up. I'm building with crash reporter enabled right now to make sure that works.

Comment 21

[(no longer active)]

(In reply to comment #19)
> Isn't everything that __except catches a crash if you don't catch it? Why
> wouldn't we catch all of them?

Yes.  Well, I guess we could go with this patch as well, since the MSDN docs do not seem to suggest that any kind of filtering happens by Windows.  I would assume that our crash reporting infrastructure is capable of handling different exception codes...

Comment 22

[Benjamin Smedberg]

Indeed, we do.

Comment 23

[Bas Schouten (:bas.schouten)]

(In reply to comment #19)
> Isn't everything that __except catches a crash if you don't catch it? Why
> wouldn't we catch all of them?

Right, if the SEH frames below us don't catch it I'd say we need to terminate. I'm guessing noone's planning on catching exceptions -through- the Win32 message layer. I think this fix is exactly what we need and it's roughly what I suggested to release-drivers. So I'm all for this.

Comment 24

[(no longer active)]

(In reply to comment #23)
> (In reply to comment #19)
> > Isn't everything that __except catches a crash if you don't catch it? Why
> > wouldn't we catch all of them?
> 
> Right, if the SEH frames below us don't catch it I'd say we need to terminate.
> I'm guessing noone's planning on catching exceptions -through- the Win32
> message layer. I think this fix is exactly what we need and it's roughly what I
> suggested to release-drivers. So I'm all for this.

FWIW, I don't agree with this in the general sense, it's always possible to have a global handler to handle some benign exceptions (or even ignore them), and it is possible to pass custom exception codes to RaiseException.  But in *this* case, none of this matters since Windows will be eating the exceptions up if we don't do it!

Comment 25

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/ed9357ce0939
http://hg.mozilla.org/mozilla-central/rev/a2c829756d99

Comment 26

[[e-mail]]

(In reply to comment #14)
> That's pretty ridiculous.
Agreed. That pretty much breaks just-in-time debugging too.

Comment 27

[Benjamin Smedberg]

Does the debugger catch these on Windows 64, or are they eaten? I'm happy to #ifdef this code so it doesn't run in debug builds, or checks whether a debugger is attached, if that's possible.

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It never caught it for me.  You can probably do it if you manually configure breaking on caught exceptions or something (since as far as the debugger knows, the exception was handled, it never propagated back up).

Comment 29

[(no longer active)]

(In reply to comment #27)
> Does the debugger catch these on Windows 64, or are they eaten? I'm happy to
> #ifdef this code so it doesn't run in debug builds, or checks whether a
> debugger is attached, if that's possible.

You can call IsDebuggerPresent(), and if it returns TRUE, call DebugBreak() maybe?

Comment 30

[timeless]

bustage fix:
http://hg.mozilla.org/mozilla-central/rev/48487f904abe

Comment 31

[[e-mail]]

This breaks debugging on 32-bit Windows too - I just clicked "Retry" on an assertion, expecting it to invoke the JIT debugger :-(

Comment 32

[Myzar]

Is it supposed to catch the crashes from https://bugzilla.mozilla.org/show_bug.cgi?id=586909 now ? i just crashed the usual way with mozalloc.dll with this build

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100817 Minefield/4.0b4pre Firefox/3.6.7

and breakpad didn't fire, is there some auto crashme test just to check that breakpad is functional on my box

Comment 33

[(no longer active)]

(In reply to comment #31)
> This breaks debugging on 32-bit Windows too - I just clicked "Retry" on an
> assertion, expecting it to invoke the JIT debugger :-(

Benjamin, can we only look at access violations here?

Comment 34

[Benjamin Smedberg]

No, we are not going to look at only access violations, that doesn't make sense. We can add a IsDebuggerPresent/DebugBreak combo in the handler.

Comment 35

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #34)
> No, we are not going to look at only access violations, that doesn't make
> sense. We can add a IsDebuggerPresent/DebugBreak combo in the handler.

We could also let the exception pass up through WindowProc on platforms that support it like 32bit on 32bit.

Comment 36

[Benjamin Smedberg]

Attachment: IsDebuggerPresent check, rev. 1

Comment 37

[[e-mail]]

Comment on attachment 467013 [details] [diff] [review]
IsDebuggerPresent check, rev. 1

This doesn't seem to have any effect at all.
When the debugger isn't attached, the process still terminates. (This is true even if I remove the IsDebuggerPresent check!)
When the debugger is attached, it breaks at the first-chance exception anyway.

Comment 38

[[e-mail]]

Attachment: What I'm using locally

Comment 39

[Bas Schouten (:bas.schouten)]

(In reply to comment #37)
> Comment on attachment 467013 [details] [diff] [review]
> IsDebuggerPresent check, rev. 1
> 
> This doesn't seem to have any effect at all.
> When the debugger isn't attached, the process still terminates. (This is true
> even if I remove the IsDebuggerPresent check!)
> When the debugger is attached, it breaks at the first-chance exception anyway.

Often debuggers are not set to break at first-chance exceptions. MSVC's default scheme for example only breaks at unhandled exceptions.

Comment 40

[Benjamin Smedberg]

Attachment: Use FatalExit to enter the debugger, rev. 1

Comment 41

[[e-mail]]

Comment on attachment 467119 [details] [diff] [review]
Use FatalExit to enter the debugger, rev. 1

So, given comment #39, DebugBreak is less worse than FatalExit.

Comment 42

[Masatoshi Kimura [:emk]]

What about using UnhandledExceptionFilter?
If the debugger is attached, it will return EXCEPTION_CONTINUE_SEARCH immediately.
If the debugger is not attached, it will call user-defined filter which will invoke our crash reporter.
If the debugger is not attached and the user-defined filter doesn't handle the exception, it will invoke the JIT debugger before returning EXCEPTION_CONTINUE_SEARCH.
So we can DebugBreak if the function returns EXCEPTION_CONTINUE_SEARCH.
Here's a pseudocode of UnHandledExceptionFilter.
https://www.microsoft.com/msj/0197/exception/exceptiontextfigs.htm#fig13

Comment 43

[Joe Drew (not getting mail)]

This really seems as though it hasn't been totally fixed, as people are still seeing a lot of uncrashing crashes on win64.

Also, interestingly: http://support.microsoft.com/kb/976038

Comment 44

[Joe Drew (not getting mail)]

Gavin asked that I not reopen this despite it remaining unfixed. I have filed bug 589495 on finishing the fixing of this bug.