Closed Bug 589495 Opened 9 years ago Closed 9 years ago

Win32 native exceptions (which could be crashes) on WoW64 are not fatal

Categories

(Core :: General, defect, critical)

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: joe, Assigned: ehsan)

References

(Depends on 1 open bug)

Details

(Whiteboard: [softblocker])

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #587406 +++

Gavin asked that I file a new bug rather than reopen bug 587406, which remains unfixed. 

We are receiving a lot of reports of people on Windows 64 continuing to have non-fatal crashes, and crash reports that they do get being empty and/or not saved.

See also: http://support.microsoft.com/kb/976038
Note that KB976038 is included in the Windows 7 and Windows Server 2008 R2 Service Pack 1 Beta (source: http://www.microsoft.com/downloads/details.aspx?FamilyID=61924cea-83fe-46e9-96d8-027ae59ddc11 ).

Although the documentation for the hotfix says "Independent software vendors (ISV) should not change the Image File Execution Options (IFEO) keys in their installers," the callback filter really only seems in the way. On the other hand, by the time the first service pack for Windows 7 ships (nevermind the next service pack for Windows Vista) maybe we'll have a 64-bit version of Flash ... fingers crossed.
(In reply to comment #1)
> Although the documentation for the hotfix says "Independent software vendors
> (ISV) should not change the Image File Execution Options (IFEO) keys in their
> installers," the callback filter really only seems in the way.
SetProcessUserModeExceptionPolicy function disables the callback filter without changing the registry key. The documentation is a bit confusing.
On Win7 SP1, the callback filter will be automatically disabled for Win7 manifested app per
http://blog.paulbetts.org/index.php/2010/07/20/the-case-of-the-disappearing-onload-exception-user-mode-callback-exceptions-in-x64/
We already added a Win7 manifest in bug 567497.
Attached patch patchSplinter Review
nsWindow::WindowProc is not the outermost function for plug-in windows. It may not be even called from plug-in's wndproc. It doesn't also cover the OOPP children.
Attachment #468301 - Flags: review?(benjamin)
I'll look at this patch, but it's certainly not related to the reports in comment #0, because we run all plugins OOPP now, and the reports are about the main Firefox process.

FWIW, all the crashes I see from OOM in the D2D code are caught correctly (Win7 64bit).
Nope it's only catching them when the crash is in mozalloc.dll , when the same crash happens in xul.dll breakpad doesn't fire. The crash in xul.dll it's this one

Faulting application name: firefox.exe, version: 2.0.0.3887, time stamp: 0x4c735b16
Faulting module name: xul.dll, version: 2.0.0.3887, time stamp: 0x4c735a4f
Exception code: 0xc0000005
Comment on attachment 468301 [details] [diff] [review]
patch

On what system did you test the combination of UnhandledExceptionFilter and DebugBreak? According to MSDN, DebugBreak causes an exception: is that going to get caught in some first-chance exception filter in MSVC?

It sounds like we really want to make a common place to have this, since it's currently different in nsWindow.cpp and this code.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
There's no reason to block beta5 on plugin crash-catching of this sort.
blocking2.0: beta5+ → final+
Really? Don't we want to be able to understand that sort of crash data before we ship the browser? Feels like this should be betaN to me ...
It's pretty edge-case. Doesn't matter much to me, except that the questions haven't been answered yet and this code is different from the widget code which already landed.
blocking2.0: final+ → betaN+
(In reply to comment #6)
> On what system did you test the combination of UnhandledExceptionFilter and
> DebugBreak?
I didn't test the patch yet, sorry. How can I test this reliably?

> According to MSDN, DebugBreak causes an exception: is that going to
> get caught in some first-chance exception filter in MSVC?
Debuggers will catch the first-chance STATUS_BREAKPOINT exception unlike most other exceptions.
Comment on attachment 468301 [details] [diff] [review]
patch

In either case, this and the widget code should share a helper function (put it in xpcom/base if necessary for sharing). Test it by crashing the testplugin from within an even handler, perhaps?
Attachment #468301 - Flags: review?(benjamin) → review-
Assignee: VYV03354 → benjamin
Whiteboard: [softblocker]
If this is 64-bit support for Windows, I'm not sure it needs to block Firefox 4 at all since we won't be supporting that platform.
This is 32-bit Firefox running on 64-bit windows.
Keywords: qawanted
This title is confusing, maybe say "WoW64" instead of "windows 64".
Benjamin, if all that needs to be done here is comment 11, maybe I could take this?
Summary: Exceptions on windows 64 are not fatal → Win32 native exceptions (which could be crashes) on WoW64 are not fatal
I'm not sure, the issue keeps being confused about the behavior when a debugger is attached and when it's not. I'm pretty sure that we must not use ::DebugBreak when a debugger is not attached.
(In reply to comment #16)
> I'm not sure, the issue keeps being confused about the behavior when a debugger
> is attached and when it's not. I'm pretty sure that we must not use
> ::DebugBreak when a debugger is not attached.

So does wrapping that call with an IsDebuggerPresent() check address this concern?
I really don't want any ::DebugBreak for FF4. We can experiment later.
Attached patch Patch (v2)Splinter Review
Assignee: benjamin → ehsan
Attachment #511605 - Flags: review?(benjamin)
Comment on attachment 511605 [details] [diff] [review]
Patch (v2)

Protcted ? r=me with fixed spelling
Attachment #511605 - Flags: review?(benjamin) → review+
Whiteboard: [softblocker] → [softblocker][needs landing][fix spelling before landing]
http://hg.mozilla.org/mozilla-central/rev/3510c76c789f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: qawanted
Resolution: --- → FIXED
Whiteboard: [softblocker][needs landing][fix spelling before landing] → [softblocker]
Target Milestone: --- → mozilla2.0b12
Depends on: 633820
This makes the debugging experience slightly worse on native Win32 though; I clicked "Retry" on an assertion dialog assuming that the just-in-time debugger would launch, but instead the process simply terminated itself :-(
You need to log in before you can comment on or make changes to this bug.