The VirtualAlloc from bug 837835 doesn't actually reserve any space. The function aborts due to a missing parameter: 0:000> !gle LastErrorValue: (Win32) 0x57 (87) - The parameter is incorrect. LastStatusValue: (NTSTATUS) 0xc0000045 - The specified page protection was not valid. In a debugger I tried changing the parameter to PAGE_NOACCESS and that made it work.

Also, for the corresponding VirtualFree: http://msdn.microsoft.com/en-us/library/windows/desktop/aa366892%28v=vs.85%29.aspx dwSize [in] The size of the region of memory to be freed, in bytes. If the dwFreeType parameter is MEM_RELEASE, this parameter must be 0 (zero).

Out of curiosity, where does the value 12MB come from?

I stepped through with a debugger and confirmed that the APIs are happy with these parameters.

mostly pulled out of my ass. it may have been the size of the largest dll in Firefox.

(In reply to comment #4) > mostly pulled out of my ass. it may have been the size of the largest dll in > Firefox. We can get that number at runtime, can't we?

On my machine I see MiniDumpWriteDump requesting a total of 2.6MB from VirtualAlloc. Presumably this is in addition to whatever space it needs to map the loaded modules.

ok I'm wrong, xul.dll is 23MB. Why don't we up the reservation size to 32MB to cover growth and writedump internal usage.

Pushed this for you, since if it works I'd love to get it into the final Fx26 beta, "security issues only" be damned: https://hg.mozilla.org/integration/mozilla-inbound/rev/60b569a38a4f

(In reply to Benjamin Smedberg [:bsmedberg] from comment #8) > Pushed this for you, since if it works I'd love to get it into the final > Fx26 beta, "security issues only" be damned: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/60b569a38a4f What's the result here on Nightly? How can you know it worked? What's the risk of uplift?

Comment on attachment 8338104 [details] [diff] [review] Fix VirtualAlloc and VirtualFree flags for gBreakpadReservedVM Data shows that the rate of empty dumps dropped by maybe 90%: 20131120030202,24200,56 20131120062258,65322,117 20131121030201,86649,149 20131122030202,72636,106 20131123030208,93683,171 20131125030201,80302,155 20131126030201,14799,49 20131126052050,42341,97 20131127030201,38443,10 20131128030201,7293,9 [Approval Request Comment] Bug caused by (feature/regressing bug #): Pre-existing condition, OOM crashes don't have stacks User impact if declined: crashes are reported as empty-minidump instead of having useful data Testing completed (on m-c, etc.): landed, verified via crash-stats Risk to taking this patch (and alternatives if risky): this is very low risk String or IDL/UUID changes made by this patch: None

This is the patch as checked into trunk in comment 8, which includes Benjamin's size change. It merged cleanly to Aurora.

Beta version of the same patch. Trivial NULL/nullptr merge fixup.

https://hg.mozilla.org/releases/mozilla-aurora/rev/0b4d1b22e6e8 https://hg.mozilla.org/releases/mozilla-beta/rev/8a10855baef1

Could this be uplifted to ESR too? Maybe this would help decreasing the percentage of the EMPTY-topcrasher: https://crash-stats.mozilla.com/topcrasher/products/Firefox/versions/24.1.1esr

I don't see why we should take this on ESR. This does not reduce the overall amount of crashes, it just makes them appear with a different signature than EMPTY and giving us more ability to debug them. If we find very-low-risk fixes for some of those crashes out of our debugging, then we can talk about forwarding them to ESR, but I don't think a fix that just improves the ability to debug crashes is useful on a branch where we do not do direct debugging anyhow.

I strongly believe this should be taken on ESR because there is no risk and it can help us gain more data from the vast majority of the user base. And for Thunderbird specifically this will be very helpful because 1. almost all of our users are on ESR and 2. crash data from non-release builds is _totally_ insufficient for gauging trends and for finding user testcases needed to get both big and small crashes fixed. In short, improving crash data from release builds is extremely important. Please add this to ESR. Otherwise, riding the train means this won't help Thunderbird for many months.

In terms of Firefox there's no reason to take this on ESR. For Thunderbird I don't care much: this is very low risk and if tbird devs are actually going to act on the information then it seems fine.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #19) > In terms of Firefox there's no reason to take this on ESR. (In reply to Robert Kaiser (:kairo@mozilla.com) from comment #17) > I don't see why we should take this on ESR. This does not reduce the overall > amount of crashes, it just makes them appear with a different signature than > EMPTY and giving us more ability to debug them. So why don't ESR-users deserve that kind of additional info too? It's very unnerving to be greeted with the "EMPTY"-signature when reading the report after a crash, as there's no way to find out what caused the crash in the first place.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #19) > In terms of Firefox there's no reason to take this on ESR. For Thunderbird I > don't care much: this is very low risk and if tbird devs are actually going > to act on the information then it seems fine. THanks bsmedberg. dmajor, I'm assume a different patch will be needed for esr - can you make it so for approval‑mozilla‑esr24? Much appreciated.

[Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: better debugging of OOM crashes, particularly for Thunderbird User impact if declined: OOM crashes show up as EMPTY reports Fix Landed on Version: 28 Risk to taking this patch (and alternatives if risky): low risk String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

v/fixed, empty dumps are down by a lot.

Comment on attachment 8343316 [details] [diff] [review] ESR24: Fix VirtualAlloc and VirtualFree flags for gBreakpadReservedVM This is landed in 28, the next ESR will be 31, and since we wouldn't land any OOM crash fixed to ESR branch in the meantime unless they were sec-high/critical, it's not enough to justify breaking the criteria for uplifts to ESR24.

I see now that this was uplifted to 26, but the statement in 24 stands, this doesn't meet landing criteria.