ReserveBreakpadVM needs to pass a memory protection constant

VERIFIED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Core
General
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

Trunk
mozilla28
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26+ verified, firefox27+ verified, firefox28 verified, firefox-esr24- wontfix, b2g-v1.2 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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).
(Assignee)

Comment 2

4 years ago
Out of curiosity, where does the value 12MB come from?
Flags: needinfo?(benjamin)
(Assignee)

Comment 3

4 years ago
Created attachment 8338104 [details] [diff] [review]
Fix VirtualAlloc and VirtualFree flags for gBreakpadReservedVM

I stepped through with a debugger and confirmed that the APIs are happy with these parameters.
Attachment #8338104 - Flags: review?(benjamin)
(Assignee)

Updated

4 years ago
Assignee: nobody → dmajor

Comment 4

4 years ago
mostly pulled out of my ass. it may have been the size of the largest dll in Firefox.
Flags: needinfo?(benjamin)

Comment 5

4 years ago
(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?
(Assignee)

Comment 6

4 years ago
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

4 years ago
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.

Updated

4 years ago
Attachment #8338104 - Flags: review?(benjamin) → review+

Comment 8

4 years ago
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
status-firefox26: --- → affected
status-firefox27: --- → affected
tracking-firefox26: --- → ?
tracking-firefox27: --- → +
Target Milestone: --- → mozilla28
https://hg.mozilla.org/mozilla-central/rev/60b569a38a4f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(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?
Flags: needinfo?(benjamin)

Comment 11

4 years ago
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
Attachment #8338104 - Flags: approval-mozilla-beta?
Attachment #8338104 - Flags: approval-mozilla-aurora?
status-firefox28: --- → fixed
tracking-firefox26: ? → +
Attachment #8338104 - Flags: approval-mozilla-beta?
Attachment #8338104 - Flags: approval-mozilla-beta+
Attachment #8338104 - Flags: approval-mozilla-aurora?
Attachment #8338104 - Flags: approval-mozilla-aurora+
Flags: needinfo?(benjamin)
(Assignee)

Comment 12

4 years ago
Created attachment 8341177 [details] [diff] [review]
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.
Attachment #8338104 - Attachment is obsolete: true
Attachment #8341177 - Flags: review+
(Assignee)

Comment 13

4 years ago
Created attachment 8341178 [details] [diff] [review]
Beta: Fix VirtualAlloc and VirtualFree flags for gBreakpadReservedVM

Beta version of the same patch. Trivial NULL/nullptr merge fixup.
Attachment #8341178 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/0b4d1b22e6e8
https://hg.mozilla.org/releases/mozilla-beta/rev/8a10855baef1
status-firefox26: affected → fixed
status-firefox27: affected → fixed
Keywords: checkin-needed
status-b2g-v1.2: --- → fixed

Comment 16

4 years ago
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

4 years ago
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

4 years ago
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

4 years ago
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.

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Depends on: 946381

Comment 20

4 years ago
(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.

Comment 21

4 years ago
(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.
Flags: needinfo?(dmajor)
(Assignee)

Updated

4 years ago
status-firefox-esr24: --- → affected
tracking-firefox-esr24: --- → ?
Flags: needinfo?(dmajor)
(Assignee)

Comment 22

4 years ago
Created attachment 8343316 [details] [diff] [review]
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.
Attachment #8343316 - Flags: review+
Attachment #8343316 - Flags: approval-mozilla-esr24?

Comment 23

4 years ago
v/fixed, empty dumps are down by a lot.
Status: RESOLVED → VERIFIED
status-firefox26: fixed → verified
status-firefox27: fixed → verified
status-firefox28: fixed → verified
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.
Attachment #8343316 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24-
status-firefox-esr24: affected → wontfix
tracking-firefox-esr24: ? → -
I see now that this was uplifted to 26, but the statement in 24 stands, this doesn't meet landing criteria.
You need to log in before you can comment on or make changes to this bug.