943051 - ReserveBreakpadVM needs to pass a memory protection constant

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[(Away)]

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.

Comment 1

[(Away)]

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).

Comment 2

[(Away)]

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

Comment 3

[(Away)]

Attachment: Fix VirtualAlloc and VirtualFree flags for gBreakpadReservedVM

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

Comment 4

[Benjamin Smedberg]

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

Comment 5

[(no longer active)]

(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?

Comment 6

[(Away)]

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.

Comment 7

[Benjamin Smedberg]

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.

Comment 8

[Benjamin Smedberg]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/60b569a38a4f

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

(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 11

[Benjamin Smedberg]

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

Comment 12

[(Away)]

Attachment: Aurora: Fix VirtualAlloc and VirtualFree flags for gBreakpadReservedVM

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

Comment 13

[(Away)]

Attachment: Beta: Fix VirtualAlloc and VirtualFree flags for gBreakpadReservedVM

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8a10855baef1

Comment 16

[Elbart]

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

Comment 17

[Robert Kaiser]

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.

Comment 18

[Wayne Mery (:wsmwk)]

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.

Comment 19

[Benjamin Smedberg]

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.

Comment 20

[Elbart]

(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 (:[e-mail]) 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.

Comment 21

[Wayne Mery (:wsmwk)]

(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.

Comment 22

[(Away)]

Attachment: ESR24: Fix VirtualAlloc and VirtualFree flags for gBreakpadReservedVM

[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.

Comment 23

[Benjamin Smedberg]

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

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

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