Closed Bug 943051 Opened 6 years ago Closed 6 years ago

ReserveBreakpadVM needs to pass a memory protection constant

Categories

(Core :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 + verified
firefox27 + verified
firefox28 --- verified
firefox-esr24 - wontfix
b2g-v1.2 --- fixed

People

(Reporter: dmajor, Assigned: dmajor)

References

Details

Attachments

(3 files, 1 obsolete file)

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?
Flags: needinfo?(benjamin)
I stepped through with a debugger and confirmed that the APIs are happy with these parameters.
Attachment #8338104 - Flags: review?(benjamin)
Assignee: nobody → dmajor
mostly pulled out of my ass. it may have been the size of the largest dll in Firefox.
Flags: needinfo?(benjamin)
(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.
Attachment #8338104 - Flags: review?(benjamin) → review+
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
Target Milestone: --- → mozilla28
(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 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?
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)
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+
Beta version of the same patch. Trivial NULL/nullptr merge fixup.
Attachment #8341178 - Flags: review+
Keywords: checkin-needed
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.
Keywords: checkin-needed
Keywords: checkin-needed
Depends on: 946381
(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.
Flags: needinfo?(dmajor)
Flags: needinfo?(dmajor)
[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?
v/fixed, empty dumps are down by a lot.
Status: RESOLVED → 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-
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.